Re: [PATCH net-next v6 07/14] net: lan966x: add FDMA ops dispatch for PCIe support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Network device is registered and exposed before FDMA initialization completes, leading to NULL pointer dereference.
- [High] FDMA data structures are freed before network devices are unregistered, causing Use-After-Free on concurrent transmission.
- [High] Missing NULL check for partially populated `lan966x->ports` array when processing received frames.
--

commit ac4a299eb602b1195f379c3cd6e7672750ac4f72
Author: Daniel Machon <daniel.machon@microchip.com>

net: lan966x: add FDMA ops dispatch for PCIe support

Introduce lan966x_fdma_ops to support different FDMA implementations
for platform and PCIe. Plumb fdma_init, fdma_deinit, fdma_xmit,
fdma_poll and fdma_resize through the ops table, and select the
implementation at probe time. Only the platform implementation exists
at this point; the PCIe implementation is added in a later patch.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> index 15fc59cb4ad79..2695bc41e52a9 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> @@ -947,7 +947,7 @@ void lan966x_fdma_netdev_init(struct lan966x *lan966x, struct net_device *dev)
>  		return;
>  
>  	lan966x->fdma_ndev = dev;
> -	netif_napi_add(dev, &lan966x->napi, lan966x_fdma_napi_poll);
> +	netif_napi_add(dev, &lan966x->napi, lan966x->ops->fdma_poll);
>  	napi_enable(&lan966x->napi);
>  }

[Severity: High]
This is a pre-existing issue, but looking at the RX path in
lan966x_fdma_rx_check_frame(), is there a missing NULL check for the port
array?

The lan966x->ports array is sparsely populated in lan966x_probe() depending on
the provided device tree nodes. When checking the frame, the src_port is
extracted from the Inter-Frame Header (IFH):

lan966x_fdma.c:lan966x_fdma_rx_check_frame() {
    ...
    if (WARN_ON(*src_port >= lan966x->num_phys_ports))
        return FDMA_ERROR;

    port = lan966x->ports[*src_port];
    if (!lan966x_xdp_port_present(port))
    ...
}

While src_port is checked against lan966x->num_phys_ports, there is no check to
ensure lan966x->ports[*src_port] is non-NULL before passing it to
lan966x_xdp_port_present(port).

If the switch hardware generates or loops back a frame with an IFH source port
corresponding to an unconfigured physical port, could this cause a NULL pointer
dereference?

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index 2741f7c9fa4c0..6e6c08bb8eeab 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c

[ ... ]

> @@ -1250,7 +1260,7 @@ static int lan966x_probe(struct platform_device *pdev)
>  	if (err)
>  		goto cleanup_fdb;
>  
> -	err = lan966x_fdma_init(lan966x);
> +	err = lan966x->ops->fdma_init(lan966x);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential race
condition here during probe.

The network device is registered and exposed to userspace earlier in
lan966x_probe() via lan966x_probe_port() -> register_netdev().
NAPI is also enabled. However, fdma_init() is called only here, which means
there is a window where the netdev is up before the FDMA ring structures
like tx->dcbs_buf are allocated.

If a user-space application configures the interface up and initiates a
transmission concurrently before fdma_init() finishes allocating the DCB
buffers, could lan966x_fdma_get_next_dcb() access a NULL pointer?

>  	if (err)
>  		goto cleanup_ptp;
>  

[ ... ]

> @@ -1291,7 +1301,7 @@ static void lan966x_remove(struct platform_device *pdev)
>  
>  	lan966x_taprio_deinit(lan966x);
>  	lan966x_vcap_deinit(lan966x);
> -	lan966x_fdma_deinit(lan966x);
> +	lan966x->ops->fdma_deinit(lan966x);

[Severity: High]
This is a pre-existing issue, but I see a potential use-after-free in the
unload path.

In lan966x_remove(), ops->fdma_deinit() is called before
lan966x_cleanup_ports() unregisters the network devices. For the platform
implementation, lan966x_fdma_deinit() frees tx->dcbs_buf without stopping the
netdev transmission queues using netif_tx_disable().

If a packet is transmitted concurrently from userspace via ndo_start_xmit
while the module is unloading, could lan966x_fdma_xmit() dereference the
already freed tx->dcbs_buf buffer?

>  	lan966x_cleanup_ports(lan966x);
>  
>  	cancel_delayed_work_sync(&lan966x->stats_work);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-lan966x-pci-fdma-v6-0-6f48dab9d671@microchip.com?part=7