| From foo@baz Sat 28 Dec 2019 06:10:39 AM EST |
| From: Ben Hutchings <ben@decadent.org.uk> |
| Date: Tue, 17 Dec 2019 01:57:40 +0000 |
| Subject: net: qlogic: Fix error paths in ql_alloc_large_buffers() |
| |
| From: Ben Hutchings <ben@decadent.org.uk> |
| |
| [ Upstream commit cad46039e4c99812db067c8ac22a864960e7acc4 ] |
| |
| 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> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| drivers/net/ethernet/qlogic/qla3xxx.c | 8 ++++---- |
| 1 file changed, 4 insertions(+), 4 deletions(-) |
| |
| --- 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 |
| 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 |
| 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 |
| 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 - |