[PATCH net-next] BNX2: fix a Null Pointer for stats_blk

[PATCH net-next] BNX2: fix a Null Pointer for stats_blk

[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'.

BTW, the other branch has this problem as well.

So we add a spin_lock to protect stats_blk.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 drivers/net/ethernet/broadcom/bnx2.c | 42 ++++++++++++++++++++++++++++--------
 drivers/net/ethernet/broadcom/bnx2.h |  1 +
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index 2b66ef3..aec4081 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -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
diff --git a/drivers/net/ethernet/broadcom/bnx2.h b/drivers/net/ethernet/broadcom/bnx2.h
index f92f76c..c88c21b 100644
--- a/drivers/net/ethernet/broadcom/bnx2.h
+++ b/drivers/net/ethernet/broadcom/bnx2.h
@@ -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;
-- 
1.7.12

Re: [PATCH net-next] BNX2: fix a Null Pointer for stats_blk

[David Miller]

From: Weidong Wang <wangweidong1@huawei.com>
Date: Tue, 22 Sep 2015 20:42:40 +0800

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

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.

Re: [PATCH net-next] BNX2: fix a Null Pointer for stats_blk

[Weidong Wang]

On 2015/9/24 6:31, David Miller wrote:
> From: Weidong Wang <wangweidong1@huawei.com>
> Date: Tue, 22 Sep 2015 20:42:40 +0800
> 
>> @@ -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;
> 
> 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.

> 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

> .
> 

Re: [PATCH net-next] BNX2: fix a Null Pointer for stats_blk

[David Miller]

From: Weidong Wang <wangweidong1@huawei.com>
Date: Thu, 24 Sep 2015 10:00:45 +0800

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

Re: [PATCH net-next] BNX2: fix a Null Pointer for stats_blk

[Weidong Wang]

On 2015/9/24 13:34, David Miller wrote:
> From: Weidong Wang <wangweidong1@huawei.com>
> Date: Thu, 24 Sep 2015 10:00:45 +0800
> 
>> 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

> 

[PATCH net-next v2] BNX2: fix a Null Pointer for stats_blk

[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 <dingtianhong@huawei.com>
---
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(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index 2b66ef3..1f33982 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -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) {
-- 
1.9.0

Re: [PATCH net-next v2] BNX2: fix a Null Pointer for stats_blk

[Weidong Wang]

On 2015/9/28 15:01, Weidong Wang wrote:
> 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 <dingtianhong@huawei.com>
> ---

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

Just Ignore it.

Regards.
Weidong

[PATCH net-next v2 RESEND] BNX2: fix a Null Pointer for stats_blk

[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 <wangweidong1@huawei.com>
---
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(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index 2b66ef3..1f33982 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -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) {
-- 
1.9.0

Re: [PATCH net-next v2 RESEND] BNX2: fix a Null Pointer for stats_blk

[David Miller]

From: Weidong Wang <wangweidong1@huawei.com>
Date: Tue, 29 Sep 2015 11:18:18 +0800

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

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.

[PATCH net-next v3] BNX2: fix a Null Pointer for stats_blk

[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: Wang Weidong <wangweidong1@huawei.com>
---
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(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index 2b66ef3..6259064 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -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) {
diff --git a/drivers/net/ethernet/broadcom/bnx2.h b/drivers/net/ethernet/broadcom/bnx2.h
index f92f76c..380234d 100644
--- a/drivers/net/ethernet/broadcom/bnx2.h
+++ b/drivers/net/ethernet/broadcom/bnx2.h
@@ -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;
-- 
1.9.0

Re: [PATCH net-next v3] BNX2: fix a Null Pointer for stats_blk

[David Miller]

From: Weidong Wang <wangweidong1@huawei.com>
Date: Thu, 8 Oct 2015 18:03:47 +0800

> 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 <wangweidong1@huawei.com>

Applied, thank you.