linux-kernel - Re: [PATCH v2] drivers: core: synchronize really_probe() and dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <669073b8ea479_5fffa294c1@dwillia2-xfh.jf.intel.com.notmuch>
Date: Thu, 11 Jul 2024 17:07:21 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Dirk Behme <dirk.behme@...bosch.com>, <linux-kernel@...r.kernel.org>,
	"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>
CC: <dirk.behme@...bosch.com>, Rafael J Wysocki <rafael@...nel.org>, "Eugeniu
 Rosca" <eugeniu.rosca@...ch.com>,
	<syzbot+ffa8143439596313a85a@...kaller.appspotmail.com>, Ashish Sangwan
	<a.sangwan@...sung.com>, Namjae Jeon <namjae.jeon@...sung.com>,
	<linux-cxl@...r.kernel.org>
Subject: Re: [PATCH v2] drivers: core: synchronize really_probe() and
 dev_uevent()

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