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

From: Eric W. Biederman
Date: Wed Oct 30 2013 - 01:29:55 EST

Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:

> On Tue, Oct 29, 2013 at 5:39 PM, Eric W. Biederman
> <ebiederm@xxxxxxxxxxxx> 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)
in __sysfs_remove_dir very suspicious. I expect we want a

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.


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at