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

[Alex Chiang <achiang@hp.com>]

* Tejun Heo <tj@kernel.org>:
> Alex Chiang wrote:
> > * 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?
> > 
> > What other interface can I use to remove objects from sysfs?
> 
> I haven't read your code yet but I seem to recall doing something
> similar.  Ah.. okay, this one didn't get in and I forgot about this.
> 
>   http://thread.gmane.org/gmane.linux.kernel/582130
> 
> But, yeah, committing suicide is currently quite hariy.  I tought SCSI
> did it correctly with all the grab/release dances.  Does SCSI have the
> problem too?

I haven't dived into the SCSI code yet, but they are doing some
sort of magic that I don't understand with their state machine.

Regardless, I think we have two issues.

	1. The existing callback mechanism that everyone hates
	has a "bug".

	2. Your suicide patches haven't made it into mainline yet.

The reason that I think that the "bug" is with the callback
mechanism is because any caller can repeatedly schedule suicide
over and over again, and the callback handler will eventually get
a stale pointer. Rather than make all the callsites handle the
locking, doesn't it make more sense for the infrastructure to do
it?

I realize we're trying to fix something that everyone wants to go
away, but the PCI rescan patches add some pretty useful
functionality and pretty much ready to go except for this. I
could add the bookkeeping into my suicide path, but that's
actually a slightly bigger patch, because now I have to malloc my
own callback structs. And again, I think it's more appropriate to
put that sort of code into the core.

Can we fix 1 in the short term and move towards 2 as the real
solution?

Thanks.

/ac