[PATCH net v2 0/7] Double free fixes and NULL pointer checks

[PATCH net v2 0/7] Double free fixes and NULL pointer checks

[Shinas Rasheed]

Fix double frees which happen on reset scenarios, and add
NULL pointer checks for when rare cases might trigger
dereferences of such.

Changes:
V2: Added more context in commit messages and split patches for each
    commit fix

V1: https://lore.kernel.org/all/20241101103416.1064930-1-srasheed@marvell.com/

Shinas Rasheed (6):
  octeon_ep: Fix null dereferences to IQ/OQ pointers
  octeon_ep: add protective null checks in napi callbacks for cn9k cards
  octeon_ep: add protective null checks in napi callbacks for cnxk cards
  octeon_ep_vf: Fix null dereferences to IQ/OQ pointers
  octeon_ep_vf: add protective null checks in napi callbacks for cn9k
    cards
  octeon_ep_vf: add protective null checks in napi callbacks for cnxk
    cards

Vimlesh Kumar (1):
  octeon_ep: Add checks to fix double free crashes.

 .../marvell/octeon_ep/octep_cn9k_pf.c         |  9 +++-
 .../marvell/octeon_ep/octep_cnxk_pf.c         |  9 +++-
 .../ethernet/marvell/octeon_ep/octep_main.c   | 42 ++++++++++++-------
 .../net/ethernet/marvell/octeon_ep/octep_tx.c |  2 +
 .../marvell/octeon_ep_vf/octep_vf_cn9k.c      |  8 +++-
 .../marvell/octeon_ep_vf/octep_vf_cnxk.c      | 12 +++++-
 .../marvell/octeon_ep_vf/octep_vf_main.c      |  3 ++
 7 files changed, 65 insertions(+), 20 deletions(-)

-- 
2.25.1

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

[Shinas Rasheed]

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");
 }
 
@@ -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);
-- 
2.25.1

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

[Vadim Fedorenko]

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 ..]

[PATCH net v2 2/7] octeon_ep: Fix null dereferences to IQ/OQ pointers

[Shinas Rasheed]

During unload, sometimes race scenarios are seen wherein
the get stats callback proceeds to retrieve the IQ/OQ stats,
but by then the IQ/OQ might have been already freed.

Protect against such conditions by defensively checking if
the IQ/OQ pointers are null before dereference.

Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V2:
  - Split from earlier patch into a separate patch
  - Added more context

V2: https://lore.kernel.org/all/20241101103416.1064930-3-srasheed@marvell.com/

 drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index ff72b796bd25..dc783c568e2c 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -1016,6 +1016,9 @@ static void octep_get_stats64(struct net_device *netdev,
 		struct octep_iq *iq = oct->iq[q];
 		struct octep_oq *oq = oct->oq[q];
 
+		if (!iq || !oq)
+			return;
+
 		tx_packets += iq->stats.instr_completed;
 		tx_bytes += iq->stats.bytes_sent;
 		rx_packets += oq->stats.packets;
-- 
2.25.1

[PATCH net v2 3/7] octeon_ep: add protective null checks in napi callbacks for cn9k cards

[Shinas Rasheed]

During unload, at times the OQ parsed in the napi callbacks
have been observed to be null, causing system crash.
Add protective checks to avoid the same, for cn9k cards.

Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V2:
  - Split into a separate patch
  - Added more context

V1: https://lore.kernel.org/all/20241101103416.1064930-3-srasheed@marvell.com/

 drivers/net/ethernet/marvell/octeon_ep/octep_cn9k_pf.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_cn9k_pf.c b/drivers/net/ethernet/marvell/octeon_ep/octep_cn9k_pf.c
index b5805969404f..b87336b2e4b9 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_cn9k_pf.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_cn9k_pf.c
@@ -617,7 +617,14 @@ static irqreturn_t octep_rsvd_intr_handler_cn93_pf(void *dev)
 static irqreturn_t octep_ioq_intr_handler_cn93_pf(void *data)
 {
 	struct octep_ioq_vector *vector = (struct octep_ioq_vector *)data;
-	struct octep_oq *oq = vector->oq;
+	struct octep_oq *oq;
+
+	if (!vector)
+		return IRQ_HANDLED;
+	oq = vector->oq;
+
+	if (!oq || !(oq->napi))
+		return IRQ_HANDLED;
 
 	napi_schedule_irqoff(oq->napi);
 	return IRQ_HANDLED;
-- 
2.25.1

[PATCH net v2 4/7] octeon_ep: add protective null checks in napi callbacks for cnxk cards

[Shinas Rasheed]

During unload, at times the OQ parsed in the napi callbacks
have been observed to be null, causing system crash.
Add protective checks to avoid the same, for cnxk cards.

Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V2:
  - Split into a separate patch
  - Added more context

V1: https://lore.kernel.org/all/20241101103416.1064930-3-srasheed@marvell.com/

 drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c b/drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c
index 5de0b5ecbc5f..65a8dc1d492b 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c
@@ -638,7 +638,14 @@ static irqreturn_t octep_rsvd_intr_handler_cnxk_pf(void *dev)
 static irqreturn_t octep_ioq_intr_handler_cnxk_pf(void *data)
 {
 	struct octep_ioq_vector *vector = (struct octep_ioq_vector *)data;
-	struct octep_oq *oq = vector->oq;
+	struct octep_oq *oq;
+
+	if (!vector)
+		return IRQ_HANDLED;
+	oq = vector->oq;
+
+	if (!oq || !(oq->napi))
+		return IRQ_HANDLED;
 
 	napi_schedule_irqoff(oq->napi);
 	return IRQ_HANDLED;
-- 
2.25.1

[PATCH net v2 5/7] octeon_ep_vf: Fix null dereferences to IQ/OQ pointers

[Shinas Rasheed]

During unload, sometimes race scenarios are seen wherein
the get stats callback proceeds to retrieve the IQ/OQ stats,
but by then the IQ/OQ might have been already freed.

Protect against such conditions by defensively checking if
the IQ/OQ pointers are null before dereference.

Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V2:
  - Split into a separate patch
  - Added more context

V1: https://lore.kernel.org/all/20241101103416.1064930-4-srasheed@marvell.com/

 drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
index 7e6771c9cdbb..79d9ffd593eb 100644
--- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
@@ -790,6 +790,9 @@ static void octep_vf_get_stats64(struct net_device *netdev,
 		struct octep_vf_iq *iq = oct->iq[q];
 		struct octep_vf_oq *oq = oct->oq[q];
 
+		if (!iq || !oq)
+			return;
+
 		tx_packets += iq->stats.instr_completed;
 		tx_bytes += iq->stats.bytes_sent;
 		rx_packets += oq->stats.packets;
-- 
2.25.1

[PATCH net v2 6/7] octeon_ep_vf: add protective null checks in napi callbacks for cn9k cards

[Shinas Rasheed]

During unload, at times the OQ parsed in the napi callbacks
have been observed to be null, causing system crash.
Add protective checks to avoid the same, for cn9k cards.

Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V2:
  - Split into a separate patch
  - Added more context

V1: https://lore.kernel.org/all/20241101103416.1064930-4-srasheed@marvell.com/

 drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_cn9k.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_cn9k.c b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_cn9k.c
index 88937fce75f1..f1b7eda3fa42 100644
--- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_cn9k.c
+++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_cn9k.c
@@ -273,8 +273,14 @@ static irqreturn_t octep_vf_ioq_intr_handler_cn93(void *data)
 	struct octep_vf_oq *oq;
 	u64 reg_val;
 
-	oct = vector->octep_vf_dev;
+	if (!vector)
+		return IRQ_HANDLED;
+
 	oq = vector->oq;
+	if (!oq)
+		return IRQ_HANDLED;
+
+	oct = vector->octep_vf_dev;
 	/* Mailbox interrupt arrives along with interrupt of tx/rx ring pair 0 */
 	if (oq->q_no == 0) {
 		reg_val = octep_vf_read_csr64(oct, CN93_VF_SDP_R_MBOX_PF_VF_INT(0));
-- 
2.25.1

[PATCH net v2 7/7] octeon_ep_vf: add protective null checks in napi callbacks for cnxk cards

[Shinas Rasheed]

During unload, at times the OQ parsed in the napi callbacks
have been observed to be null, causing system crash.
Add protective checks to avoid the same, for cnxk cards.

Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V2:
  - Split into a separate patch
  - Added more context

V1: https://lore.kernel.org/all/20241101103416.1064930-4-srasheed@marvell.com/

 .../ethernet/marvell/octeon_ep_vf/octep_vf_cnxk.c    | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_cnxk.c b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_cnxk.c
index 1f79dfad42c6..31c0d7c0492a 100644
--- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_cnxk.c
+++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_cnxk.c
@@ -284,8 +284,14 @@ static irqreturn_t octep_vf_ioq_intr_handler_cnxk(void *data)
 	struct octep_vf_oq *oq;
 	u64 reg_val;
 
-	oct = vector->octep_vf_dev;
+	if (!vector)
+		return IRQ_HANDLED;
+
 	oq = vector->oq;
+	if (!oq)
+		return IRQ_HANDLED;
+
+	oct = vector->octep_vf_dev;
 	/* Mailbox interrupt arrives along with interrupt of tx/rx ring pair 0 */
 	if (oq->q_no == 0) {
 		reg_val = octep_vf_read_csr64(oct, CNXK_VF_SDP_R_MBOX_PF_VF_INT(0));
@@ -294,6 +300,10 @@ static irqreturn_t octep_vf_ioq_intr_handler_cnxk(void *data)
 			octep_vf_write_csr64(oct, CNXK_VF_SDP_R_MBOX_PF_VF_INT(0), reg_val);
 		}
 	}
+
+	if (!(oq->napi))
+		return IRQ_HANDLED;
+
 	napi_schedule_irqoff(oq->napi);
 	return IRQ_HANDLED;
 }
-- 
2.25.1