* [PATCH net-next] BNX2: fix a Null Pointer for stats_blk
@ 2015-09-22 12:42 Weidong Wang
2015-09-23 22:31 ` David Miller
2015-09-28 7:01 ` [PATCH net-next v2] " Weidong Wang
0 siblings, 2 replies; 11+ messages in thread
From: Weidong Wang @ 2015-09-22 12:42 UTC (permalink / raw)
To: sony.chacko, Dept-HSGLinuxNICDev; +Cc: linux-kernel, netdev, davem, rui.xiang
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] BNX2: fix a Null Pointer for stats_blk
2015-09-22 12:42 [PATCH net-next] BNX2: fix a Null Pointer for stats_blk Weidong Wang
@ 2015-09-23 22:31 ` David Miller
2015-09-24 2:00 ` Weidong Wang
2015-09-28 7:01 ` [PATCH net-next v2] " Weidong Wang
1 sibling, 1 reply; 11+ messages in thread
From: David Miller @ 2015-09-23 22:31 UTC (permalink / raw)
To: wangweidong1
Cc: sony.chacko, Dept-HSGLinuxNICDev, linux-kernel, netdev, rui.xiang
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] BNX2: fix a Null Pointer for stats_blk
2015-09-23 22:31 ` David Miller
@ 2015-09-24 2:00 ` Weidong Wang
2015-09-24 5:34 ` David Miller
0 siblings, 1 reply; 11+ messages in thread
From: Weidong Wang @ 2015-09-24 2:00 UTC (permalink / raw)
To: David Miller
Cc: sony.chacko, Dept-HSGLinuxNICDev, linux-kernel, netdev, rui.xiang
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
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] BNX2: fix a Null Pointer for stats_blk
2015-09-24 2:00 ` Weidong Wang
@ 2015-09-24 5:34 ` David Miller
2015-09-24 6:53 ` Weidong Wang
0 siblings, 1 reply; 11+ messages in thread
From: David Miller @ 2015-09-24 5:34 UTC (permalink / raw)
To: wangweidong1
Cc: sony.chacko, Dept-HSGLinuxNICDev, linux-kernel, netdev, rui.xiang
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] BNX2: fix a Null Pointer for stats_blk
2015-09-24 5:34 ` David Miller
@ 2015-09-24 6:53 ` Weidong Wang
0 siblings, 0 replies; 11+ messages in thread
From: Weidong Wang @ 2015-09-24 6:53 UTC (permalink / raw)
To: David Miller
Cc: sony.chacko, Dept-HSGLinuxNICDev, linux-kernel, netdev, rui.xiang
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next v2] BNX2: fix a Null Pointer for stats_blk
2015-09-22 12:42 [PATCH net-next] BNX2: fix a Null Pointer for stats_blk Weidong Wang
2015-09-23 22:31 ` David Miller
@ 2015-09-28 7:01 ` Weidong Wang
2015-09-29 3:15 ` Weidong Wang
2015-09-29 3:18 ` [PATCH net-next v2 RESEND] " Weidong Wang
1 sibling, 2 replies; 11+ messages in thread
From: Weidong Wang @ 2015-09-28 7:01 UTC (permalink / raw)
To: sony.chacko, Dept-HSGLinuxNICDev, David Miller
Cc: linux-kernel, netdev, rui.xiang, manish.chopra
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v2] BNX2: fix a Null Pointer for stats_blk
2015-09-28 7:01 ` [PATCH net-next v2] " Weidong Wang
@ 2015-09-29 3:15 ` Weidong Wang
2015-09-29 3:18 ` [PATCH net-next v2 RESEND] " Weidong Wang
1 sibling, 0 replies; 11+ messages in thread
From: Weidong Wang @ 2015-09-29 3:15 UTC (permalink / raw)
To: sony.chacko, Dept-HSGLinuxNICDev, David Miller
Cc: linux-kernel, netdev, rui.xiang, manish.chopra
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next v2 RESEND] BNX2: fix a Null Pointer for stats_blk
2015-09-28 7:01 ` [PATCH net-next v2] " Weidong Wang
2015-09-29 3:15 ` Weidong Wang
@ 2015-09-29 3:18 ` Weidong Wang
2015-09-30 4:10 ` David Miller
1 sibling, 1 reply; 11+ messages in thread
From: Weidong Wang @ 2015-09-29 3:18 UTC (permalink / raw)
To: sony.chacko, Dept-HSGLinuxNICDev, David Miller
Cc: linux-kernel, netdev, rui.xiang, manish.chopra
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v2 RESEND] BNX2: fix a Null Pointer for stats_blk
2015-09-29 3:18 ` [PATCH net-next v2 RESEND] " Weidong Wang
@ 2015-09-30 4:10 ` David Miller
2015-10-08 10:03 ` [PATCH net-next v3] " Weidong Wang
0 siblings, 1 reply; 11+ messages in thread
From: David Miller @ 2015-09-30 4:10 UTC (permalink / raw)
To: wangweidong1
Cc: sony.chacko, Dept-HSGLinuxNICDev, linux-kernel, netdev,
rui.xiang, manish.chopra
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next v3] BNX2: fix a Null Pointer for stats_blk
2015-09-30 4:10 ` David Miller
@ 2015-10-08 10:03 ` Weidong Wang
2015-10-11 12:07 ` David Miller
0 siblings, 1 reply; 11+ messages in thread
From: Weidong Wang @ 2015-10-08 10:03 UTC (permalink / raw)
To: David Miller
Cc: sony.chacko, Dept-HSGLinuxNICDev, linux-kernel, netdev,
rui.xiang, manish.chopra
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v3] BNX2: fix a Null Pointer for stats_blk
2015-10-08 10:03 ` [PATCH net-next v3] " Weidong Wang
@ 2015-10-11 12:07 ` David Miller
0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2015-10-11 12:07 UTC (permalink / raw)
To: wangweidong1
Cc: sony.chacko, Dept-HSGLinuxNICDev, linux-kernel, netdev,
rui.xiang, manish.chopra
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-10-11 11:51 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-22 12:42 [PATCH net-next] BNX2: fix a Null Pointer for stats_blk Weidong Wang
2015-09-23 22:31 ` David Miller
2015-09-24 2:00 ` Weidong Wang
2015-09-24 5:34 ` David Miller
2015-09-24 6:53 ` Weidong Wang
2015-09-28 7:01 ` [PATCH net-next v2] " Weidong Wang
2015-09-29 3:15 ` Weidong Wang
2015-09-29 3:18 ` [PATCH net-next v2 RESEND] " Weidong Wang
2015-09-30 4:10 ` David Miller
2015-10-08 10:03 ` [PATCH net-next v3] " Weidong Wang
2015-10-11 12:07 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.