linux-kernel - Re: [PATCH] driver core: bus: Mark an impossible error path with WARN_ON() in bus_add

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: <2ef86bb7-def3-4e2f-b4c4-f054b17bcbc7@icloud.com>
Date: Sun, 15 Sep 2024 22:15:27 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, linux-kernel@...r.kernel.org,
 Zijun Hu <quic_zijuhu@...cinc.com>
Subject: Re: [PATCH] driver core: bus: Mark an impossible error path with
 WARN_ON() in bus_add_driver()

On 2024/9/15 21:55, Greg Kroah-Hartman wrote:
> On Sun, Sep 15, 2024 at 09:38:15PM +0800, Zijun Hu wrote:
>> On 2024/9/15 21:00, Greg Kroah-Hartman wrote:
>>> On Sun, Sep 15, 2024 at 06:22:05PM +0800, Zijun Hu wrote:
>>>> From: Zijun Hu <quic_zijuhu@...cinc.com>
>>>>
>>>> driver_attach() called by bus_add_driver() always returns 0, so its
>>>> corresponding error path will never happen, hence mark the impossible
>>>> error path with WARN_ON() to remind readers to disregard it.
>>>
>>> So you just caused the machine to crash and reboot if that happens
>>> (remember, panic-on-warn is enabled in a few billion Linux systems...)
>>>
[cut]
>> what i want to show is that this error patch will never happen here
>> currently, so readers can disregard it.
> 
> Then just remove it, and document in the changelog text why it can never
> happen.  But if it can never happen, then why is the function returning
> anything at all?
> 

the only condition for driver_attach() returning error will be
impossible within bus_add_driver()'s context as shown below:

int bus_add_driver(struct device_driver *drv)
{
	struct subsys_private *sp = bus_to_subsys(drv->bus);
	struct driver_private *priv;
	int error = 0;

	if (!sp)
		return -EINVAL;
		....
		
	if (sp->drivers_autoprobe) {
		error = driver_attach(drv);
		if (error)
			goto out_del_list;
	}
...
}

int driver_attach(const struct device_driver *drv)
{
return bus_for_each_dev(drv->bus, NULL, (void *)drv, __driver_attach);
}

int bus_for_each_dev(const struct bus_type *bus, struct device *start,
		     void *data, int (*fn)(struct device *, void *))
{
	struct subsys_private *sp = bus_to_subsys(bus);
	struct klist_iter i;
	struct device *dev;
	int error = 0;

	if (!sp)
		return -EINVAL;   // this is the only condition for
                                  // driver_attach() to return error.

	klist_iter_init_node(&sp->klist_devices, &i,
			     (start ? &start->p->knode_bus : NULL));
	while (!error && (dev = next_device(&i)))
		error = fn(dev, data);
	klist_iter_exit(&i);
	subsys_put(sp);
	return error;
}

int __driver_attach(struct device *dev, void *data)() will always
return 0 even if it has int as return value type.

> thanks,
> 
> greg k-h