[PATCH v2] drivers: core: synchronize really_probe() and dev_uevent()

[PATCH v2] drivers: core: synchronize really_probe() and dev_uevent()

[Dirk Behme]

Synchronize the dev->driver usage in really_probe() and dev_uevent().
These can run in different threads, what can result in the following
race condition for dev->driver uninitialization:

Thread #1:
==========

really_probe() {
...
probe_failed:
...
device_unbind_cleanup(dev) {
    ...
    dev->driver = NULL;   // <= Failed probe sets dev->driver to NULL
    ...
    }
...
}

Thread #2:
==========

dev_uevent() {
...
if (dev->driver)
      // If dev->driver is NULLed from really_probe() from here on,
      // after above check, the system crashes
      add_uevent_var(env, "DRIVER=%s", dev->driver->name);
...
}

really_probe() holds the lock, already. So nothing needs to be done
there. dev_uevent() is called with lock held, often, too. But not
always. What implies that we can't add any locking in dev_uevent()
itself. So fix this race by adding the lock to the non-protected
path. This is the path where above race is observed:

 dev_uevent+0x235/0x380
 uevent_show+0x10c/0x1f0  <= Add lock here
 dev_attr_show+0x3a/0xa0
 sysfs_kf_seq_show+0x17c/0x250
 kernfs_seq_show+0x7c/0x90
 seq_read_iter+0x2d7/0x940
 kernfs_fop_read_iter+0xc6/0x310
 vfs_read+0x5bc/0x6b0
 ksys_read+0xeb/0x1b0
 __x64_sys_read+0x42/0x50
 x64_sys_call+0x27ad/0x2d30
 do_syscall_64+0xcd/0x1d0
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Similar cases are reported by syzkaller in

https://syzkaller.appspot.com/bug?extid=ffa8143439596313a85a

But these are regarding the *initialization* of dev->driver

dev->driver = drv;

As this switches dev->driver to non-NULL these reports can be considered
to be false-positives (which should be "fixed" by this commit, as well,
though).

The same issue was reported and tried to be fixed back in 2015 in

https://lore.kernel.org/lkml/1421259054-2574-1-git-send-email-a.sangwan@samsung.com/

already.

Fixes: 239378f16aa1 ("Driver core: add uevent vars for devices of a class")
Cc: syzbot+ffa8143439596313a85a@syzkaller.appspotmail.com
Cc: Ashish Sangwan <a.sangwan@samsung.com>,
Cc: Namjae Jeon <namjae.jeon@samsung.com>
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
Changes in v2: Use locking instead of READ_ONCE().

 drivers/base/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index b93f3c5716ae..e2a1dd015074 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2723,8 +2723,11 @@ static ssize_t uevent_show(struct device *dev, struct device_attribute *attr,
 	if (!env)
 		return -ENOMEM;
 
+	/* Synchronize with really_probe() */
+	device_lock(dev);
 	/* let the kset specific function add its keys */
 	retval = kset->uevent_ops->uevent(&dev->kobj, env);
+	device_unlock(dev);
 	if (retval)
 		goto out;
 
-- 
2.28.0

Re: [PATCH v2] drivers: core: synchronize really_probe() and dev_uevent()

[Dan Williams]

Dirk Behme wrote:
> Synchronize the dev->driver usage in really_probe() and dev_uevent().
> These can run in different threads, what can result in the following
> race condition for dev->driver uninitialization:

This fix introduces an ABBA deadlock scenario via the known antipattern
of taking the device_lock() within device attributes that are removed
while the lock is held.

Lockdep splat below. I previously reported this on a syzbot report
against nvdimm subsytems with a more complicated splat [1], but this one
is more straightforward.

Recall that the reason this lockdep report is not widespread is because
CXL and NVDIMM are among the only subsystems that add lockdep coverage
to device_lock() with a local key.

[1]: http://lore.kernel.org/667a2ae44c0c0_5be92947e@dwillia2-mobl3.amr.corp.intel.com.notmuch

One potential hack is something like this if it is backstopped with
synchronization between unregistering drivers from buses relative to
uevent callbacks for those buses:

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2b4c0624b704..dfba73ef39af 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2640,6 +2640,7 @@ static const char *dev_uevent_name(const struct kobject *kobj)
 static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
 {
        const struct device *dev = kobj_to_dev(kobj);
+       struct device_driver *driver;
        int retval = 0;
 
        /* add device node properties if present */
@@ -2668,8 +2669,14 @@ static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
        if (dev->type && dev->type->name)
                add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
 
-       if (dev->driver)
-               add_uevent_var(env, "DRIVER=%s", dev->driver->name);
+       /*
+        * While it is likely that this races driver detach, it is
+        * unlikely that any driver attached with this device is racing being
+        * freed relative to a uevent for the same device
+        */
+       driver = READ_ONCE(dev->driver);
+       if (driver)
+               add_uevent_var(env, "DRIVER=%s", driver->name);
 
        /* Add common DT information about the device */
        of_device_uevent(dev, env);


---


 ======================================================
 WARNING: possible circular locking dependency detected
 6.10.0-rc7+ #275 Tainted: G           OE    N
 ------------------------------------------------------
 modprobe/2374 is trying to acquire lock:
 ffff8c2270070de0 (kn->active#6){++++}-{0:0}, at: __kernfs_remove+0xde/0x220
 
 but task is already holding lock:
 ffff8c22016e88f8 (&cxl_root_key){+.+.}-{3:3}, at: device_release_driver_internal+0x39/0x210
 
 which lock already depends on the new lock.
 
 
 the existing dependency chain (in reverse order) is:
 
 -> #1 (&cxl_root_key){+.+.}-{3:3}:
        __mutex_lock+0x99/0xc30
        uevent_show+0xac/0x130
        dev_attr_show+0x18/0x40
        sysfs_kf_seq_show+0xac/0xf0
        seq_read_iter+0x110/0x450
        vfs_read+0x25b/0x340
        ksys_read+0x67/0xf0
        do_syscall_64+0x75/0x190
        entry_SYSCALL_64_after_hwframe+0x76/0x7e
 
 -> #0 (kn->active#6){++++}-{0:0}:
        __lock_acquire+0x121a/0x1fa0
        lock_acquire+0xd6/0x2e0
        kernfs_drain+0x1e9/0x200
        __kernfs_remove+0xde/0x220
        kernfs_remove_by_name_ns+0x5e/0xa0
        device_del+0x168/0x410
        device_unregister+0x13/0x60
        devres_release_all+0xb8/0x110
        device_unbind_cleanup+0xe/0x70
        device_release_driver_internal+0x1c7/0x210
        driver_detach+0x47/0x90
        bus_remove_driver+0x6c/0xf0
        cxl_acpi_exit+0xc/0x11 [cxl_acpi]
        __do_sys_delete_module.isra.0+0x181/0x260
        do_syscall_64+0x75/0x190
        entry_SYSCALL_64_after_hwframe+0x76/0x7e
 
 other info that might help us debug this:
 
  Possible unsafe locking scenario:
 
        CPU0                    CPU1
        ----                    ----
   lock(&cxl_root_key);
                                lock(kn->active#6);
                                lock(&cxl_root_key);
   lock(kn->active#6);
 
  *** DEADLOCK ***

Re: [PATCH v2] drivers: core: synchronize really_probe() and dev_uevent()

[Greg Kroah-Hartman]

On Thu, Jul 11, 2024 at 05:07:21PM -0700, Dan Williams wrote:
> Dirk Behme wrote:
> > Synchronize the dev->driver usage in really_probe() and dev_uevent().
> > These can run in different threads, what can result in the following
> > race condition for dev->driver uninitialization:
> 
> This fix introduces an ABBA deadlock scenario via the known antipattern
> of taking the device_lock() within device attributes that are removed
> while the lock is held.

Ugh, yes :(

device attributes should not be taking that lock, don't we have a
different call for an attribute that will be removing itself?

> Lockdep splat below. I previously reported this on a syzbot report
> against nvdimm subsytems with a more complicated splat [1], but this one
> is more straightforward.
> 
> Recall that the reason this lockdep report is not widespread is because
> CXL and NVDIMM are among the only subsystems that add lockdep coverage
> to device_lock() with a local key.
> 
> [1]: http://lore.kernel.org/667a2ae44c0c0_5be92947e@dwillia2-mobl3.amr.corp.intel.com.notmuch
> 
> One potential hack is something like this if it is backstopped with
> synchronization between unregistering drivers from buses relative to
> uevent callbacks for those buses:
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 2b4c0624b704..dfba73ef39af 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2640,6 +2640,7 @@ static const char *dev_uevent_name(const struct kobject *kobj)
>  static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
>  {
>         const struct device *dev = kobj_to_dev(kobj);
> +       struct device_driver *driver;
>         int retval = 0;
>  
>         /* add device node properties if present */
> @@ -2668,8 +2669,14 @@ static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
>         if (dev->type && dev->type->name)
>                 add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
>  
> -       if (dev->driver)
> -               add_uevent_var(env, "DRIVER=%s", dev->driver->name);
> +       /*
> +        * While it is likely that this races driver detach, it is
> +        * unlikely that any driver attached with this device is racing being
> +        * freed relative to a uevent for the same device
> +        */
> +       driver = READ_ONCE(dev->driver);
> +       if (driver)
> +               add_uevent_var(env, "DRIVER=%s", driver->name);
>  
>         /* Add common DT information about the device */
>         of_device_uevent(dev, env);
> 

I'll take this patch for now if you want to also include the removal of
the lock patch that caused your splat.

thanks,

greg k-h