Re: Thread Safety Analysis and the Linux kernel

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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);