linux-kernel - Re: [PATCH] driver core: faux: Check device binding state by dedicated API device_is

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: <245f2248-e655-40e1-b1b0-6d99a5b4f365@icloud.com>
Date: Sat, 8 Mar 2025 06:45:18 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
 Danilo Krummrich <dakr@...nel.org>, linux-kernel@...r.kernel.org,
 Zijun Hu <quic_zijuhu@...cinc.com>
Subject: Re: [PATCH] driver core: faux: Check device binding state by
 dedicated API device_is_bound()

On 2025/3/7 22:04, Greg Kroah-Hartman wrote:
>> -	if (!dev->driver) {
>> +	/* Do not need to device_lock(dev) due to no race here */
>> +	if (!device_is_bound(dev)) {
> Ick, this feels wrong.  This is the driver core code, it can poke in

both 'dev->driver' and device_is_bound() is okay with this very special
context.

'dev->driver'    :     is binding or have bound successfully.
device_is_bound():     have bound successfully.

device_is_bound() may be more accurate here.

> dev->driver if it wants to, and as the lock is not held, and there is no
> lock even mentioned in this driver, the comment is confusing unless you
> dig and go read that device_is_bound() requires this.
> 

agree.

> Also, when we start to add locking requirements to functions like
> device_is_bound() (which we really should) this call will fail the
> check, right?  How are you going to explain that?  🙂

Generically, for 'dev->driver' and device_is_bound(), device_lock()
should be hold firstly to avoid race as driver core codes do.

this patch may be still okay if device_is_bound() is changed to
bool device_is_bound(struct device *dev)
{
	bool ret;

	device_lock(dev);
	ret = dev->p && klist_node_attached(&dev->p->knode_driver);
	device_unlock(dev);

	return ret;
}