On Wed, Nov 04, 2020 at 12:12:15PM -0800, Dan Williams wrote:

On Wed, Nov 4, 2020 at 8:50 AM Bjorn Helgaas [off-list ref] wrote:

quoted

On Wed, Nov 04, 2020 at 09:44:04AM +0100, Daniel Vetter wrote:

quoted

On Tue, Nov 3, 2020 at 11:09 PM Dan Williams [off-list ref] wrote:

quoted

On Tue, Nov 3, 2020 at 1:28 PM Bjorn Helgaas [off-list ref] wrote:

quoted

On Fri, Oct 30, 2020 at 11:08:11AM +0100, Daniel Vetter wrote:

quoted

There's three ways to access PCI BARs from userspace: /dev/mem, sysfs
files, and the old proc interface. Two check against
iomem_is_exclusive, proc never did. And with CONFIG_IO_STRICT_DEVMEM,
this starts to matter, since we don't want random userspace having
access to PCI BARs while a driver is loaded and using it.

Fix this by adding the same iomem_is_exclusive() check we already have
on the sysfs side in pci_mmap_resource().

References: 90a545e98126 ("restrict /dev/mem to idle io memory ranges")
Signed-off-by: Daniel Vetter <redacted>

This is OK with me but it looks like IORESOURCE_EXCLUSIVE is currently
only used in a few places:

  e1000_probe() calls pci_request_selected_regions_exclusive(),
  ne_pci_probe() calls pci_request_regions_exclusive(),
  vmbus_allocate_mmio() calls request_mem_region_exclusive()

which raises the question of whether it's worth keeping
IORESOURCE_EXCLUSIVE at all.  I'm totally fine with removing it
completely.

Now that CONFIG_IO_STRICT_DEVMEM upgrades IORESOURCE_BUSY to
IORESOURCE_EXCLUSIVE semantics the latter has lost its meaning so I'd
be in favor of removing it as well.

Still has some value since it enforces exclusive access even if the
config isn't enabled, and iirc e1000 had some fun with userspace tools
clobbering the firmware and bricking the chip.

There's *some* value; I'm just skeptical since only three drivers use
it.

IORESOURCE_EXCLUSIVE is from e8de1481fd71 ("resource: allow MMIO
exclusivity for device drivers"), and the commit message says this is
only active when CONFIG_STRICT_DEVMEM is set.  I didn't check to see
whether that's still true.

That commit adds a bunch of wrappers and "__"-prefixed functions to
pass the IORESOURCE_EXCLUSIVE flag around.  That's a fair bit of
uglification for three drivers.

quoted

Another thing I kinda wondered, since pci maintainer is here: At least
in drivers/gpu I see very few drivers explicitly requestion regions
(this might be a historical artifact due to the shadow attach stuff
before we had real modesetting drivers). And pci core doesn't do that
either, even when a driver is bound. Is this intentional, or
should/could we do better? Since drivers work happily without
reserving regions I don't think "the drivers need to remember to do
this" will ever really work out well.

You're right, many drivers don't call pci_request_regions().  Maybe we
could do better, but I haven't looked into that recently.  There is a
related note in Documentation/PCI/pci.rst that's been there for a long
time (it refers to "pci_request_resources()", which has never existed
AFAICT).  I'm certainly open to proposals.

It seems a bug that the kernel permits MMIO regions with side effects
to be ioremap()'ed without request_mem_region() on the resource. I
wonder how much log spam would happen if ioremap() reported whenever a
non-IORESOURE_BUSY range was passed to it? The current state of
affairs to trust *remap users to have claimed their remap target seems
too ingrained to unwind now.

Yeah I think that's hopeless. I think the only feasible approach is if bus
drivers claim resources by default when a driver is bound (it should nest,
so if the driver claims again, I think that should all keep working), just
using the driver name. Probably with some special casing for legacy io
(vgaarb.c should claim these I guess). Still probably tons of fallout.

Once that's rolled out to all bus drivers we could perhaps add the ioremap
check without drowning in log spam. Still a multi-year project I think :-/
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel