Re: [PATCH] sysfs: move assignment to be under lock in sysfs_remove_dir()

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

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Tue, Oct 29, 2013 at 5:39 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>
>> I don't have a strong feeling either way but how would that matter?
>> There is only ever one sd associated with a kobj.
>
> What does that matter? If you have multiple callers, they might try to
> free that one sd twice, since they could both see a non-NULL case.

>> And we better be under the sysfs_mutex when the assignment and and
>> sysfs_remove_dir are called.
>
> Not as far as I can tell. kobject_del() calls sysfs_remove_dir(), and
> I'm not seeing why that would be under the mutex.  The only locking I
> see is that sysfs_assoc_lock, which _isn't_ held for the reading of
> kobj->sd.
>
> Now, there may be other reasons for this all working (like the fact
> that only one user ever calls kobject_del() on any particular object,
> but it sure as hell isn't obvious. The fact that you seem to be
> confused about this only proves my point.

I never actually looked deeply into it, and I was working from several
year old memory and a quick skim of the patch when I asked the question.

The protection we have previous to this patch is that syfs_remove_dir is
only sane to call once.

Which makes the code that does:
	if (!dir_sd)
        	return;
in __sysfs_remove_dir very suspicious.   I expect we want a
WARN_ON(!dir_sd);

But the entire directory removal process and working on sysfs stopped
being fun before I managed to get that cleaned up.  And unless I missed
something go by Tejun is going to go generalize this thing before this
bit gets cleaned up.  Sigh.

> Besides, the "design pattern" of having a lock for the assignment, but
> then reading the value without that lock seems to be all kinds of
> f*cking stupid, wouldn't you agree? I'm really not seeing how that
> could _ever_ be something you make excuses for in the first place.
> Even if there is some external locking (which, as far as I can tell,
> there is not), that would just raise the question as to what reason
> that spinlock has to exist at all.

I wasn't making excuses I was just trying to understand the reasoning
for this little patch flying through my inbox.

On an equally bizarre note.  I don't understand why we have a separate
spinlock there.  Looks...  Sigh.  We use a different lock from
everything as a premature optimization so that sysfs_remove_dir could be
modified to just take a sysfs_dirent, and all of the kobject handling
could be removed.

Sigh. It was never in my way and while I was working on the code that
there was a good locking reason for doing that silly thing.

> The code doesn't make any sense with the locking the way it is now. It
> might _work_, of course, but it sure as hell doesn't make sense.

In net I agree.

Eric