Re: [PATCH, RFC] sysfs: only allow one scheduled removal callback per kobj

[Greg KH <gregkh@suse.de>]

On Tue, Mar 10, 2009 at 05:20:27PM -0600, Alex Chiang wrote:
> Hi Vegard, sysfs folks,
> 
> Vegard was nice enough to test my PCI remove/rescan patches under
> kmemcheck. Maybe "torture" is a more appropriate term. ;)
> 
> My patch series introduces a sysfs "remove" attribute for PCI
> devices, which will remove that device (and child devices).
> 
> 	http://thread.gmane.org/gmane.linux.kernel.pci/3495
> 
> Vegard decided that he wanted to do something like:
> 
> 	# while true ; do echo 1 > /sys/bus/pci/devices/.../remove ; done
> 
> which caused a nasty oops in my code. You can see the results of
> his testing in the thread I referenced above.
> 
> After looking at my code for a bit, I decided that maybe it
> wasn't completely my fault. ;) See, I'm using device_schedule_callback()

why?  Are you really in interrupt context here to need to do the remove
at a later time?

> which really is a wrapper around sysfs_schedule_callback() which
> is the way that a sysfs attribute is supposed to remove itself to
> prevent deadlock.

Yeah, it's the scsi code that needed this mess :(

If at all possible, I would recommend not using it.

> The problem that Vegard's test exposed is that if you repeatedly
> call a sysfs attribute that's supposed to remove itself using
> device_schedule_callback, we'll keep scheduling work queue tasks
> with a kobj that we really want to release.
> 
> [nb, I bet that /sys/bus/scsi/devices/.../delete will exhibit the
> same problems]

I think it's harder to remove devices multiple times, as it isn't done
through sysfs, but from another external request.  But I could be wrong.

> This is very racy, and at some point, whatever remove handler
> we've scheduled with device_schedule_callback will end up
> referencing a freed kobj.
> 
> I came up with the below patch which changes the semantics of
> device/sysfs_schedule_callback. We now only allow one in-flight
> callback per kobj, and return -EBUSY if that kobj already has a
> callback scheduled for it.
> 
> This patch, along with my updated 07/11 patch in my series,
> prevents at least the first oops that Vegard reported, and I
> suspect it prevents the second kmemcheck error too, although I
> haven't tested under kmemcheck (yet*).
> 
> I'm looking for comments on the approach I took, specifically:
> 
> 	- are we ok with the new restriction I imposed?
> 	- is it ok to return -EBUSY to our callers?
> 	- is the simple linked list proof of concept
> 	  implementation going to scale too poorly?
> 
> To answer my own first two questions, I checked for callers of
> both device_ and sysfs_schedule_callback, and it looks like
> everyone is using it the same way: to schedule themselves for
> removal. That is, although the interface could be used to
> schedule any sort of callback, the only use case is the removal
> use case. I don't think it will be a problem to limit ourselves
> to one remove callback per kobj.

I agree.

> Maybe this patch really wants to be a new interface called
> sysfs_schedule_callback_once or _single, where we check for an
> already-scheduled callback for a kobj, and if we pass, then we
> simply continue on to the existing, unchanged
> sysfs_schedule_callback. I don't feel too strongly about creating
> a new interface, but my belief is that changing the semantics of
> the existing interface is probably the better solution.
> 
> My opinion on my own third question is that removing a device is
> not in the performance path, so a simple linked list is
> sufficient.
> 
> Depending on the feedback here, I'll resend this patch with a
> full changelog (and giving credit to Vegard/kmemcheck as Ingo
> requested I do) or I can rework it.

I have no objection to this change.  But I really would recommend not
using this interface at all if possible.

thanks,

greg k-h