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

From: Vadim Fedorenko
Date: Wed Nov 13 2024 - 10:20:57 EST


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);