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

[David Miller]

From: Weidong Wang 
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 

Applied, thank you.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 
---
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(>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(>pdev->dev, bp->status_stats_size,
+>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(>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(>pdev->dev, bp->status_stats_size,
->status_blk_mapping, GFP_KERNEL);
-   if (status_blk == NULL)
-   goto alloc_mem_err;

bnapi = >bnx2_napi[0];
-   bnapi->status_blk.msi = status_blk;
+   bnapi->status_blk.msi = bp->status_blk;
bnapi->hw_tx_cons_ptr =
>status_blk.msi->status_tx_quick_consumer_index0;
bnapi->hw_rx_cons_ptr =
@@ -870,7 +892,7 @@ bnx2_alloc_mem(struct bnx2 *bp)

bnapi = >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 =
>status_tx_quick_consumer_index;
@@ -880,10 +902,6 @@ bnx2_alloc_mem(struct bnx2 *bp)
}
}

-