Messages in this thread Patch in this message |  | | From | Simon Horman <> | | Subject | Re: [PATCH] net: lan966x: avoid unregistering netdev on register failure | | Date | Tue, 28 Apr 2026 14:45:16 +0100 |
| |
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
|  |