[PATCH net-next 1/2] pcnet32: fix reallocation error

[PATCH net-next 1/2] pcnet32: fix reallocation error

[Don Fry]

pcnet32_realloc_rx_ring() only worked on the first log2 number of
entries in the receive ring instead of the all the entries.
Replaced "1 << size" with more descriptive variable.
This is my original bug from 2006.  Found while testing another problem.
Tested on 79C972 and 79C976.

Signed-off-by: Don Fry <pcnet32@frontier.com>
---
 drivers/net/ethernet/amd/pcnet32.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/amd/pcnet32.c b/drivers/net/ethernet/amd/pcnet32.c
index 9339ccc..25cd04a 100644
--- a/drivers/net/ethernet/amd/pcnet32.c
+++ b/drivers/net/ethernet/amd/pcnet32.c
@@ -549,35 +549,36 @@ static void pcnet32_realloc_rx_ring(struct net_device *dev,
 	struct pcnet32_rx_head *new_rx_ring;
 	struct sk_buff **new_skb_list;
 	int new, overlap;
+	unsigned int entries = 1 << size;
 
 	new_rx_ring = pci_alloc_consistent(lp->pci_dev,
 					   sizeof(struct pcnet32_rx_head) *
-					   (1 << size),
+					   entries,
 					   &new_ring_dma_addr);
 	if (new_rx_ring == NULL) {
 		netif_err(lp, drv, dev, "Consistent memory allocation failed\n");
 		return;
 	}
-	memset(new_rx_ring, 0, sizeof(struct pcnet32_rx_head) * (1 << size));
+	memset(new_rx_ring, 0, sizeof(struct pcnet32_rx_head) * entries);
 
-	new_dma_addr_list = kcalloc(1 << size, sizeof(dma_addr_t), GFP_ATOMIC);
+	new_dma_addr_list = kcalloc(entries, sizeof(dma_addr_t), GFP_ATOMIC);
 	if (!new_dma_addr_list)
 		goto free_new_rx_ring;
 
-	new_skb_list = kcalloc(1 << size, sizeof(struct sk_buff *),
+	new_skb_list = kcalloc(entries, sizeof(struct sk_buff *),
 			       GFP_ATOMIC);
 	if (!new_skb_list)
 		goto free_new_lists;
 
 	/* first copy the current receive buffers */
-	overlap = min(size, lp->rx_ring_size);
+	overlap = min(entries, lp->rx_ring_size);
 	for (new = 0; new < overlap; new++) {
 		new_rx_ring[new] = lp->rx_ring[new];
 		new_dma_addr_list[new] = lp->rx_dma_addr[new];
 		new_skb_list[new] = lp->rx_skbuff[new];
 	}
 	/* now allocate any new buffers needed */
-	for (; new < size; new++) {
+	for (; new < entries; new++) {
 		struct sk_buff *rx_skbuff;
 		new_skb_list[new] = netdev_alloc_skb(dev, PKT_BUF_SKB);
 		rx_skbuff = new_skb_list[new];
@@ -612,7 +613,7 @@ static void pcnet32_realloc_rx_ring(struct net_device *dev,
 			    lp->rx_ring_size, lp->rx_ring,
 			    lp->rx_ring_dma_addr);
 
-	lp->rx_ring_size = (1 << size);
+	lp->rx_ring_size = entries;
 	lp->rx_mod_mask = lp->rx_ring_size - 1;
 	lp->rx_len_bits = (size << 4);
 	lp->rx_ring = new_rx_ring;
@@ -634,8 +635,7 @@ free_new_lists:
 	kfree(new_dma_addr_list);
 free_new_rx_ring:
 	pci_free_consistent(lp->pci_dev,
-			    sizeof(struct pcnet32_rx_head) *
-			    (1 << size),
+			    sizeof(struct pcnet32_rx_head) * entries,
 			    new_rx_ring,
 			    new_ring_dma_addr);
 }
-- 
1.7.2.3

Re: [PATCH net-next 1/2] pcnet32: fix reallocation error

[Tetsuo Handa]

Don Fry wrote:
> pcnet32_realloc_rx_ring() only worked on the first log2 number of
> entries in the receive ring instead of the all the entries.
> Replaced "1 << size" with more descriptive variable.

>  	/* first copy the current receive buffers */
> -	overlap = min(size, lp->rx_ring_size);
> +	overlap = min(entries, lp->rx_ring_size);

>  	/* now allocate any new buffers needed */
> -	for (; new < size; new++) {
> +	for (; new < entries; new++) {

Until this fix, lp->rx_skbuff[size...(1<<size)-1] were not allocated by
netdev_alloc_skb() and lp->rx_ring_dma_addr[size...(1<<size)-1] were not mapped
by pci_map_single(), right?

Since lp->rx_ring_size was set to (1<<size), wasn't there possibility that
something bad happens by accessing lp->rx_skbuff[] and lp->rx_ring_dma_addr[]
up to (1<<size)-1 ? (In other words, don't we want to backport this fix?)

Re: [PATCH net-next 1/2] pcnet32: fix reallocation error

[Don Fry]

-----Original Message-----
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Subject: Re: [PATCH net-next 1/2] pcnet32: fix reallocation error
Date: Tue, 18 Feb 2014 21:29:00 +0900

>Don Fry wrote:
>> pcnet32_realloc_rx_ring() only worked on the first log2 number of
>> entries in the receive ring instead of the all the entries.
>> Replaced "1 << size" with more descriptive variable.
>
>>  	/* first copy the current receive buffers */
>> -	overlap = min(size, lp->rx_ring_size);
>> +	overlap = min(entries, lp->rx_ring_size);
>
>>  	/* now allocate any new buffers needed */
>> -	for (; new < size; new++) {
>> +	for (; new < entries; new++) {
>>
>Until this fix, lp->rx_skbuff[size...(1<<size)-1] were not allocated by
>netdev_alloc_skb() and lp->rx_ring_dma_addr[size...(1<<size)-1] were not mapped
>by pci_map_single(), right?
>
>Since lp->rx_ring_size was set to (1<<size), wasn't there possibility that
>something bad happens by accessing lp->rx_skbuff[] and lp->rx_ring_dma_addr[]
>up to (1<<size)-1 ? (In other words, don't we want to backport this fix?)
>

I looked into this when testing.  If the device is operational
(netif_running) when the ring is resized, it will do pcnet32_restart and
make sure all rx buffers are full.  If the device is not operational it
will leave the ring partially full until it is opened and then the ring
is filled via pcnet32_init_ring().  Another thing that could happen
while it is down is that a selftest is done (offline) which also calls
pcnet32_restart and makes sure the ring is full.  I could not find a
case that would cause a problem.  No-one has reported this in the last 8
years so it does not look like backporting this fix is needed, in my
opinion.

Re: [PATCH net-next 1/2] pcnet32: fix reallocation error

[David Miller]

From: Don Fry <pcnet32@frontier.com>
Date: Mon, 17 Feb 2014 20:57:46 -0800

> pcnet32_realloc_rx_ring() only worked on the first log2 number of
> entries in the receive ring instead of the all the entries.
> Replaced "1 << size" with more descriptive variable.
> This is my original bug from 2006.  Found while testing another problem.
> Tested on 79C972 and 79C976.
> 
> Signed-off-by: Don Fry <pcnet32@frontier.com>

Applied.