Re: [net,v2,2/2] net: liquidio: Initialize netdev pointer before queue setup

[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.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: liquidio: Initialize netdev pointer before queue setup

This patch moves the initialization of lio and props structures earlier
in setup_nic_devices() so that props->netdev is set before calling
netif_set_real_num_rx_queues() and netif_set_real_num_tx_queues(). The
intent is to fix a memory leak where the allocated netdev would not be
freed if queue setup fails.

> diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c b/drivers/net/ethernet/cavium/liquidio/lio_main.c
> index 3ba2806f5d1e..2383f0173a54 100644
> --- a/drivers/net/ethernet/cavium/liquidio/lio_main.c
> +++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c
> @@ -3505,6 +3505,16 @@ static int setup_nic_devices(struct octeon_device *octeon_dev)
>  		 */
>  		netdev->netdev_ops = &lionetdevops;
>
> +		lio = GET_LIO(netdev);
> +
> +		memset(lio, 0, sizeof(struct lio));
> +
> +		lio->ifidx = ifidx_or_pfnum;
> +
> +		props = &octeon_dev->props[i];
> +		props->gmxport = resp->cfg_info.linfo.gmxport;
> +		props->netdev = netdev;

Moving this initialization earlier fixes the memory leak, but now if
netif_set_real_num_rx_queues() or netif_set_real_num_tx_queues() fails
below, the cleanup path in liquidio_destroy_nic_device() will proceed
further than before since props->netdev is no longer NULL.

Previously, when queue setup failed, liquidio_destroy_nic_device() would
return early at the "if (!netdev)" check. Now it continues and calls
cleanup_rx_oom_poll_fn().

In cleanup_rx_oom_poll_fn() (lio_core.c), the code does:

    struct lio *lio = GET_LIO(netdev);
    struct octeon_device *oct = lio->oct_dev;
    ...
    for (q_no = 0; q_no < oct->num_oqs; q_no++) {

Since lio->oct_dev is not assigned until much later in setup_nic_devices()
(around line 3602), it will be NULL after the memset above. Would this
cause a NULL pointer dereference when cleanup_rx_oom_poll_fn() tries to
access oct->num_oqs?

> +
>  		retval = netif_set_real_num_rx_queues(netdev, num_oqueues);
>  		if (retval) {
>  			dev_err(&octeon_dev->pci_dev->dev,

[ ... ]
-- 
pw-bot: cr