Re: Thread Safety Analysis and the Linux kernel

Greg Kroah-Hartman
2025-03-07T14:22:37Z

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