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

Date: Thu, 17 Aug 2023 14:10:11 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Ruan Jinjie <ruanjinjie@...wei.com>
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 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.

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!