Re: [PATCH 04/24] sysfs: Normalize removing sysfs directories.

[ebiederm@xmission.com (Eric W. Biederman)]

Tejun Heo <tj@kernel.org> writes:

> Hello,
>
> Eric W. Biederman wrote:
>> I guess we are going to have to disagree on this one.
>
> Yeap, seems like it.
>
>> My take is simply that a correct user has to wait until no one else
>> can find the kobject before calling kobject_del.  At which point
>> races are impossible, and it doesn't matter if sysfs_mutex is held
>> across the entire operation.
>
> This one also is a matter of degree.  Way back when users could crash
> sysfs reliably from userland, the sysfs code had a lot of assumptions
> about object lifetime and synchronizaion which even the sysfs code
> itself didn't really follow leading to fragility.  My focus while
> restructuring the code was to make the code behave as expected by the
> usual conventions.  It could be that I'm a bit paranoid about this,
> but in general I really don't like when low level code doesn't do its
> due diligence to save several hours of effort to implement clean
> semantics, but again there's nothing wrong with your due and my due
> being different.

Given a history of user space was crashing sysfs I can see where you
would be nervous.

I come from the perspective of sysfs still having bugs (although not
easily triggered), but the code is so complicated that people can't
see them because there is just so much code.  So I value every little
bit of extra simplicity I can.

>> For the long term I still intend to kill __sysfs_remove_dir.  Just
>> not in this patch series.
>
> Yeah, sysfs code is in the middle of two sane ways.  sysfs either has
> to deal with children creation/removal including atomicity of the
> operations or it should force its users to do so.  I prefer the former
> but any would be better than the current situation.

What user space cares about is that we perform the additions in
the following order:

kobject_add()
sysfs_add_file()
kobject_uevent()

So that when user space receives the event it can consult sysfs and
be safe in assuming all of the attributes for the device are present.

Since device_add calls kobject_uevent we must have all of the
attributes setup in a manner that device_add can add them to sysfs,
before device_add calls kobject_uevent.


So for most of the kernel today, the right solution is to setup attribute
groups and set them on the device before device_add is called.  For
most of the code that really isn't too hard.

Making that also to get all of the attributes removed in device_del
before sysfs_remove_dir is called.

So for the common case all of the complexity is handled at the device
layer today.

There are a few more cases but that just takes time.


When I add that to the fact that there are cases where sysfs simply
gets it wrong by deleting things like /sys/dev/char and
/sys/dev/block.  I can't see keeping __sysfs_remove_dir indefinitely.


Just adding my warning has turned up an issue in the scsi target vs
host lifetime rules that does the wrong thing in sysfs, and that isn't
something I can see sysfs making any easier by trying to be helpful.

Eric