| Message ID | 20241107132846.1118835-2-srasheed@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Double free fixes and NULL pointer checks | expand |
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 ..]
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"); } @@ -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"); } @@ -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; + } } } @@ -666,7 +672,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); } } 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);