Re: Thread Safety Analysis and the Linux kernel

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?

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

> +			guard(device)(dev);

This is fine, but really, why is this even needed?  None of the code you
are indenting here breaks out of the loop early, so how/why is this even
needed?

> +
> +			/* 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);
> +			}
>  		}
>  
> -		device_unlock(dev);
> -		if (parent)
> -			device_unlock(parent);
> -

So we save 3 lines?  Again, feels like total overkill.


>  		put_device(dev);
>  		put_device(parent);
>  
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index ec00e3f7af2b..bf72fec6f99b 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -300,7 +300,7 @@ static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
>  #define DEFINE_GUARD_COND(_name, _ext, _condlock) \
>  	__DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, true); \
>  	EXTEND_CLASS(_name, _ext, \
> -		     ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \
> +		     ({ void *_t = (_condlock) ? _T : NULL; _t; }), \
>  		     class_##_name##_t _T) \
>  	static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \
>  	{ return class_##_name##_lock_ptr(_T); }
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 80a5b3268986..4e7ebbb7fb64 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1046,6 +1046,7 @@ static inline void device_unlock(struct device *dev)
>  }
>  
>  DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T))
> +DEFINE_GUARD_COND(device, _cond, (_T ? (device_lock(_T), true) : false))

As I really don't want others to be calling this, worst case, just put
it in the driver core C file.

But again, ick ick ick, I worry this is going to be very subtle and
cause problems over time.

But very cool hack :)

thanks,

greg k-h