linux-kernel - Re: [PATCH v2] driver core: bus: Remove an impossible error handling path 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: <c155f97e-b466-4740-ae8e-ce56bcdddf65@icloud.com>
Date: Sun, 13 Oct 2024 23:46:46 +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 v2] driver core: bus: Remove an impossible error handling
 path in bus_add_driver()

On 2024/10/13 23:02, Greg Kroah-Hartman wrote:
> On Tue, Sep 17, 2024 at 02:53:32PM +0800, Zijun Hu wrote:
>> On 2024/9/17 14:49, Zijun Hu wrote:
>>> From: Zijun Hu <quic_zijuhu@...cinc.com>
>>>
>>> For the following function call chain:
>>> API driver_register() -> bus_add_driver() -> driver_attach()
>>>
>>> There is an error handling path for driver_attach() returning non-zero
>>> or failure in bus_add_driver(), remove it with below reasons:
>>>
>>> - It is impossible for driver_attach() to have failure in bus_add_driver()
>>>   For int driver_attach(const struct device_driver *drv), the only factor
>>>   which makes it failed is that bus_to_subsys(@drv->bus) is NULL, but
>>>   the factor has been excluded by bus_add_driver() before calling it.
>>>
>>> - driver_attach() is irrelevant with driver_register(), so the former's
>>>   result should not also have an impact on the later.
>>>
>>> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>
>>> ---
>>> Changes in v2:
>>> - Remove the error handling path instead of WARN_ON() it.
>>> - Correct title and commit message
>>> - Link to v1: https://lore.kernel.org/r/20240915-bus_add_driver_fix-v1-1-ce5cf1f66601@quicinc.com
>>> ---
>>>  drivers/base/bus.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
>>> index 657c93c38b0d..54ff92aece92 100644
>>> --- a/drivers/base/bus.c
>>> +++ b/drivers/base/bus.c
>>> @@ -674,7 +674,8 @@ int bus_add_driver(struct device_driver *drv)
>>>  	if (sp->drivers_autoprobe) {
>>>  		error = driver_attach(drv);
>>>  		if (error)
>>> -			goto out_del_list;
>>> +			pr_warn("%s: failed to attach driver '%s' to bus '%s'\n",
>>> +				__func__, drv->name, sp->bus->name);
>>
>> driver_attach() has __must_check attribute and this error may be
>> inconsequential for driver_register(), so give pr_warn() here
> 
> Yes, but you  now ignore the error, so someone will come back and add
> that error handling in.  I'd just leave it as-is.
> 

driver API driver_attach() may ONLY have below error -EINVAL.
is it worthy of a __must_check attribute ?

i agree with you to leave it as-is if your answer is "YES".
otherwise, i would like to also simply remove __must_check attribute.

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);
	...
	int error = 0;

	if (!sp)
		return -EINVAL;  // this is the only error for the API.
	...
}

> thanks,
> 
> greg k-h