linux-kernel - Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CA+55aFyzYXSTTuVoGfVeC574-7i0zp3q39RTsiPn3GoRceuwAQ@mail.gmail.com>
Date:   Mon, 2 Jul 2018 12:24:46 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Benjamin Herrenschmidt <benh@...nel.crashing.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Joel Stanley <joel@....id.au>
Subject: Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier

On Mon, Jul 2, 2018 at 3:23 AM Benjamin Herrenschmidt
<benh@...nel.crashing.org> wrote:
>
> Let me try one last ditch attempt to convince you using maybe a
> different perspective: this is how sysfs is intended to work and how
> the device model already does everywhere else except the gluedirs.

So don't get me wrong. I don't think that patch is _wrong_. It may
well be the best thing we can do now.

I just think some of the arguments for the patch are bogus.

I still think that the auto-cleanup is actually a good thing in
general. Not because it simplifies things (which it can do), but
because it fundamentally *allows* you to use less locking.

And that ends up being my real reason to not like your patch all that
much. It depends on

 (a) an extra reference count

 (b) on fairly heavy-handed locking (ie the whole "lock on release
too, not just on allocation").

and I think both of those are non-optimal.

So:

> The actual lifetime of the struct device is handled *separately* by
> device_get/put which are just wrappers on kobject_get/put.

I agree that that is the end result of your patch (and perhaps the
buggy intent of the original code).

I just don't necessarily agree it's a great thing in general. It
basically uses sysfs visibility as an argument for why lifetimes
should differ from the refcounted lifetimes.

And that may be a practical argument, but I don't think it's a very
good argument in general. I think it would arguably be much better to
be less serialized, and depend on refcounting more, and make less of a
deal about the sysfs visibility.

For example, if we *really* add back the exact same device immediately
after removing it, and the device was still in use somehow (ie the
refcount never went down to zero), maybe we really should not have
reused the same name? Or if we do re-use the same name, maybe we
should have re-used the device node itself?

See what I'm saying? I understand where your patch is coming from, but
I am suspicious of the fact that it seems to want to re-use a name
(but not the object) that is by definition still in use.

Maybe that's the right thing in this case. Considering that we have a
real oops and a real problem, and I don't have an alternative patch, I
guess the "patches talk, bullshit walks" rule applies.

                Linus