netdev - Re: [PATCH net-next v2 1/4] net: phy: fixed_phy: Fix return value check for fixed_phy_get

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]

Date: Thu, 17 Aug 2023 21:32:04 +0800
From: Ruan Jinjie <ruanjinjie@...wei.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
CC: <rafal@...ecki.pl>, <bcm-kernel-feedback-list@...adcom.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <opendmb@...il.com>, <florian.fainelli@...adcom.com>,
	<bryan.whitehead@...rochip.com>, <andrew@...n.ch>, <hkallweit1@...il.com>,
	<mdf@...nel.org>, <pgynther@...gle.com>,
	<Pavithra.Sathyanarayanan@...rochip.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 1/4] net: phy: fixed_phy: Fix return value
 check for fixed_phy_get_gpiod



On 2023/8/17 21:10, Russell King (Oracle) wrote:
> On Thu, Aug 17, 2023 at 08:16:28PM +0800, Ruan Jinjie wrote:
>> Since fixed_phy_get_gpiod() return NULL instead of ERR_PTR(),
>> if it fails, the IS_ERR() can never return the error. So check NULL
>> and return ERR_PTR(-EINVAL) if fails.
> 
> No, this is totally and utterly wrong, and this patch introduces a new
> bug. The original code is _correct_.
> 
>> Fixes: 71bd106d2567 ("net: fixed-phy: Add fixed_phy_register_with_gpiod() API")
>> Signed-off-by: Ruan Jinjie <ruanjinjie@...wei.com>
>> ---
>>  drivers/net/phy/fixed_phy.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
>> index aef739c20ac4..4e7406455b6e 100644
>> --- a/drivers/net/phy/fixed_phy.c
>> +++ b/drivers/net/phy/fixed_phy.c
>> @@ -239,8 +239,8 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,
>>  	/* Check if we have a GPIO associated with this fixed phy */
>>  	if (!gpiod) {
>>  		gpiod = fixed_phy_get_gpiod(np);
>> -		if (IS_ERR(gpiod))
>> -			return ERR_CAST(gpiod);
>> +		if (!gpiod)
>> +			return ERR_PTR(-EINVAL);
> 
> Let's look at fixed_phy_get_gpiod():
> 
>         gpiod = fwnode_gpiod_get_index(of_fwnode_handle(fixed_link_node),
>                                        "link", 0, GPIOD_IN, "mdio");
>         if (IS_ERR(gpiod) && PTR_ERR(gpiod) != -EPROBE_DEFER) {
> ...
> 		gpiod = NULL;
> 	}
> ...
> 	return gpiod;
> 
> If fwnode_gpiod_get_index() returns -EPROBE_DEFER, _then_ we return an
> error pointer. So it _does_ return an error pointer.
> 
> It also returns NULL when there is no device node passed to it, or
> if there is no fixed-link specifier, or there is some other error
> from fwnode_gpiod_get_index().
> 
> Otherwise, it returns a valid pointer to a gpio descriptor.
> 
> The gpio is optional. The device node is optional. When
> fixed_phy_get_gpiod() returns NULL, it is _not_ an error, it means
> that we don't have a GPIO. Just because something returns NULL does
> _not_ mean it's an error - please get out of that thinking, because
> if you don't your patches will introduce lots of new bugs.

Thank you, I understand what you mean, NULL is not an error here, so it
is not handled.

> 
> Only when fwnode_gpiod_get_index() wants to defer probe do we return
> an error.
> 
> So, sorry but NAK to this patch, it is incorrect.
>