Skip to contentSkip to conversation

BNX2: fix a Null Pointer for stats_blk

PATCH Superseded Weidong Wang 2 participants 11 messages Sep 22, 2015 – Oct 11, 2015

Subsystems (3)
Networking Network drivers Broadcom BNX2 gigabit Ethernet driver
lore
Share & tools
lore

Weidong Wang

author
#0 lore raw

PATCH 1/1 BNX2: fix a Null Pointer for stats_blk

we have two processes to do:
P1#: ifconfig eth0 down; which will call bnx2_close, then will , and set Null to stats_blk
P2#: ifconfig eth0; which will call bnx2_get_stats64, it will use stats_blk.
In one case:

    --P1#--                   --P2#--
                              stats_blk(no null)
    bnx2_free_mem
    ->bp->stats_blk = NULL
                              GET_64BIT_NET_STATS

then it will cause 'NULL Pointer' Problem.
it is as well with 'ethtool -S ethx'.

BTW, the other branch has this problem as well.

So we add a spin_lock to protect stats_blk.

Signed-off-by Wang Weidong <[email protected]>
---
 drivers/net/ethernet/broadcom/bnx2.c | 42 ++++++++++++++++++++++++++++--------
 drivers/net/ethernet/broadcom/bnx2.h |  1 +
 2 files changed, 34 insertions(+), 9 deletions(-)
drivers/net/ethernet/broadcom/bnx2.c +33 -9
index 2b66ef3..aec4081 100644
@@ -830,11 +830,13 @@ bnx2_free_mem(struct bnx2 *bp)
 		}
 	}
 	if (bnapi->status_blk.msi) {
+		spin_lock(&bp->stats64_lock);
 		dma_free_coherent(&bp->pdev->dev, bp->status_stats_size,
 				  bnapi->status_blk.msi,
 				  bp->status_blk_mapping);
 		bnapi->status_blk.msi = NULL;
 		bp->stats_blk = NULL;
+		spin_unlock(&bp->stats64_lock);
 	}
 }
 

@@ -880,6 +882,7 @@ bnx2_alloc_mem(struct bnx2 *bp)
 		}
 	}
 
+	spin_lock(&bp->stats64_lock);
 	bp->stats_blk = status_blk + status_blk_size;
 
 	bp->stats_blk_mapping = bp->status_blk_mapping + status_blk_size;

@@ -894,20 +897,23 @@ bnx2_alloc_mem(struct bnx2 *bp)
 						&bp->ctx_blk_mapping[i],
 						GFP_KERNEL);
 			if (bp->ctx_blk[i] == NULL)
-				goto alloc_mem_err;
+				goto free_stats64_lock;
 		}
 	}
 
 	err = bnx2_alloc_rx_mem(bp);
 	if (err)
-		goto alloc_mem_err;
+		goto free_stats64_lock;
 
 	err = bnx2_alloc_tx_mem(bp);
 	if (err)
-		goto alloc_mem_err;
+		goto free_stats64_lock;
 
+	spin_unlock(&bp->stats64_lock);
 	return 0;
 
+free_stats64_lock:
+	spin_unlock(&bp->stats64_lock);
 alloc_mem_err:
 	bnx2_free_mem(bp);
 	return -ENOMEM;

@@ -6756,10 +6762,14 @@ bnx2_close(struct net_device *dev)
 static void
 bnx2_save_stats(struct bnx2 *bp)
 {
-	u32 *hw_stats = (u32 *) bp->stats_blk;
-	u32 *temp_stats = (u32 *) bp->temp_stats_blk;
+	u32 *hw_stats;
+	u32 *temp_stats;
 	int i;
 
+	spin_lock(&bp->stats64_lock);
+	hw_stats = (u32 *) bp->stats_blk;
+	temp_stats = (u32 *) bp->temp_stats_blk;
+
 	/* The 1st 10 counters are 64-bit counters */
 	for (i = 0; i < 20; i += 2) {
 		u32 hi;

@@ -6775,6 +6785,8 @@ bnx2_save_stats(struct bnx2 *bp)
 
 	for ( ; i < sizeof(struct statistics_block) / 4; i++)
 		temp_stats[i] += hw_stats[i];
+
+	spin_unlock(&bp->stats64_lock);
 }
 
 #define GET_64BIT_NET_STATS64(ctr)		\

@@ -6793,8 +6805,11 @@ bnx2_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *net_stats)
 {
 	struct bnx2 *bp = netdev_priv(dev);
 
-	if (bp->stats_blk == NULL)
+	spin_lock(&bp->stats64_lock);
+	if (bp->stats_blk == NULL) {
+		spin_unlock(&bp->stats64_lock);
 		return net_stats;
+	}
 
 	net_stats->rx_packets =
 		GET_64BIT_NET_STATS(stat_IfHCInUcastPkts) +

@@ -6858,6 +6873,7 @@ bnx2_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *net_stats)
 		GET_32BIT_NET_STATS(stat_IfInMBUFDiscards) +
 		GET_32BIT_NET_STATS(stat_FwRxDrop);
 
+	spin_unlock(&bp->stats64_lock);
 	return net_stats;
 }
 

@@ -7634,13 +7650,17 @@ bnx2_get_ethtool_stats(struct net_device *dev,
 {
 	struct bnx2 *bp = netdev_priv(dev);
 	int i;
-	u32 *hw_stats = (u32 *) bp->stats_blk;
-	u32 *temp_stats = (u32 *) bp->temp_stats_blk;
+	u32 *hw_stats;
+	u32 *temp_stats;
 	u8 *stats_len_arr = NULL;
 
+	spin_lock(&bp->stats64_lock);
+	hw_stats = (u32 *) bp->stats_blk;
+	temp_stats = (u32 *) bp->temp_stats_blk;
+
 	if (hw_stats == NULL) {
 		memset(buf, 0, sizeof(u64) * BNX2_NUM_STATS);
-		return;
+		goto free_stats64_lock;
 	}
 
 	if ((BNX2_CHIP_ID(bp) == BNX2_CHIP_ID_5706_A0) ||

@@ -7673,6 +7693,9 @@ bnx2_get_ethtool_stats(struct net_device *dev,
 			 (((u64) *(temp_stats + offset)) << 32) +
 			 *(temp_stats + offset + 1);
 	}
+
+free_stats64_lock:
+	spin_unlock(&bp->stats64_lock);
 }
 
 static int

@@ -8125,6 +8148,7 @@ bnx2_init_board(struct pci_dev *pdev, struct net_device *dev)
 
 	spin_lock_init(&bp->phy_lock);
 	spin_lock_init(&bp->indirect_lock);
+	spin_lock_init(&bp->stats64_lock);
 #ifdef BCM_CNIC
 	mutex_init(&bp->cnic_lock);
 #endif
drivers/net/ethernet/broadcom/bnx2.h +1 -0
index f92f76c..c88c21b 100644
@@ -6928,6 +6928,7 @@ struct bnx2 {
 
 	dma_addr_t		status_blk_mapping;
 
+	spinlock_t stats64_lock;
 	struct statistics_block	*stats_blk;
 	struct statistics_block	*temp_stats_blk;
 	dma_addr_t		stats_blk_mapping;
Signature · 1 line
1.7.12
maintainer
#1 lore raw

From: Weidong Wang <[email protected]>
Date: Tue, 22 Sep 2015 20:42:40 +0800

Quoted 11 lines
drivers/net/ethernet/broadcom/bnx2.c +1 -0
@@ -880,6 +882,7 @@ bnx2_alloc_mem(struct bnx2 *bp)
 		}
 	}
 
+	spin_lock(&bp->stats64_lock);
 	bp->stats_blk = status_blk + status_blk_size;
 
 	bp->stats_blk_mapping = bp->status_blk_mapping + status_blk_size;

@@ -894,20 +897,23 @@ bnx2_alloc_mem(struct bnx2 *bp)
 						&bp->ctx_blk_mapping[i],
 						GFP_KERNEL);

drivers/net/ethernet/broadcom/bnx2.c bnx2_alloc_mem(struct bnx2 *bp) · line 906

Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2.c +2 -2
 			if (bp->ctx_blk[i] == NULL)
-				goto alloc_mem_err;
+				goto free_stats64_lock;
 		}
 	}
 
 	err = bnx2_alloc_rx_mem(bp);
 	if (err)
-		goto alloc_mem_err;
+		goto free_stats64_lock;

You're holding a spinlock while doing GFP_KERNEL allocations.

Second of all, taking a spinlock in get_stats64() defeats the whole intention of making statistics acquisition as fast and as SMP scalable as possible.

author
#2 lore raw
Quoted 20 lines · David Miller

From: Weidong Wang <[email protected]>
Date: Tue, 22 Sep 2015 20:42:40 +0800

Quoted 17 lines
drivers/net/ethernet/broadcom/bnx2.c +2 -1
@@ -880,6 +882,7 @@ bnx2_alloc_mem(struct bnx2 *bp)
 		}
 	}
 
+	spin_lock(&bp->stats64_lock);
 	bp->stats_blk = status_blk + status_blk_size;
 
 	bp->stats_blk_mapping = bp->status_blk_mapping + status_blk_size;

@@ -894,20 +897,23 @@ bnx2_alloc_mem(struct bnx2 *bp)
 						&bp->ctx_blk_mapping[i],
 						GFP_KERNEL);
 			if (bp->ctx_blk[i] == NULL)
-				goto alloc_mem_err;
+				goto free_stats64_lock;
 		}
 	}
Quoted 7 lines

drivers/net/ethernet/broadcom/bnx2.c bnx2_alloc_mem(struct bnx2 *bp) · line 906

Quoted 4 lines
drivers/net/ethernet/broadcom/bnx2.c +1 -1
 	err = bnx2_alloc_rx_mem(bp);
 	if (err)
-		goto alloc_mem_err;
+		goto free_stats64_lock;

You're holding a spinlock while doing GFP_KERNEL allocations.

hm, yep, I should move it after the allocations. Like this:

@@ -880,7 +882,9 @@ bnx2_alloc_mem(struct bnx2 *bp)

                }
        }
+	spin_lock(&bp->stats64_lock);
	bp->stats_blk = status_blk + status_blk_size;

+ spin_unlock(&bp->stats64_lock);

the allocations won't use the stats_blk, so I shouldn't hold the lock while doing allocations.

Quoted 4 lines

Second of all, taking a spinlock in get_stats64() defeats the whole intention of making statistics acquisition as fast and as SMP scalable as possible.

It does affect the intention. Although, the problem exists then makes the system panic within some case.

Do you have any idea about it?

Best Regards,
Weidong

Quoted 2 lines

.

maintainer
#3 lore raw

From: Weidong Wang <[email protected]>
Date: Thu, 24 Sep 2015 10:00:45 +0800

Quoted 4 lines

It does affect the intention. Although, the problem exists then makes the system panic within some case.

Do you have any idea about it?

Allocate the statistics block at probe time so that this problem is impossible.

author
#4 lore raw
Quoted 11 lines · David Miller

From: Weidong Wang <[email protected]>
Date: Thu, 24 Sep 2015 10:00:45 +0800

Quoted 4 lines

It does affect the intention. Although, the problem exists then makes the system panic within some case.

Do you have any idea about it?

Allocate the statistics block at probe time so that this problem is impossible.

It is a good idea.

Yet, what is the intention of the dynamic to alloc/free stats_block? what will be affected by allocating the statistics block.

Best Regards,
Weidong

Quoted 1 line
author
#5 lore raw

PATCH v2 1/1 BNX2: fix a Null Pointer for stats_blk

In reply to Weidong Wang

we have two processes to do:
P1#: ifconfig eth0 down; which will call bnx2_close, then will , and set Null to stats_blk
P2#: ifconfig eth0; which will call bnx2_get_stats64, it will use stats_blk.
In one case:

    --P1#--                   --P2#--
                              stats_blk(no null)
    bnx2_free_mem
    ->bp->stats_blk = NULL
                              GET_64BIT_NET_STATS

then it will cause 'NULL Pointer' Problem.
it is as well with 'ethtool -S ethx'.

Allocate the statistics block at probe time so that this problem is impossible

Signed-off-by Tianhong Ding <[email protected]>

---
Change in v2:
- Use Allocate the statistics block instead of spinlock, which suggested by David Miller.
- Updating commit message according to changes.

---
 drivers/net/ethernet/broadcom/bnx2.c | 61 +++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 21 deletions(-)
drivers/net/ethernet/broadcom/bnx2.c +40 -21
index 2b66ef3..1f33982 100644
@@ -813,22 +813,11 @@ bnx2_alloc_rx_mem(struct bnx2 *bp)
 }
 
 static void
-bnx2_free_mem(struct bnx2 *bp)
+bnx2_free_stats_blk(struct net_device *dev)
 {
-	int i;
+	struct bnx2 *bp = netdev_priv(dev);
 	struct bnx2_napi *bnapi = &bp->bnx2_napi[0];
 
-	bnx2_free_tx_mem(bp);
-	bnx2_free_rx_mem(bp);
-
-	for (i = 0; i < bp->ctx_pages; i++) {
-		if (bp->ctx_blk[i]) {
-			dma_free_coherent(&bp->pdev->dev, BNX2_PAGE_SIZE,
-					  bp->ctx_blk[i],
-					  bp->ctx_blk_mapping[i]);
-			bp->ctx_blk[i] = NULL;
-		}
-	}
 	if (bnapi->status_blk.msi) {
 		dma_free_coherent(&bp->pdev->dev, bp->status_stats_size,
 				  bnapi->status_blk.msi,

@@ -839,11 +828,12 @@ bnx2_free_mem(struct bnx2 *bp)
 }
 
 static int
-bnx2_alloc_mem(struct bnx2 *bp)
+bnx2_alloc_stats_blk(struct net_device *dev)
 {
-	int i, status_blk_size, err;
+	int i, status_blk_size;
 	struct bnx2_napi *bnapi;
 	void *status_blk;
+	struct bnx2 *bp = netdev_priv(dev);
 
 	/* Combine status and statistics blocks into one allocation. */
 	status_blk_size = L1_CACHE_ALIGN(sizeof(struct status_block));

@@ -852,11 +842,10 @@ bnx2_alloc_mem(struct bnx2 *bp)
 						 BNX2_SBLK_MSIX_ALIGN_SIZE);
 	bp->status_stats_size = status_blk_size +
 				sizeof(struct statistics_block);
-
 	status_blk = dma_zalloc_coherent(&bp->pdev->dev, bp->status_stats_size,
 					 &bp->status_blk_mapping, GFP_KERNEL);
 	if (status_blk == NULL)
-		goto alloc_mem_err;
+		return -ENOMEM;
 
 	bnapi = &bp->bnx2_napi[0];
 	bnapi->status_blk.msi = status_blk;

@@ -865,11 +854,10 @@ bnx2_alloc_mem(struct bnx2 *bp)
 	bnapi->hw_rx_cons_ptr =
 		&bnapi->status_blk.msi->status_rx_quick_consumer_index0;
 	if (bp->flags & BNX2_FLAG_MSIX_CAP) {
-		for (i = 1; i < bp->irq_nvecs; i++) {
+		for (i = 1; i < BNX2_MAX_MSIX_HW_VEC; i++) {
 			struct status_block_msix *sblk;
 
 			bnapi = &bp->bnx2_napi[i];
-
 			sblk = (status_blk + BNX2_SBLK_MSIX_ALIGN_SIZE * i);
 			bnapi->status_blk.msix = sblk;
 			bnapi->hw_tx_cons_ptr =

@@ -879,11 +867,35 @@ bnx2_alloc_mem(struct bnx2 *bp)
 			bnapi->int_num = i << 24;
 		}
 	}
-
 	bp->stats_blk = status_blk + status_blk_size;
-
 	bp->stats_blk_mapping = bp->status_blk_mapping + status_blk_size;
 
+	return 0;
+}
+
+static void
+bnx2_free_mem(struct bnx2 *bp)
+{
+	int i;
+
+	bnx2_free_tx_mem(bp);
+	bnx2_free_rx_mem(bp);
+
+	for (i = 0; i < bp->ctx_pages; i++) {
+		if (bp->ctx_blk[i]) {
+			dma_free_coherent(&bp->pdev->dev, BNX2_PAGE_SIZE,
+					  bp->ctx_blk[i],
+					  bp->ctx_blk_mapping[i]);
+			bp->ctx_blk[i] = NULL;
+		}
+	}
+}
+
+static int
+bnx2_alloc_mem(struct bnx2 *bp)
+{
+	int i, err;
+
 	if (BNX2_CHIP(bp) == BNX2_CHIP_5709) {
 		bp->ctx_pages = 0x2000 / BNX2_PAGE_SIZE;
 		if (bp->ctx_pages == 0)

@@ -8330,6 +8342,11 @@ bnx2_init_board(struct pci_dev *pdev, struct net_device *dev)
 
 	bp->phy_addr = 1;
 
+	/* allocate stats_blk */
+	rc = bnx2_alloc_stats_blk(dev);
+	if (rc)
+		goto err_out_unmap;
+
 	/* Disable WOL support if we are running on a SERDES chip. */
 	if (BNX2_CHIP(bp) == BNX2_CHIP_5709)
 		bnx2_get_5709_media(bp);

@@ -8586,6 +8603,7 @@ error:
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
 err_free:
+	bnx2_free_stats_blk(dev);
 	free_netdev(dev);
 	return rc;
 }

@@ -8603,6 +8621,7 @@ bnx2_remove_one(struct pci_dev *pdev)
 
 	pci_iounmap(bp->pdev, bp->regview);
 
+	bnx2_free_stats_blk(dev);
 	kfree(bp->temp_stats_blk);
 
 	if (bp->flags & BNX2_FLAG_AER_ENABLED) {
Signature · 1 line
1.9.0
author
#6 lore raw
Quoted 12 lines · Weidong Wang

we have two processes to do:
P1#: ifconfig eth0 down; which will call bnx2_close, then will , and set Null to stats_blk
P2#: ifconfig eth0; which will call bnx2_get_stats64, it will use stats_blk.
In one case:

    --P1#--                   --P2#--
                              stats_blk(no null)
    bnx2_free_mem
    ->bp->stats_blk = NULL
                              GET_64BIT_NET_STATS
Quoted 8 lines

then it will cause 'NULL Pointer' Problem.
it is as well with 'ethtool -S ethx'.

Allocate the statistics block at probe time so that this problem is impossible

Signed-off-by Tianhong Ding <[email protected]>

---

Sorry for that, The sob is Error. I will fixed it.

Just Ignore it.

Regards.
Weidong

author
#7 lore raw

PATCH RESEND v2 1/1 BNX2: fix a Null Pointer for stats_blk

In reply to Weidong Wang

we have two processes to do:
P1#: ifconfig eth0 down; which will call bnx2_close, then will , and set Null to stats_blk
P2#: ifconfig eth0; which will call bnx2_get_stats64, it will use stats_blk.
In one case:

    --P1#--                   --P2#--
                              stats_blk(no null)
    bnx2_free_mem
    ->bp->stats_blk = NULL
                              GET_64BIT_NET_STATS

then it will cause 'NULL Pointer' Problem.
it is as well with 'ethtool -S ethx'.

Allocate the statistics block at probe time so that this problem is impossible

Signed-off-by Weidong Wang <[email protected]>

---
Change in v2:
- Use Allocate the statistics block instead of spinlock, which suggested by David Miller.
- Updating commit message according to changes.

---
 drivers/net/ethernet/broadcom/bnx2.c | 61 +++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 21 deletions(-)
drivers/net/ethernet/broadcom/bnx2.c +40 -21
index 2b66ef3..1f33982 100644
@@ -813,22 +813,11 @@ bnx2_alloc_rx_mem(struct bnx2 *bp)
 }
 
 static void
-bnx2_free_mem(struct bnx2 *bp)
+bnx2_free_stats_blk(struct net_device *dev)
 {
-	int i;
+	struct bnx2 *bp = netdev_priv(dev);
 	struct bnx2_napi *bnapi = &bp->bnx2_napi[0];
 
-	bnx2_free_tx_mem(bp);
-	bnx2_free_rx_mem(bp);
-
-	for (i = 0; i < bp->ctx_pages; i++) {
-		if (bp->ctx_blk[i]) {
-			dma_free_coherent(&bp->pdev->dev, BNX2_PAGE_SIZE,
-					  bp->ctx_blk[i],
-					  bp->ctx_blk_mapping[i]);
-			bp->ctx_blk[i] = NULL;
-		}
-	}
 	if (bnapi->status_blk.msi) {
 		dma_free_coherent(&bp->pdev->dev, bp->status_stats_size,
 				  bnapi->status_blk.msi,

@@ -839,11 +828,12 @@ bnx2_free_mem(struct bnx2 *bp)
 }
 
 static int
-bnx2_alloc_mem(struct bnx2 *bp)
+bnx2_alloc_stats_blk(struct net_device *dev)
 {
-	int i, status_blk_size, err;
+	int i, status_blk_size;
 	struct bnx2_napi *bnapi;
 	void *status_blk;
+	struct bnx2 *bp = netdev_priv(dev);
 
 	/* Combine status and statistics blocks into one allocation. */
 	status_blk_size = L1_CACHE_ALIGN(sizeof(struct status_block));

@@ -852,11 +842,10 @@ bnx2_alloc_mem(struct bnx2 *bp)
 						 BNX2_SBLK_MSIX_ALIGN_SIZE);
 	bp->status_stats_size = status_blk_size +
 				sizeof(struct statistics_block);
-
 	status_blk = dma_zalloc_coherent(&bp->pdev->dev, bp->status_stats_size,
 					 &bp->status_blk_mapping, GFP_KERNEL);
 	if (status_blk == NULL)
-		goto alloc_mem_err;
+		return -ENOMEM;
 
 	bnapi = &bp->bnx2_napi[0];
 	bnapi->status_blk.msi = status_blk;

@@ -865,11 +854,10 @@ bnx2_alloc_mem(struct bnx2 *bp)
 	bnapi->hw_rx_cons_ptr =
 		&bnapi->status_blk.msi->status_rx_quick_consumer_index0;
 	if (bp->flags & BNX2_FLAG_MSIX_CAP) {
-		for (i = 1; i < bp->irq_nvecs; i++) {
+		for (i = 1; i < BNX2_MAX_MSIX_HW_VEC; i++) {
 			struct status_block_msix *sblk;
 
 			bnapi = &bp->bnx2_napi[i];
-
 			sblk = (status_blk + BNX2_SBLK_MSIX_ALIGN_SIZE * i);
 			bnapi->status_blk.msix = sblk;
 			bnapi->hw_tx_cons_ptr =

@@ -879,11 +867,35 @@ bnx2_alloc_mem(struct bnx2 *bp)
 			bnapi->int_num = i << 24;
 		}
 	}
-
 	bp->stats_blk = status_blk + status_blk_size;
-
 	bp->stats_blk_mapping = bp->status_blk_mapping + status_blk_size;
 
+	return 0;
+}
+
+static void
+bnx2_free_mem(struct bnx2 *bp)
+{
+	int i;
+
+	bnx2_free_tx_mem(bp);
+	bnx2_free_rx_mem(bp);
+
+	for (i = 0; i < bp->ctx_pages; i++) {
+		if (bp->ctx_blk[i]) {
+			dma_free_coherent(&bp->pdev->dev, BNX2_PAGE_SIZE,
+					  bp->ctx_blk[i],
+					  bp->ctx_blk_mapping[i]);
+			bp->ctx_blk[i] = NULL;
+		}
+	}
+}
+
+static int
+bnx2_alloc_mem(struct bnx2 *bp)
+{
+	int i, err;
+
 	if (BNX2_CHIP(bp) == BNX2_CHIP_5709) {
 		bp->ctx_pages = 0x2000 / BNX2_PAGE_SIZE;
 		if (bp->ctx_pages == 0)

@@ -8330,6 +8342,11 @@ bnx2_init_board(struct pci_dev *pdev, struct net_device *dev)
 
 	bp->phy_addr = 1;
 
+	/* allocate stats_blk */
+	rc = bnx2_alloc_stats_blk(dev);
+	if (rc)
+		goto err_out_unmap;
+
 	/* Disable WOL support if we are running on a SERDES chip. */
 	if (BNX2_CHIP(bp) == BNX2_CHIP_5709)
 		bnx2_get_5709_media(bp);

@@ -8586,6 +8603,7 @@ error:
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
 err_free:
+	bnx2_free_stats_blk(dev);
 	free_netdev(dev);
 	return rc;
 }

@@ -8603,6 +8621,7 @@ bnx2_remove_one(struct pci_dev *pdev)
 
 	pci_iounmap(bp->pdev, bp->regview);
 
+	bnx2_free_stats_blk(dev);
 	kfree(bp->temp_stats_blk);
 
 	if (bp->flags & BNX2_FLAG_AER_ENABLED) {
Signature · 1 line
1.9.0
maintainer
#8 lore raw

From: Weidong Wang <[email protected]>
Date: Tue, 29 Sep 2015 11:18:18 +0800

Quoted 5 lines
drivers/net/ethernet/broadcom/bnx2.c +0 -1
@@ -839,11 +828,12 @@ bnx2_free_mem(struct bnx2 *bp)
 }
 
 static int
-bnx2_alloc_mem(struct bnx2 *bp)

drivers/net/ethernet/broadcom/bnx2.c bnx2_free_mem(struct bnx2 *bp) · line 839

Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2.c +3 -1
+bnx2_alloc_stats_blk(struct net_device *dev)
 {
-	int i, status_blk_size, err;
+	int i, status_blk_size;
 	struct bnx2_napi *bnapi;
 	void *status_blk;
+	struct bnx2 *bp = netdev_priv(dev);
 
 	/* Combine status and statistics blocks into one allocation. */
 	status_blk_size = L1_CACHE_ALIGN(sizeof(struct status_block));

This function is not just allocating the stats block, it's allocating a whole bunch of other things too.

Only allocate the stats block at probe time, not the NAPI et al. stuff as well. That can safely stay in the open/close paths.

author
#9 lore raw

PATCH v3 1/1 BNX2: fix a Null Pointer for stats_blk

we have two processes to do:
P1#: ifconfig eth0 down; which will call bnx2_close, then will , and set Null to stats_blk
P2#: ifconfig eth0; which will call bnx2_get_stats64, it will use stats_blk.
In one case:

    --P1#--                   --P2#--
                              stats_blk(no null)
    bnx2_free_mem
    ->bp->stats_blk = NULL
                              GET_64BIT_NET_STATS

then it will cause 'NULL Pointer' Problem.
it is as well with 'ethtool -S ethx'.

Allocate the statistics block at probe time so that this problem is impossible

Signed-off-by Wang Weidong <[email protected]>

---
Change in v2:
- Use Allocate the statistics block instead of spinlock, which suggested by David Miller.
- Updating commit message according to changes.

Change in v3:
- bnx2_alloc_stats_blk is just allocating the stats block.
- use 'bp->status_blk' to store the status_blk which would used in other funcs, just to key the codes simplified.

---
 drivers/net/ethernet/broadcom/bnx2.c | 79 ++++++++++++++++++++++++------------
 drivers/net/ethernet/broadcom/bnx2.h |  1 +
 2 files changed, 53 insertions(+), 27 deletions(-)
drivers/net/ethernet/broadcom/bnx2.c +52 -27
index 2b66ef3..6259064 100644
@@ -813,6 +813,46 @@ bnx2_alloc_rx_mem(struct bnx2 *bp)
 }
 
 static void
+bnx2_free_stats_blk(struct net_device *dev)
+{
+	struct bnx2 *bp = netdev_priv(dev);
+
+	if (bp->status_blk) {
+		dma_free_coherent(&bp->pdev->dev, bp->status_stats_size,
+				  bp->status_blk,
+				  bp->status_blk_mapping);
+		bp->status_blk = NULL;
+		bp->stats_blk = NULL;
+	}
+}
+
+static int
+bnx2_alloc_stats_blk(struct net_device *dev)
+{
+	int status_blk_size;
+	void *status_blk;
+	struct bnx2 *bp = netdev_priv(dev);
+
+	/* Combine status and statistics blocks into one allocation. */
+	status_blk_size = L1_CACHE_ALIGN(sizeof(struct status_block));
+	if (bp->flags & BNX2_FLAG_MSIX_CAP)
+		status_blk_size = L1_CACHE_ALIGN(BNX2_MAX_MSIX_HW_VEC *
+						 BNX2_SBLK_MSIX_ALIGN_SIZE);
+	bp->status_stats_size = status_blk_size +
+				sizeof(struct statistics_block);
+	status_blk = dma_zalloc_coherent(&bp->pdev->dev, bp->status_stats_size,
+					 &bp->status_blk_mapping, GFP_KERNEL);
+	if (status_blk == NULL)
+		return -ENOMEM;
+
+	bp->status_blk = status_blk;
+	bp->stats_blk = status_blk + status_blk_size;
+	bp->stats_blk_mapping = bp->status_blk_mapping + status_blk_size;
+
+	return 0;
+}
+
+static void
 bnx2_free_mem(struct bnx2 *bp)
 {
 	int i;

@@ -829,37 +869,19 @@ bnx2_free_mem(struct bnx2 *bp)
 			bp->ctx_blk[i] = NULL;
 		}
 	}
-	if (bnapi->status_blk.msi) {
-		dma_free_coherent(&bp->pdev->dev, bp->status_stats_size,
-				  bnapi->status_blk.msi,
-				  bp->status_blk_mapping);
+
+	if (bnapi->status_blk.msi)
 		bnapi->status_blk.msi = NULL;
-		bp->stats_blk = NULL;
-	}
 }
 
 static int
 bnx2_alloc_mem(struct bnx2 *bp)
 {
-	int i, status_blk_size, err;
+	int i, err;
 	struct bnx2_napi *bnapi;
-	void *status_blk;
-
-	/* Combine status and statistics blocks into one allocation. */
-	status_blk_size = L1_CACHE_ALIGN(sizeof(struct status_block));
-	if (bp->flags & BNX2_FLAG_MSIX_CAP)
-		status_blk_size = L1_CACHE_ALIGN(BNX2_MAX_MSIX_HW_VEC *
-						 BNX2_SBLK_MSIX_ALIGN_SIZE);
-	bp->status_stats_size = status_blk_size +
-				sizeof(struct statistics_block);
-
-	status_blk = dma_zalloc_coherent(&bp->pdev->dev, bp->status_stats_size,
-					 &bp->status_blk_mapping, GFP_KERNEL);
-	if (status_blk == NULL)
-		goto alloc_mem_err;
 
 	bnapi = &bp->bnx2_napi[0];
-	bnapi->status_blk.msi = status_blk;
+	bnapi->status_blk.msi = bp->status_blk;
 	bnapi->hw_tx_cons_ptr =
 		&bnapi->status_blk.msi->status_tx_quick_consumer_index0;
 	bnapi->hw_rx_cons_ptr =

@@ -870,7 +892,7 @@ bnx2_alloc_mem(struct bnx2 *bp)
 
 			bnapi = &bp->bnx2_napi[i];
 
-			sblk = (status_blk + BNX2_SBLK_MSIX_ALIGN_SIZE * i);
+			sblk = (bp->status_blk + BNX2_SBLK_MSIX_ALIGN_SIZE * i);
 			bnapi->status_blk.msix = sblk;
 			bnapi->hw_tx_cons_ptr =
 				&sblk->status_tx_quick_consumer_index;

@@ -880,10 +902,6 @@ bnx2_alloc_mem(struct bnx2 *bp)
 		}
 	}
 
-	bp->stats_blk = status_blk + status_blk_size;
-
-	bp->stats_blk_mapping = bp->status_blk_mapping + status_blk_size;
-
 	if (BNX2_CHIP(bp) == BNX2_CHIP_5709) {
 		bp->ctx_pages = 0x2000 / BNX2_PAGE_SIZE;
 		if (bp->ctx_pages == 0)

@@ -8330,6 +8348,11 @@ bnx2_init_board(struct pci_dev *pdev, struct net_device *dev)
 
 	bp->phy_addr = 1;
 
+	/* allocate stats_blk */
+	rc = bnx2_alloc_stats_blk(dev);
+	if (rc)
+		goto err_out_unmap;
+
 	/* Disable WOL support if we are running on a SERDES chip. */
 	if (BNX2_CHIP(bp) == BNX2_CHIP_5709)
 		bnx2_get_5709_media(bp);

@@ -8586,6 +8609,7 @@ error:
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
 err_free:
+	bnx2_free_stats_blk(dev);
 	free_netdev(dev);
 	return rc;
 }

@@ -8603,6 +8627,7 @@ bnx2_remove_one(struct pci_dev *pdev)
 
 	pci_iounmap(bp->pdev, bp->regview);
 
+	bnx2_free_stats_blk(dev);
 	kfree(bp->temp_stats_blk);
 
 	if (bp->flags & BNX2_FLAG_AER_ENABLED) {
drivers/net/ethernet/broadcom/bnx2.h +1 -0
index f92f76c..380234d 100644
@@ -6928,6 +6928,7 @@ struct bnx2 {
 
 	dma_addr_t		status_blk_mapping;
 
+	void *status_blk;
 	struct statistics_block	*stats_blk;
 	struct statistics_block	*temp_stats_blk;
 	dma_addr_t		stats_blk_mapping;
Signature · 1 line
1.9.0
maintainer Applied
#10 lore raw

From: Weidong Wang <[email protected]>
Date: Thu, 8 Oct 2015 18:03:47 +0800

Quoted 12 lines

we have two processes to do:
P1#: ifconfig eth0 down; which will call bnx2_close, then will , and set Null to stats_blk
P2#: ifconfig eth0; which will call bnx2_get_stats64, it will use stats_blk.
In one case:

    --P1#--                   --P2#--
                              stats_blk(no null)
    bnx2_free_mem
    ->bp->stats_blk = NULL
                              GET_64BIT_NET_STATS
Quoted 7 lines

then it will cause 'NULL Pointer' Problem.
it is as well with 'ethtool -S ethx'.

Allocate the statistics block at probe time so that this problem is impossible

Signed-off-by Wang Weidong <[email protected]>

Applied, thank you.