linux-kernel - Re: [PATCH RFC] drivers/core: Replace lockdep_set_novalidate

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:   Sun, 12 Feb 2023 14:14:02 -0500
From:   Kent Overstreet <kent.overstreet@...ux.dev>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Kent Overstreet <kent.overstreet@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Coly Li <colyli@...e.de>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        syzkaller <syzkaller@...glegroups.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Boqun Feng <boqun.feng@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        USB list <linux-usb@...r.kernel.org>,
        Hillf Danton <hdanton@...a.com>
Subject: Re: [PATCH RFC] drivers/core: Replace lockdep_set_novalidate_class()
 with unique class keys

On Sun, Feb 12, 2023 at 10:23:44AM -0500, Alan Stern wrote:
> On Sat, Feb 11, 2023 at 10:10:23PM -0500, Kent Overstreet wrote:
> > So IMO the more correct way to do this would be to change
> > device_initialize() to __device_initialize(), have it take a
> > lock_class_key as a parameter, and then use __mutex_init() instead of
> > mutex_init().
> 
> Yes, maybe.  The increase in code size might outweigh the space saved.

Where would the increase in code size come from? And whatever effect
would only be when lockdep is enabled, so not a concern.

But consider that the name of a lock as registered with lockdep really
should correspond to a source code location - i.e. it should be
something you can grep for. (We should probably consider adding file and
line number to that string, since current names are not unambiguous).

Whereas in your pass, you're calling lockdep_set_class(), which
generates a class name via stringifcation - with the same name every
time.

Definitely _don't_ do that. With your patch, the generated lockdep
traces will be useless.

> > But let's think about this more. Will there ever be situations where
> > lock ordering is dependent on what hardware is plugged into what, or
> > what hardware is plugged in first?
> 
> Device lock ordering is always dependent on what hardware is plugged 
> into what.  However, I'm not aware of any situations where, given two 
> different kinds of hardware, either one could be plugged into the other.
> Such things may exist but I can't think of any examples.

Different brands of hubs?

Lots of things have hubs embedded into them these days. 15 years ago I
had an Apple keyboard with an embedded hub.

> On the other hand, there are obvious cases where two pieces of the 
> _same_ kind of hardware can be plugged together in either order.  USB 
> hubs are a good example.
> 
> Consider the possibility that a driver might want to lock all of a 
> device's children at once.  (I don't know if this ever happens, but it 
> might.)  Provided it acquires the parent device's lock first, this is 
> utterly safe no matter what order the children are locked in.  Try 
> telling that to lockdep!  In the end, we'd probably have to enforce a 
> single fixed ordering.

The way you speak of hypotheticals and possibilities makes me think that
the actual locking rules are not ironed out at all :)

The patch I posted would be an improvement over the current situation,
because it'd get you checking w.r.t. other lock types - but with that
you would still have to have your own deadlock avoidance strategy, and
you'd have to be _really_ clear on what it is and how you know that
you're getting it right - you're still opting out of checking.

I think you should really be investigating wait/wound mutexes here.