Re: Thread Safety Analysis and the Linux kernel

Peter Zijlstra
2025-03-07T12:52:30Z

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? :-)

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);
+ guard(device)(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);
+ }
}

- device_unlock(dev);
- if (parent)
- device_unlock(parent);
-
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))

static inline void device_lock_assert(struct device *dev)
{