[PATCH] net: lan966x: avoid unregistering netdev on register failure

[PATCH] net: lan966x: avoid unregistering netdev on register failure

[Myeonghun Pak]

lan966x_probe_port() stores the newly allocated net_device in the
port before calling register_netdev(). If register_netdev() fails,
the probe error path calls lan966x_cleanup_ports(), which sees
port->dev and calls unregister_netdev() for a device that was never
registered.

Destroy the phylink instance created for this port and clear port->dev
before returning the registration error, matching the existing guard
used by the common cleanup path.

Fixes: d28d6d2e37d1 ("net: lan966x: add port module support")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 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 47752d3fde..22c496f588 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -873,6 +873,9 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
 	err = register_netdev(dev);
 	if (err) {
 		dev_err(lan966x->dev, "register_netdev failed\n");
+		phylink_destroy(phylink);
+		port->phylink = NULL;
+		port->dev = NULL;
 		return err;
 	}
 

Re: [PATCH] net: lan966x: avoid unregistering netdev on register failure

[Maxime Chevallier]

On 26/04/2026 16:27, Myeonghun Pak wrote:
> lan966x_probe_port() stores the newly allocated net_device in the
> port before calling register_netdev(). If register_netdev() fails,
> the probe error path calls lan966x_cleanup_ports(), which sees
> port->dev and calls unregister_netdev() for a device that was never
> registered.
> 
> Destroy the phylink instance created for this port and clear port->dev
> before returning the registration error, matching the existing guard
> used by the common cleanup path.
> 
> Fixes: d28d6d2e37d1 ("net: lan966x: add port module support")
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
>  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 47752d3fde..22c496f588 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -873,6 +873,9 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
>  	err = register_netdev(dev);
>  	if (err) {
>  		dev_err(lan966x->dev, "register_netdev failed\n");
> +		phylink_destroy(phylink);
> +		port->phylink = NULL;
> +		port->dev = NULL;
>  		return err;
>  	}
>  
> 

Strictly speaking, setting port->dev to NULL should be enough, but IMO
it's a bit cleaner your way as the function cleans everything after itself.

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com

Maxime

Re: [PATCH] net: lan966x: avoid unregistering netdev on register failure

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: lan966x: avoid unregistering netdev on register failure

lan966x_probe_port() stores the newly allocated net_device in the
port before calling register_netdev(). If register_netdev() fails,
the probe error path calls lan966x_cleanup_ports(), which sees
port->dev and calls unregister_netdev() for a device that was never
registered.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index 47752d3fde0b..22c496f5882a 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -873,6 +873,9 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
>  	err = register_netdev(dev);
>  	if (err) {
>  		dev_err(lan966x->dev, "register_netdev failed\n");
> +		phylink_destroy(phylink);
> +		port->phylink = NULL;
> +		port->dev = NULL;

By setting port->dev = NULL on registration failure, does this patch
inadvertently cause a null pointer dereference regression during error
handling?

If lan966x_probe_port() fails at register_netdev(), it now sets
port->dev = NULL and returns an error, leading to lan966x_cleanup_ports():

lan966x_main.c:lan966x_cleanup_ports() {
...
		if (port->dev)
			unregister_netdev(port->dev);

		lan966x_xdp_port_deinit(port);
		if (lan966x->fdma && lan966x->fdma_ndev == port->dev)
			lan966x_fdma_netdev_deinit(lan966x, port->dev);
...
}

If fdma is enabled but lan966x->fdma_ndev was never initialized (which is
true if the first port fails, as lan966x_fdma_netdev_init() is called after
lan966x_probe_port()), both lan966x->fdma_ndev and port->dev will be NULL.

This makes the condition NULL == NULL evaluate to true, unconditionally
passing port->dev (NULL) to lan966x_fdma_netdev_deinit():

lan966x_fdma.c:lan966x_fdma_netdev_deinit() {
	if (lan966x->fdma_ndev == dev) {
		netif_napi_del(&lan966x->napi);
		lan966x->fdma_ndev = NULL;
	}
}

The check lan966x->fdma_ndev == dev again passes (NULL == NULL). It will then
call netif_napi_del(&lan966x->napi).

Because the fdma napi was never initialized via netif_napi_add(), its fields
are zeroed. When netif_napi_del() is called on this uninitialized napi struct,
it will attempt to lock the device:

include/linux/netdevice.h:__netif_napi_del() {
	netdev_lock(napi->dev);
	__netif_napi_del_locked(napi);
	netdev_unlock(napi->dev);
}

Since napi->dev is NULL, netdev_lock() will dereference a null pointer.

A similar regression will occur if a subsequent port fails after the first
port was successfully cleaned up, as lan966x->fdma_ndev is reset to NULL
during the first port's cleanup.

>  		return err;
>  	}
-- 
pw-bot: changes-requested