| Message ID | 20241108074543.1123036-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 08/11/2024 07:45, 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 > > Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support") > Signed-off-by: Vimlesh Kumar <vimleshk@marvell.com> > Signed-off-by: Shinas Rasheed <srasheed@marvell.com> As you reposted in <24h, could you please take a look at the comments in v2? FYI, 24h rule in netdev: https://docs.kernel.org/process/maintainer-netdev.html Thanks.
Hi Vadim, On 08/11/2024 07:45, Shinas Rasheed wrote: >> From: Vimlesh Kumar <mailto:vimleshk@marvell.com> >> >> Add required checks to avoid double free. Crashes were >> observed due to the same on reset scenarios >> >> Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support") >> Signed-off-by: Vimlesh Kumar <mailto:vimleshk@marvell.com> >> Signed-off-by: Shinas Rasheed <mailto:srasheed@marvell.com> > >As you reposted in <24h, could you please take a look at the comments in v2? > >FYI, 24h rule in netdev: >https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.kernel.org_process_maintainer-2Dnetdev.html&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=1OxLD4y-oxrlgQ1rjXgWtmLz1pnaDjD96sDq->cKUwK4&m=VcY_pBmYwqgydJILlz4916iUWNPjepPuBfe3p4cw71yyziV7gvWgoDV9pEY1gq1g&s=tAroBPYvyDp0HN_HVt1jDiWKnBrMeyy0bzTq8AEgA64&e= > >Thanks. Extremely sorry for that, I had noticed the fixes comment was missing and mistakenly thought that it had been a day. Sure, I'll take a look at the v2 comments Thanks for your time!
Hi Vadim, Replying to the V2 patch comments: 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://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20241101103416.1064930-2D2-2Dsrasheed-40marvell.com_&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=1OxLD4y-oxrlgQ1rjXgWtmLz1pnaDjD96sDq-cKUwK4&m=qcKdIM1zsbBD4TFAsa19yJzKNkTlFLrDfM4ffSlTitSWElqZqgdFqodALcL83iWB&s=M_kpJB7LL7JqIBk-MBXonCyf_BWg3fcwHuMc8FQpun0&e= >> >> .../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? > I think you're right. This won't perhaps be the actual part of the code "fixing" the crash, and might just as a protection. I shall remove this. >> } >> >> @@ -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. > Do you mean this: if (!oct->msix_entries) return; I'll make that change as well. Thanks! Shinas
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);