On Fri, Jul 12, 2024 at 12:42:09PM -0700, Dan Williams wrote:
> uevent_show() wants to de-reference dev->driver->name. There is no clean
> way for a device attribute to de-reference dev->driver unless that
> attribute is defined via (struct device_driver).dev_groups. Instead, the
> anti-pattern of taking the device_lock() in the attribute handler risks
> deadlocks with code paths that remove device attributes while holding
> the lock.
>
> This deadlock is typically invisible to lockdep given the device_lock()
> is marked lockdep_set_novalidate_class(), but some subsystems allocate a
> local lockdep key for @dev->mutex to reveal reports of the form:
>
> ======================================================
> 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
>
> The observation though is that driver objects are typically much longer
> lived than device objects. It is reasonable to perform lockless
> de-reference of a @driver pointer even if it is racing detach from a
> device. Given the infrequency of driver unregistration, use
> synchronize_rcu() in module_remove_driver() to close any potential
> races. It is potentially overkill to suffer synchronize_rcu() just to
> handle the rare module removal racing uevent_show() event.
>
> Thanks to Tetsuo Handa for the debug analysis of the syzbot report [1].
>
> Fixes: c0a40097f0bc ("drivers: core: synchronize really_probe() and dev_uevent()")
> Reported-by: syzbot+4762dd74e32532cda5ff@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reported-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Closes: http://lore.kernel.org/5aa5558f-90a4-4864-b1b1-5d6784c5607d@xxxxxxxxxxxxxxxxxxx [1]
> Link: http://lore.kernel.org/669073b8ea479_5fffa294c1@xxxxxxxxxxxxxxxxxxxxxxxxx.notmuch
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Ashish Sangwan <a.sangwan@xxxxxxxxxxx>
> Cc: Namjae Jeon <namjae.jeon@xxxxxxxxxxx>
> Cc: Dirk Behme <dirk.behme@xxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
> drivers/base/core.c | 13 ++++++++-----
> drivers/base/module.c | 4 ++++
> 2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 2b4c0624b704..b5399262198a 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -25,6 +25,7 @@
> #include <linux/mutex.h>
> #include <linux/pm_runtime.h>
> #include <linux/netdevice.h>
> +#include <linux/rcupdate.h>
> #include <linux/sched/signal.h>
> #include <linux/sched/mm.h>
> #include <linux/string_helpers.h>
> @@ -2640,6 +2641,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 +2670,12 @@ 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);
> + /* Synchronize with module_remove_driver() */
> + rcu_read_lock();
> + driver = READ_ONCE(dev->driver);
> + if (driver)
> + add_uevent_var(env, "DRIVER=%s", driver->name);
> + rcu_read_unlock();
It's a lot of work for a "simple" thing of just "tell userspace what
happened" type of thing. But it makes sense.
I'll queue this up after -rc1 is out, there's no rush before then as
it's only lockdep warnings happening now, right?
thanks,
greg k-h
