Re: [PATCH v2 0/3] mm,kernfs,proc: Unmap mmaps of removed files via file->f_mapping

Hello,

> > The PCI resource files in sysfs and the /proc/bus/pci device files swap
> > their f_mapping to the shared iomem address space at open time, so that
> > revoke_iomem() can unmap userspace mappings when a driver claims a
> > region, see commit 636b21b50152 ("PCI: Revoke mappings like devmem").
> > 
> > Their VMAs are therefore attached to the shared address space, which
> > neither removal path reaches: kernfs_drain_open_files() unmaps the
> > sysfs inode's own mapping, which contains none of them, and
> > proc_entry_rundown() does not unmap anything at all.
> > 
> > As a result, userspace mappings of PCI BARs survive device removal,
> > and also survive a BAR resize on the sysfs side, keeping stale PTEs
> > into physical address space that the kernel may have reassigned since.
> > A mapping made before the device is removed still returns the previous
> > register value after the device has been released, through both
> > interfaces.
> 
> Thanks.  Can we please have full description of the userspace-visible
> effects of this?

Definitely.  From the PCI side, to put this into perspective:

Without this series, a process that maps a BAR keeps a fully working
mapping after the device is removed or its VFs torn down.  The same
applies when a BAR is resized, as the resize removes and recreates the
resource groups.  Nothing faults, so the process cannot tell the device
has gone.  A read returns the register contents as they were, because
removal clears Bus Master but leaves Memory Space Enable set, so the
device still decodes its BARs, and, as these attributes are writable,
a write is still accepted and reaches it.  Those addresses are released
on removal and can be assigned to another device, which a BAR resize
does explicitly, and the mapping then reaches whatever occupies them.

On the sysfs side this most likely has been a regression as the
kernfs_drain_open_files() unmapped these mappings correctly until
commit 636b21b50152 ("PCI: Revoke mappings like devmem") swapped
f_mapping to the shared iomem address space, after which the drain
walked an interval tree the VMAs were no longer on.  The procfs side
never unmapped anything, so there the behaviour is new.

A read here returns another device's register contents, and as read to
clear registers are common, it can also acknowledge an interrupt or clear
an error the owning driver has not seen.  A write can be worse, as an
offset that was a control register on the old device may be a doorbell,
a reset bit or a DMA descriptor address on the new one.

With this series the mapping is torn down as part of the removal, and
the next access raises SIGBUS, so userspace gets an error instead of
silently reading stale or unrelated registers.  Both interfaces behave
the same way afterwards.

Tested on 7.2-rc1 with and without the series applied.  In each case
the file is mmap'd, the device is removed while the mapping is held,
and the mapping is then accessed:

  - sysfs resource read after removal:
       before: returns 0x18140241, the value read before the removal
       after:  SIGBUS

   - sysfs resource written after removal:
       before: the write is accepted and reads back 0x00000000
       after:  SIGBUS on the write

   - sysfs resource mmap, fd closed, then read after removal:
       before: returns 0x18140241
       after:  SIGBUS

   - sysfs resource mmap, moved with mremap and split with munmap, then forked, both read after removal:
       before: parent and child both return 0x18140241
       after:  SIGBUS in both

   - two sysfs resources of different devices mmap, one device removed, both read:
       before: neither is unmapped, both return their values
       after:  the removed device raises SIGBUS, the other still returns 0x48140240

   - /proc/bus/pci used to mmap resource, read after removal:
       before: returns 0x18140241
       after:  SIGBUS

I hope this helps!

> AI review might have found a few things:
> 	https://sashiko.dev/#/patchset/[email protected]

I have seen the reviews.  Will reply to each.

Thank you!

	Krzysztof