| 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
| ||
|
Message-ID: <Zs8sMUxX7mnWZQnA@shell.armlinux.org.uk>
Date: Wed, 28 Aug 2024 14:54:57 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: davem@...emloft.net, Pantelis Antoniou <pantelis.antoniou@...il.com>,
Andrew Lunn <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
Christophe Leroy <christophe.leroy@...roup.eu>,
Florian Fainelli <f.fainelli@...il.com>,
Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com,
Herve Codina <herve.codina@...tlin.com>,
linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH net-next 6/6] net: ethernet: fs_enet: phylink conversion
On Wed, Aug 28, 2024 at 01:44:13PM +0200, Maxime Chevallier wrote:
> Hi Russell,
>
> On Wed, 28 Aug 2024 11:38:31 +0100
> "Russell King (Oracle)" <linux@...linux.org.uk> wrote:
>
> > On Wed, Aug 28, 2024 at 11:51:02AM +0200, Maxime Chevallier wrote:
> > > +static int fs_eth_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> > > +{
> > > + struct fs_enet_private *fep = netdev_priv(dev);
> > > +
> > > + if (!netif_running(dev))
> > > + return -EINVAL;
> >
> > Why do you need this check?
> >
>
> I included it as the original ioctl was phy_do_ioctl_running(), which
> includes that check.
>
> Is this check irrelevant with phylink ? I could only find macb and
> xilinx_axienet that do the same check in their ioctl.
>
> I can't tell you why that check is there in the first place in that
> driver, a quick grep search leads back from a major driver rework in
> 2011, at which point the check was already there...
int phylink_mii_ioctl(struct phylink *pl, struct ifreq *ifr, int cmd)
{
if (pl->phydev) {
... do PHY based access / pass on to phylib ...
} else {
... for reads:
... return emulated fixed-phy state if in fixed mode
... return emulated inband state if in inband mode
... for writes:
... ignore writes in fixed and inband modes
... otherwise return -EOPNOTSUPP
}
}
So, if a driver decides to connect the PHY during probe, the PHY will
always be accessible.
If a driver decides to connect the PHY during .ndo_open, the PHY will
only be accessible while the netdev is open, otherwise -EOPNOTSUPP
will be returned.
What do ethernet drivers return when they're not open or not having a
PHY? The answer is pretty random error codes.
drivers/net/ethernet/renesas/ravb_main.c- if (!netif_running(ndev))
drivers/net/ethernet/renesas/ravb_main.c- return -EINVAL;
drivers/net/ethernet/renesas/ravb_main.c-
drivers/net/ethernet/renesas/ravb_main.c- if (!phydev)
drivers/net/ethernet/renesas/ravb_main.c- return -ENODEV;
...
drivers/net/ethernet/renesas/ravb_main.c: return phy_mii_ioctl(phydev, req, cmd);
drivers/net/ethernet/renesas/rswitch.c- if (!netif_running(ndev))
drivers/net/ethernet/renesas/rswitch.c- return -EINVAL;
drivers/net/ethernet/8390/ax88796.c- if (!netif_running(dev))
drivers/net/ethernet/8390/ax88796.c- return -EINVAL;
drivers/net/ethernet/8390/ax88796.c-
drivers/net/ethernet/8390/ax88796.c- if (!phy_dev)
drivers/net/ethernet/8390/ax88796.c- return -ENODEV;
drivers/net/ethernet/marvell/mv643xx_eth.c- if (!dev->phydev)
drivers/net/ethernet/marvell/mv643xx_eth.c- return -ENOTSUPP;
drivers/net/ethernet/xilinx/xilinx_emaclite.c- if (!dev->phydev || !netif_running(dev))
drivers/net/ethernet/xilinx/xilinx_emaclite.c- return -EINVAL;
drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c- if (!(netif_running(netdev)))
drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c- return -EINVAL;
drivers/net/ethernet/actions/owl-emac.c- if (!netif_running(netdev))
drivers/net/ethernet/actions/owl-emac.c- return -EINVAL;
drivers/net/ethernet/engleder/tsnep_main.c- if (!netif_running(netdev))
drivers/net/ethernet/engleder/tsnep_main.c- return -EINVAL;
drivers/net/ethernet/ti/davinci_emac.c- if (!(netif_running(ndev)))
drivers/net/ethernet/ti/davinci_emac.c- return -EINVAL;
drivers/net/ethernet/ti/davinci_emac.c- if (ndev->phydev)
drivers/net/ethernet/ti/davinci_emac.c: return phy_mii_ioctl(ndev->phydev, ifrq, cmd);
drivers/net/ethernet/ti/davinci_emac.c- else
drivers/net/ethernet/ti/davinci_emac.c- return -EOPNOTSUPP;
drivers/net/ethernet/ti/netcp_ethss.c- if (phy)
drivers/net/ethernet/ti/netcp_ethss.c: return phy_mii_ioctl(phy, req, cmd);
drivers/net/ethernet/ti/netcp_ethss.c-
drivers/net/ethernet/ti/netcp_ethss.c- return -EOPNOTSUPP;
drivers/net/ethernet/ti/cpsw_priv.c- if (phy)
drivers/net/ethernet/ti/cpsw_priv.c: return phy_mii_ioctl(phy, req, cmd);
drivers/net/ethernet/ti/cpsw_priv.c-
drivers/net/ethernet/ti/cpsw_priv.c- return -EOPNOTSUPP;
drivers/net/ethernet/xscale/ixp4xx_eth.c- if (!netif_running(dev))
drivers/net/ethernet/xscale/ixp4xx_eth.c- return -EINVAL;
drivers/net/ethernet/freescale/gianfar.c- if (!netif_running(dev))
drivers/net/ethernet/freescale/gianfar.c- return -EINVAL;
...
drivers/net/ethernet/freescale/gianfar.c- if (!phydev)
drivers/net/ethernet/freescale/gianfar.c- return -ENODEV;
drivers/net/ethernet/freescale/gianfar.c-
drivers/net/ethernet/freescale/gianfar.c: return phy_mii_ioctl(phydev, rq, cmd);
drivers/net/ethernet/freescale/ucc_geth.c- if (!netif_running(dev))
drivers/net/ethernet/freescale/ucc_geth.c- return -EINVAL;
drivers/net/ethernet/freescale/ucc_geth.c-
drivers/net/ethernet/freescale/ucc_geth.c- if (!ugeth->phydev)
drivers/net/ethernet/freescale/ucc_geth.c- return -ENODEV;
drivers/net/ethernet/mediatek/mtk_star_emac.c- if (!netif_running(ndev))
drivers/net/ethernet/mediatek/mtk_star_emac.c- return -EINVAL;
drivers/net/ethernet/microchip/lan743x_main.c- if (!netif_running(netdev))
drivers/net/ethernet/microchip/lan743x_main.c- return -EINVAL;
drivers/net/ethernet/ethoc.c- if (!netif_running(dev))
drivers/net/ethernet/ethoc.c- return -EINVAL;
drivers/net/ethernet/broadcom/b44.c- int err = -EINVAL;
drivers/net/ethernet/broadcom/b44.c-
drivers/net/ethernet/broadcom/b44.c- if (!netif_running(dev))
drivers/net/ethernet/broadcom/b44.c- goto out;
drivers/net/ethernet/broadcom/sb1250-mac.c- if (!netif_running(dev) || !sc->phy_dev)
drivers/net/ethernet/broadcom/sb1250-mac.c- return -EINVAL;
drivers/net/usb/smsc95xx.c- if (!netif_running(netdev))
drivers/net/usb/smsc95xx.c- return -EINVAL;
Of 28 drivers that call phy_mii_ioctl():
- 17 drivers return EINVAL if !netif_running().
- 11 drivers do not check netif_running().
and if there's no phydev:
- 4 drivers return ENODEV
- 3 drivers return EOPNOTSUPP (plus all drivers converted to phylink)
- 2 drivers return EINVAL
- 1 driver returns ENOTSUPP
Phylib itself doesn't want NULL passed to phy_mii_ioctl(), so its up to
the driver to trap this if its calling phy_mii_ioctl(). However, phylib
also provides phy_do_ioctl() for hooking directly into .ndo_eth_ioctl,
which is:
int phy_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
{
if (!dev->phydev)
return -ENODEV;
return phy_mii_ioctl(dev->phydev, ifr, cmd);
}
then there's (as you point out) phy_do_ioctl_running(), which is:
int phy_do_ioctl_running(struct net_device *dev, struct ifreq *ifr, int cmd)
{
if (!netif_running(dev))
return -ENODEV;
return phy_do_ioctl(dev, ifr, cmd);
}
and this returns ENODEV if the netif isn't running, not EINVAL which
the majority of net drivers that manually check are doing. Maybe phylib
has the error code wrong?
In any case, I think it's pretty clear that there's no single "right"
error code for these cases, everyone just puts up on a piece of paper
with a donkey the names of various error codes, and while blindfolded
sticks a pin in to find the "right" error code to use! So, I don't see
any point in debating what is the "One True Right Error Code" for these
conditions.
Now, the question is... why do drivers do this netif_running() check?
Is it because "other drivers do" or is it because there's a valid
reason. There's no way to know, no one ever documents why the check
is there - and based on your response, it's "because other drivers do".
Without looking at the history, I wouldn't make any assumption about
using phy_do_ioctl_running() - that could be the result of a drive-by
cleanup patch converting code to use that helper.
At this point... this email has eaten up a substantial amount of time,
and I can't spend anymore time in front of the screen today so that's
the end of my contributions for today. Sorry.
--
*** please note that I probably will only be occasionally responsive
*** for an unknown period of time due to recent eye surgery making
*** reading quite difficult.
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists