Re: [PATCH] PCI/sysfs: Check IORESOURCE_DISABLED in resource mmap handler

[Krzysztof Wilczyński]

Hello,

> > > pci_mmap_resource() does not check IORESOURCE_DISABLED before mapping
> > > a PCI BAR resource into userspace. This allows new mmaps to succeed
> > > even after a device has been marked disabled or soft-unplugged by the
> > > driver to prevent further access.
> > 
> > Which driver disables resources?  Would this be some Amazon-specific thing
> > you are trying to fix?  Or are you just manually disabling a given device
> > using sysfs, or something like this?
> > 
> > For "soft-unplugged" device we have pci_dev_set_disconnected(), but this
> > does not check current flags set.
> > 
> > What is your use case here?
> > 
> > > Add the check to return -ENODEV when the resource is disabled, blocking
> > > new userspace mmaps of BAR resources after device removal.
> > > 
> > > Tested by marking the PCI BAR resource as disabled and verifying that
> > > a subsequent mmap attempt fails with -ENODEV.
> > 
> > Can you explain how did you do this?
> > 
> > > @@ -1089,6 +1089,9 @@ static int pci_mmap_resource(struct kobject *kobj, const struct bin_attribute *a
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	if (res->flags & IORESOURCE_DISABLED)
> > > +		return -ENODEV;
> > > +
> > 
> > This probably would be better if it checked IORESOURCE_DISABLED and
> > IORESOURCE_UNSET, but then probably using resource_assigned() would
> > be even better.
> 
> Yes, resource_assigned() makes more sense.
> 
> When considering Krzysztof's sysfs rework series in pci/sysfs, this all 
> should be handled in .is_visible and not in pci_mmap_resource().

We can't use the resource_assigned() sadly, but the flags is something we
definitely can check - I will add this, test, and send new series.

The callback, though, will be called on creation, or if someone calls
remove, create or update for the sysfs resource group - no driver does this
at the moment (some DRM drivers resize resources, but that's unrelated),
so I am not sure how Ravi is doing this at runtime.  Perhaps I am looking
wrong at the code somewhere...

> Also (FYI), alpha has its own pci_mmap_resource() (IIRC, it still has it 
> even after Krzysztof's series).

For the sparse/dense resources, yes.

Thank you!

	Krzysztof