Re: [PATCH v4 08/12] PCI: Introduce /sys/bus/pci/devices/.../remove

[Greg KH <greg@kroah.com>]

On Thu, Mar 19, 2009 at 02:43:34AM -0700, Andrew Morton wrote:
> On Wed, 18 Mar 2009 16:40:06 -0600 Alex Chiang <achiang@hp.com> wrote:
> 
> > This patch adds an attribute named "remove" to a PCI device's sysfs
> > directory.  Writing a non-zero value to this attribute will remove the PCI
> > device and any children of it.
> > 
> > Trent Piepho wrote the original implementation and documentation.
> > 
> > Thanks to Vegard Nossum for testing under kmemcheck and finding locking
> > issues with the sysfs interface.
> > 
> > ...
> >
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -246,6 +246,47 @@ struct bus_attribute pci_bus_attrs[] = {
> >  	__ATTR(rescan, S_IWUSR, NULL, bus_rescan_store),
> >  	__ATTR_NULL
> >  };
> > +
> > +static void remove_callback(struct device *dev)
> > +{
> > +	int bridge = 0;
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > +	mutex_lock(&pci_remove_rescan_mutex);
> > +
> > +	if (pdev->subordinate)
> > +		bridge = 1;
> > +
> > +	pci_remove_bus_device(pdev);
> > +	if (bridge && list_empty(&pdev->bus->devices))
> > +		pci_remove_bus(pdev->bus);
> > +
> > +	mutex_unlock(&pci_remove_rescan_mutex);
> > +}
> > +
> > +static ssize_t
> > +remove_store(struct device *dev, struct device_attribute *dummy,
> > +	     const char *buf, size_t count)
> > +{
> > +	int ret = 0;
> > +	unsigned long val;
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > +	if (strict_strtoul(buf, 0, &val) < 0)
> > +		return -EINVAL;
> > +
> > +	if (!capable(CAP_SYS_ADMIN))
> > +		return -EPERM;
> > +
> > +	if (pdev->subordinate && pci_is_root_bus(pdev->bus))
> > +		return -EBUSY;
> > +
> > +	if (val)
> > +		ret = device_schedule_callback(dev, remove_callback);
> > +	if (ret)
> > +		count = ret;
> > +	return count;
> > +}
> >  #endif
> 
> It is very hard for the reader (this one at least) to work out why
> device_schedule_callback() is used here, instead of simply doing the work
> directly.
> 
> The way to solve that problem is to add a code comment.
> 
> Given that we're in a sysfs write() handler where no relevant locks at all
> are held, it seems rather weird that we cannot perform this operation
> synchronously, but no doubt the comment will explain all of this.
> 
> Do we need the CAP_SYS_ADMIN check if the sysfs file permissions are
> correct?  (I keep on asking this then forgetting the answer).

You can do this kind of check if you want to be paranoid.  You can
specify the mode of the file when you create it, but if a properly
permissive user (like root) changes the mode, it sticks, so that a
"normal" use could then write to the file.

Usually I wouldn't recommend checking, as it's overkill if you set the
mode properly in the code.

thanks,

greg k-h