Re: [PATCH net] net: lan966x: fix kernel oops on ioctl when I/F is down

Re: [PATCH net] net: lan966x: fix kernel oops on ioctl when I/F is down

[Michael Walle]

Am 2022-03-26 03:17, schrieb Andrew Lunn:
> On Sat, Mar 26, 2022 at 01:02:51AM +0100, Michael Walle wrote:
>> A SIOCGMIIPHY ioctl will cause a kernel oops when the interface is 
>> down.
>> Fix it by checking the state and if it's no running, return an error.
> 
> s/no/not/
> 
> I don't think it is just SIOCGMIIPHY. phy_has_hwtstamp(dev->phydev) is
> probably also an issue. The phy is connected in open, and disconnected
> in stop. So dev->phydev is not valid outside of that time.

phy_has_hwtstamp() handles NULL gracefully. And I guess the MAC 
timestamp
handling is working if there is no phydev. Not sure if the interface
has to be up though.

> But i'm also not sure it is guaranteed to be valid while the interface
> is up. The driver uses phylink, so there could be an SFP attached to a
> port, in which case, dev->phydev will not be set.

I wonder if we should use phylink_mii_ioctl() here. Maybe as a seperate
patch for the net-next if its open again?

> So rather than testing of running, it would be better to test if the
> phydev is NULL or not.

What about the following:

static int lan966x_port_ioctl(struct net_device *dev, struct ifreq *ifr,
			     int cmd)
{
	struct lan966x_port *port = netdev_priv(dev);

	if (!phy_has_hwtstamp(dev->phydev) && port->lan966x->ptp) {
		switch (cmd) {
		case SIOCSHWTSTAMP:
			return lan966x_ptp_hwtstamp_set(port, ifr);
		case SIOCGHWTSTAMP:
			return lan966x_ptp_hwtstamp_get(port, ifr);
		}
	}

	if (!dev->phydev)
		return -ENODEV;

	return phy_mii_ioctl(dev->phydev, ifr, cmd);
}

-michael

Re: [PATCH net] net: lan966x: fix kernel oops on ioctl when I/F is down

[Andrew Lunn]

> > So rather than testing of running, it would be better to test if the
> > phydev is NULL or not.
> 
> What about the following:
> 
> static int lan966x_port_ioctl(struct net_device *dev, struct ifreq *ifr,
> 			     int cmd)
> {
> 	struct lan966x_port *port = netdev_priv(dev);
> 
> 	if (!phy_has_hwtstamp(dev->phydev) && port->lan966x->ptp) {
> 		switch (cmd) {
> 		case SIOCSHWTSTAMP:
> 			return lan966x_ptp_hwtstamp_set(port, ifr);
> 		case SIOCGHWTSTAMP:
> 			return lan966x_ptp_hwtstamp_get(port, ifr);
> 		}
> 	}
> 
> 	if (!dev->phydev)
> 		return -ENODEV;
> 
> 	return phy_mii_ioctl(dev->phydev, ifr, cmd);

Yes, that is good.

     Andrew

Re: [PATCH net] net: lan966x: fix kernel oops on ioctl when I/F is down

[Andrew Lunn]

On Sat, Mar 26, 2022 at 01:02:51AM +0100, Michael Walle wrote:
> A SIOCGMIIPHY ioctl will cause a kernel oops when the interface is down.
> Fix it by checking the state and if it's no running, return an error.

s/no/not/

I don't think it is just SIOCGMIIPHY. phy_has_hwtstamp(dev->phydev) is
probably also an issue. The phy is connected in open, and disconnected
in stop. So dev->phydev is not valid outside of that time.

But i'm also not sure it is guaranteed to be valid while the interface
is up. The driver uses phylink, so there could be an SFP attached to a
port, in which case, dev->phydev will not be set.

So rather than testing of running, it would be better to test if the
phydev is NULL or not.

       Andrew

> 
> Fixes: 735fec995b21 ("net: lan966x: Implement SIOCSHWTSTAMP and SIOCGHWTSTAMP")
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index ec42e526f6fb..0adf49d19142 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -399,6 +399,9 @@ static int lan966x_port_ioctl(struct net_device *dev, struct ifreq *ifr,
>  {
>  	struct lan966x_port *port = netdev_priv(dev);
>  
> +	if (!netif_running(dev))
> +		return -EINVAL;
> +
>  	if (!phy_has_hwtstamp(dev->phydev) && port->lan966x->ptp) {
>  		switch (cmd) {
>  		case SIOCSHWTSTAMP:
> -- 
> 2.30.2
> 

[PATCH net] net: lan966x: fix kernel oops on ioctl when I/F is down

[Michael Walle]

A SIOCGMIIPHY ioctl will cause a kernel oops when the interface is down.
Fix it by checking the state and if it's no running, return an error.

Fixes: 735fec995b21 ("net: lan966x: Implement SIOCSHWTSTAMP and SIOCGHWTSTAMP")
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index ec42e526f6fb..0adf49d19142 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -399,6 +399,9 @@ static int lan966x_port_ioctl(struct net_device *dev, struct ifreq *ifr,
 {
 	struct lan966x_port *port = netdev_priv(dev);
 
+	if (!netif_running(dev))
+		return -EINVAL;
+
 	if (!phy_has_hwtstamp(dev->phydev) && port->lan966x->ptp) {
 		switch (cmd) {
 		case SIOCSHWTSTAMP:
-- 
2.30.2