linux-kernel - Re: [syzbot] [kernfs?] possible deadlock in __kernfs

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] [day] [month] [year] [list]

Message-ID: <667a2ae44c0c0_5be92947e@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Date: Mon, 24 Jun 2024 19:26:44 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>, syzbot
	<syzbot+4762dd74e32532cda5ff@...kaller.appspotmail.com>,
	<syzkaller-bugs@...glegroups.com>, <tj@...nel.org>, Peter Zijlstra
	<peterz@...radead.org>, Vishal Verma <vishal.l.verma@...el.com>, Dave Jiang
	<dave.jiang@...el.com>, Ira Weiny <ira.weiny@...el.com>, Dan Williams
	<dan.j.williams@...el.com>, Ashish Sangwan <a.sangwan@...sung.com>, "Namjae
 Jeon" <namjae.jeon@...sung.com>, Dirk Behme <dirk.behme@...bosch.com>, "Greg
 Kroah-Hartman" <gregkh@...uxfoundation.org>
CC: <linux-kernel@...r.kernel.org>
Subject: Re: [syzbot] [kernfs?] possible deadlock in __kernfs_remove

Tetsuo Handa wrote:
> This dependency is added by recent bug fix commit c0a40097f0bc ("drivers:
> core: synchronize really_probe() and dev_uevent()").
> 
> Commit 4a0079bc7aae ("nvdimm: Replace lockdep_mutex with local lock
> classes") changed to assign nvdimm_namespace_key on dev->mutex instead of
> __lockdep_no_validate__, which made lockdep to report this dependency.

At first glance this looks like a true positive lockdep report. One
observation is that attributes that depend on dev->driver being stable
are typically more appropriate as a driver lifetime scoped attribute
rather than adding locking within the attribute handler.

So a rough idea for an alternate fix would be something like:

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 83d352394fdf..a0536e6eaea3 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -672,6 +672,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
                goto probe_failed;
        }

+       device_remove_file(dev, &dev_attr_uevent);
+       device_create_file(dev, &dev_attr_driver_uevent);
        ret = device_add_groups(dev, drv->dev_groups);
        if (ret) {
                dev_err(dev, "device_add_groups() failed\n");

...where the only difference between those 2 attributes is that
dev_attr_driver_uevent additionally appends the "DRIVER=" key to the
uevent.

The problem with the above though is userspace may not expect that it
needs to re-open the uevent file over a driver attach / detach event.

Either way I think c0a40097f0bc ("drivers: core: synchronize
really_probe() and dev_uevent()") is potentially more of a band-aid than
a fix.