[PATCH net] net: qlogic: Fix error paths in ql_alloc_large_buffers()

[PATCH net] net: qlogic: Fix error paths in ql_alloc_large_buffers()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2742 bytes --]

ql_alloc_large_buffers() has the usual RX buffer allocation
loop where it allocates skbs and maps them for DMA.  It also
treats failure as a fatal error.

There are (at least) three bugs in the error paths:

1. ql_free_large_buffers() assumes that the lrg_buf[] entry for the
first buffer that couldn't be allocated will have .skb == NULL.
But the qla_buf[] array is not zero-initialised.

2. ql_free_large_buffers() DMA-unmaps all skbs in lrg_buf[].  This is
incorrect for the last allocated skb, if DMA mapping failed.

3. Commit 1acb8f2a7a9f ("net: qlogic: Fix memory leak in
ql_alloc_large_buffers") added a direct call to dev_kfree_skb_any()
after the skb is recorded in lrg_buf[], so ql_free_large_buffers()
will double-free it.

The bugs are somewhat inter-twined, so fix them all at once:

* Clear each entry in qla_buf[] before attempting to allocate
  an skb for it.  This goes half-way to fixing bug 1.
* Set the .skb field only after the skb is DMA-mapped.  This
  fixes the rest.

Fixes: 1357bfcf7106 ("qla3xxx: Dynamically size the rx buffer queue ...")
Fixes: 0f8ab89e825f ("qla3xxx: Check return code from pci_map_single() ...")
Fixes: 1acb8f2a7a9f ("net: qlogic: Fix memory leak in ql_alloc_large_buffers")
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
This is compile-tested only, unfortunately.  But if it's correct, it
needs to go to stable.

Ben.

 drivers/net/ethernet/qlogic/qla3xxx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qla3xxx.c b/drivers/net/ethernet/qlogic/qla3xxx.c
index b4b8ba00ee01..986f26578d34 100644
--- a/drivers/net/ethernet/qlogic/qla3xxx.c
+++ b/drivers/net/ethernet/qlogic/qla3xxx.c
@@ -2756,6 +2756,9 @@ static int ql_alloc_large_buffers(struct ql3_adapter *qdev)
 	int err;
 
 	for (i = 0; i < qdev->num_large_buffers; i++) {
+		lrg_buf_cb = &qdev->lrg_buf[i];
+		memset(lrg_buf_cb, 0, sizeof(struct ql_rcv_buf_cb));
+
 		skb = netdev_alloc_skb(qdev->ndev,
 				       qdev->lrg_buffer_len);
 		if (unlikely(!skb)) {
@@ -2766,11 +2769,7 @@ static int ql_alloc_large_buffers(struct ql3_adapter *qdev)
 			ql_free_large_buffers(qdev);
 			return -ENOMEM;
 		} else {
-
-			lrg_buf_cb = &qdev->lrg_buf[i];
-			memset(lrg_buf_cb, 0, sizeof(struct ql_rcv_buf_cb));
 			lrg_buf_cb->index = i;
-			lrg_buf_cb->skb = skb;
 			/*
 			 * We save some space to copy the ethhdr from first
 			 * buffer
@@ -2792,6 +2791,7 @@ static int ql_alloc_large_buffers(struct ql3_adapter *qdev)
 				return -ENOMEM;
 			}
 
+			lrg_buf_cb->skb = skb;
 			dma_unmap_addr_set(lrg_buf_cb, mapaddr, map);
 			dma_unmap_len_set(lrg_buf_cb, maplen,
 					  qdev->lrg_buffer_len -

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net] net: qlogic: Fix error paths in ql_alloc_large_buffers()

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Tue, 17 Dec 2019 01:57:40 +0000

> ql_alloc_large_buffers() has the usual RX buffer allocation
> loop where it allocates skbs and maps them for DMA.  It also
> treats failure as a fatal error.
> 
> There are (at least) three bugs in the error paths:
> 
> 1. ql_free_large_buffers() assumes that the lrg_buf[] entry for the
> first buffer that couldn't be allocated will have .skb == NULL.
> But the qla_buf[] array is not zero-initialised.
> 
> 2. ql_free_large_buffers() DMA-unmaps all skbs in lrg_buf[].  This is
> incorrect for the last allocated skb, if DMA mapping failed.
> 
> 3. Commit 1acb8f2a7a9f ("net: qlogic: Fix memory leak in
> ql_alloc_large_buffers") added a direct call to dev_kfree_skb_any()
> after the skb is recorded in lrg_buf[], so ql_free_large_buffers()
> will double-free it.
> 
> The bugs are somewhat inter-twined, so fix them all at once:
> 
> * Clear each entry in qla_buf[] before attempting to allocate
>   an skb for it.  This goes half-way to fixing bug 1.
> * Set the .skb field only after the skb is DMA-mapped.  This
>   fixes the rest.
> 
> Fixes: 1357bfcf7106 ("qla3xxx: Dynamically size the rx buffer queue ...")
> Fixes: 0f8ab89e825f ("qla3xxx: Check return code from pci_map_single() ...")
> Fixes: 1acb8f2a7a9f ("net: qlogic: Fix memory leak in ql_alloc_large_buffers")
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

I've been over this a few times, applied and queued up for -stable.

Thanks.