Messages in this thread | | | From | (Eric W. Biederman) | Date | Tue, 29 Oct 2013 22:29:43 -0700 | Subject | Re: [PATCH] sysfs: move assignment to be under lock in sysfs_remove_dir() |
| |
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
| |