Re: Thread Safety Analysis and the Linux kernel

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.

> > 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.