Re: [PATCH net v4 1/7] octeon_ep: Add checks to fix double free crashes

[Vadim Fedorenko]

On 13/11/2024 11:13, Shinas Rasheed wrote:
> From: Vimlesh Kumar <vimleshk@xxxxxxxxxxx>
>
> Add required checks to avoid double free. Crashes were
> observed due to the same on reset scenarios, when reset
> was tried multiple times, and the first reset had torn
> down resources already.
>
> Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support")
> Signed-off-by: Vimlesh Kumar <vimleshk@xxxxxxxxxxx>
> Signed-off-by: Shinas Rasheed <srasheed@xxxxxxxxxxx>
> ---
> V4:
>    - Removed unnecessary protective code. Added fast return from
>      octep_free_irqs()
>    - Improved commit message
>
> V3: https://lore.kernel.org/all/20241108074543.1123036-2-srasheed@xxxxxxxxxxx/
>    - Added back "Fixes" to the changelist
>
> V2: https://lore.kernel.org/all/20241107132846.1118835-2-srasheed@xxxxxxxxxxx/
>    - No changes
>
> V1: https://lore.kernel.org/all/20241101103416.1064930-2-srasheed@xxxxxxxxxxx/
>
>   .../net/ethernet/marvell/octeon_ep/octep_main.c    | 14 ++++++++++----
>   drivers/net/ethernet/marvell/octeon_ep/octep_tx.c  |  2 ++
>   2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> index 549436efc204..29796544feb6 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> @@ -496,6 +496,9 @@ static void octep_free_irqs(struct octep_device *oct)
>   {
>   	int i;
>   
> +	if (!oct->msix_entries)
> +		return;
> +
>   	/* First few MSI-X interrupts are non queue interrupts; free them */
>   	for (i = 0; i < CFG_GET_NON_IOQ_MSIX(oct->conf); i++)
>   		free_irq(oct->msix_entries[i].vector, oct);
> @@ -505,7 +508,7 @@ static void octep_free_irqs(struct octep_device *oct)
>   	for (i = CFG_GET_NON_IOQ_MSIX(oct->conf); i < oct->num_irqs; i++) {
>   		irq_set_affinity_hint(oct->msix_entries[i].vector, NULL);
>   		free_irq(oct->msix_entries[i].vector,
> -			 oct->ioq_vector[i - CFG_GET_NON_IOQ_MSIX(oct->conf)]);
> +				oct->ioq_vector[i - CFG_GET_NON_IOQ_MSIX(oct->conf)]);

Looks like this chunk was unintended, it was perfectly aligned before...

>   	}
>   	netdev_info(oct->netdev, "IRQs freed\n");
>   }
> @@ -635,8 +638,10 @@ static void octep_napi_delete(struct octep_device *oct)
>   
>   	for (i = 0; i < oct->num_oqs; i++) {
>   		netdev_dbg(oct->netdev, "Deleting NAPI on Q-%d\n", i);
> -		netif_napi_del(&oct->ioq_vector[i]->napi);
> -		oct->oq[i]->napi = NULL;
> +		if (oct->oq[i]->napi) {
> +			netif_napi_del(&oct->ioq_vector[i]->napi);
> +			oct->oq[i]->napi = NULL;
> +		}

I would just add

if (!oct->oq[i]->napi)
	continue

as the first line within the loop. Otherwise debug message could be
misleading - deleting NAPI on queue which doesn't exist.

>   	}
>   }
>   
> @@ -666,7 +671,8 @@ static void octep_napi_disable(struct octep_device *oct)
>   
>   	for (i = 0; i < oct->num_oqs; i++) {
>   		netdev_dbg(oct->netdev, "Disabling NAPI on Q-%d\n", i);
> -		napi_disable(&oct->ioq_vector[i]->napi);
> +		if (oct->oq[i]->napi)
> +			napi_disable(&oct->ioq_vector[i]->napi);

And here again, the same pattern.

>   	}
>   }
>   
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
> index 06851b78aa28..157bf489ae19 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
> @@ -323,6 +323,8 @@ void octep_free_iqs(struct octep_device *oct)
>   	int i;
>   
>   	for (i = 0; i < CFG_GET_PORTS_ACTIVE_IO_RINGS(oct->conf); i++) {
> +		if (!oct->iq[i])
> +			continue;
>   		octep_free_iq(oct->iq[i]);
>   		dev_dbg(&oct->pdev->dev,
>   			"Successfully destroyed IQ(TxQ)-%d.\n", i);