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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 07/11/2024 13:28, Shinas Rasheed wrote:
> From: Vimlesh Kumar <vimleshk@marvell.com>
> 
> Add required checks to avoid double free. Crashes were
> observed due to the same on reset scenarios
> 
> Signed-off-by: Vimlesh Kumar <vimleshk@marvell.com>
> Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> ---
> V2:
>    - No changes
> 
> V1: https://lore.kernel.org/all/20241101103416.1064930-2-srasheed@marvell.com/
> 
>   .../ethernet/marvell/octeon_ep/octep_main.c   | 39 +++++++++++--------
>   .../net/ethernet/marvell/octeon_ep/octep_tx.c |  2 +
>   2 files changed, 25 insertions(+), 16 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..ff72b796bd25 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> @@ -154,9 +154,11 @@ static int octep_enable_msix_range(struct octep_device *oct)
>    */
>   static void octep_disable_msix(struct octep_device *oct)
>   {
> -	pci_disable_msix(oct->pdev);
> -	kfree(oct->msix_entries);
> -	oct->msix_entries = NULL;
> +	if (oct->msix_entries) {
> +		pci_disable_msix(oct->pdev);
> +		kfree(oct->msix_entries);
> +		oct->msix_entries = NULL;
> +	}
>   	dev_info(&oct->pdev->dev, "Disabled MSI-X\n");

How can this function crash? pci_disable_msix() will have checks for
already disabled msix, kfree can properly deal with NULL pointer.
Do you have stack trace of the crash here?

>   }
>   
> @@ -496,16 +498,18 @@ static void octep_free_irqs(struct octep_device *oct)
>   {
>   	int i;
>   
> -	/* 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);
> -	kfree(oct->non_ioq_irq_names);
> -
> -	/* Free IRQs for Input/Output (Tx/Rx) queues */
> -	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)]);
> +	if (oct->msix_entries) {
> +		/* 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);
> +		kfree(oct->non_ioq_irq_names);
> +
> +		/* Free IRQs for Input/Output (Tx/Rx) queues */
> +		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)]);
> +		}
>   	}
>   	netdev_info(oct->netdev, "IRQs freed\n");
>   }

Have you considered fast return option? like

if (!octep_disable_msix)
	return;

It will make less intendation and less changes in LoC but will presume
the same behavior.

> @@ -635,8 +639,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;
> +		}
>   	}
>   }
>   

[ .. snip ..]