linux-kernel - Re: [PATCH] sysfs: move assignment to be under lock in sysfs_remove

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 29 Oct 2013 18:25:53 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Greg KH <gregkh@...uxfoundation.org>, Tejun Heo <tj@...nel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] sysfs: move assignment to be under lock in sysfs_remove_dir()

On Tue, Oct 29, 2013 at 5:39 PM, Eric W. Biederman
<ebiederm@...ssion.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.

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.

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.

              Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/