Re: Thread Safety Analysis and the Linux kernel

Greg Kroah-Hartman
2025-03-08T06:06:27Z

On Fri, Mar 07, 2025 at 03:35:17PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 07, 2025 at 03:22:33PM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Mar 07, 2025 at 01:52:25PM +0100, Peter Zijlstra wrote:
> > > On Fri, Mar 07, 2025 at 09:52:04AM +0100, Peter Zijlstra wrote:
> > >
> > > > Yeah, so IIRC I once proposed a guard that takes a NULL pointer to mean
> > > > not take the lock, but people had a bit of a fit.
> > > >
> > > > It would've allowed writing the thing like:
> > > >
> > > > {
> > > > guard(device)(parent);
> > > > device_release_driver(dev);
> > > > }
> > >
> > > So the below does compile... Greg, how revolted are you? :-)
> >
> > Eeek! But why?
>
> Right; I forgot to tell. This clang Thread Safety Analyser can't deal
> with conditional locks. Things like:
>
> if (parent)
> device_lock(parent)
> do_something();
> if (parent)
> device_unlock(parent)
>
> make it quite upset. The above would, once it properly understands the
> guards, make it think the parent lock was unconditionally taken. It
> effectively hides the condition from the analyser.
>
> But yes, first time I proposed something like this Linus had a wee bit
> of a wobble too :-) I figured this one at least has a different name.
>
> Trouble is, this kind of pattern is quite common -- lots of driver code
> has it. The alternative is disabling analysis for the entire function,
> with the obvious down-side it won't find anything else in there either.
>
> So I'm currently exploring how far we can push changing the code to
> suit the analyser, because Aaron (co-author of said clang feature) is
> quite hesitant to even consider trying to fix this.
>
> Fixing this in the analyser would be near turning it into an interpreter
> and risk running into the halting problem at compile time -- not a
> pretty thought either.

Ah, thanks for the explaination, that makes more sense.

> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 5a1f05198114..7c95e7800b89 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -4796,33 +4796,30 @@ void device_shutdown(void)
> > > spin_unlock(&devices_kset->list_lock);
> > >
> > > /* hold lock to avoid race with probe/release */
> > > - if (parent)
> > > - device_lock(parent);
> > > - device_lock(dev);
> > > -
> > > - /* Don't allow any more runtime suspends */
> > > - pm_runtime_get_noresume(dev);
> > > - pm_runtime_barrier(dev);
> > > -
> > > - if (dev->class && dev->class->shutdown_pre) {
> > > - if (initcall_debug)
> > > - dev_info(dev, "shutdown_pre\n");
> > > - dev->class->shutdown_pre(dev);
> > > - }
> > > - if (dev->bus && dev->bus->shutdown) {
> > > - if (initcall_debug)
> > > - dev_info(dev, "shutdown\n");
> > > - dev->bus->shutdown(dev);
> > > - } else if (dev->driver && dev->driver->shutdown) {
> > > - if (initcall_debug)
> > > - dev_info(dev, "shutdown\n");
> > > - dev->driver->shutdown(dev);
> > > + {
> > > + guard(device_cond)(parent);
> >
> > This is just so subtle it's scary. I don't like that.
>
> Yeah, I was afraid of that. It's basically, if parent, take the lock,
> otherwise nop out.
>
> I don't suppose its better when written like: guard(if_device)(parent);
> ? I mean, its just naming, but sometimes that's all it takes.

Naming matters here, so yes, a better way would be essential if you want
to do this. This last suggestion is better, but still odd. How about
something like:
guard(if_exists)(parent);