netdev - Re: [PATCH 1/5] phylib: don't return NULL from get_phy

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

Message-ID: <20160427194932.GF29024@lunn.ch>
Date:	Wed, 27 Apr 2016 21:49:32 +0200
From:	Andrew Lunn <andrew@...n.ch>
To:	Vivien Didelot <vivien.didelot@...oirfairelinux.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
	f.fainelli@...il.com, arnd@...db.de, netdev@...r.kernel.org,
	kernel@...oirfairelinux.com
Subject: Re: [PATCH 1/5] phylib: don't return NULL from get_phy_device()

On Wed, Apr 27, 2016 at 03:30:57PM -0400, Vivien Didelot wrote:
> Hi David, All,
> 
> Sergei Shtylyov <sergei.shtylyov@...entembedded.com> writes:
> 
> > Arnd Bergmann asked that get_phy_device() returns either NULL or the error
> > value,  not both on error.  Do as he said, return ERR_PTR(-ENODEV) instead
> > of NULL when the PHY ID registers read as  all ones.
> >
> > Suggested-by: Arnd Bergmann <arnd@...db.de>
> > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
> >
> > ---
> >  drivers/net/phy/phy_device.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Index: net-next/drivers/net/phy/phy_device.c
> > ===================================================================
> > --- net-next.orig/drivers/net/phy/phy_device.c
> > +++ net-next/drivers/net/phy/phy_device.c
> > @@ -529,7 +529,7 @@ struct phy_device *get_phy_device(struct
> >  
> >  	/* If the phy_id is mostly Fs, there is no device there */
> >  	if ((phy_id & 0x1fffffff) == 0x1fffffff)
> > -		return NULL;
> > +		return ERR_PTR(-ENODEV);
> >  
> >  	return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
> >  }

This change is wrong, it needs reverting, or the call sights need
fixing to expect ENODEV.

The point is, the device not being there is not an error, with respect
to the code calling this function.

It gets called by mdiobus_scan()

struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
{
        struct phy_device *phydev;
        int err;

        phydev = get_phy_device(bus, addr, false);
        if (IS_ERR(phydev) || phydev == NULL)
                return phydev;

So before, we return NULL, if the device was not there. Now we return
ERR_PTR(-ENODEV).

This is being called by:

int __mdiobus_register(struct mii_bus *bus, struct module *owner)
{
        struct mdio_device *mdiodev;
...
        for (i = 0; i < PHY_MAX_ADDR; i++) {
                if ((bus->phy_mask & (1 << i)) == 0) {
                        struct phy_device *phydev;

                        phydev = mdiobus_scan(bus, i);
                        if (IS_ERR(phydev)) {
                                err = PTR_ERR(phydev);
                                goto error;
                        }
                }
        }

This is treating ERR_PTR(-ENODEV) as a fatal error, where as before
IS_ERR(NULL) would be false and it would continue scanning other
addresses on the bus.

Please revert this, or fix all the callsites such that ENODEV is not a
fatal error.

	     Andrew