| Message ID | 1496442569-11307-1-git-send-email-housel@acm.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On Sat, Jun 3, 2017 at 1:29 AM, Peter S. Housel <housel@acm.org> wrote: > An earlier change to this function (3bdae810721b) fixed a leak in the > case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the > glom_skb buffer, used for emulating a scattering read, is never used > or referenced after its contents are copied into the destination > buffers, and therefore always needs to be freed by the end of the > function. > err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, false, addr, > glom_skb); > - if (err) { > - brcmu_pkt_buf_free_skb(glom_skb); > - goto done; > - } > - > - skb_queue_walk(pktq, skb) { > - memcpy(skb->data, glom_skb->data, skb->len); > - skb_pull(glom_skb, skb->len); > + if (!err) { This is not so often in use type of pattern. > + skb_queue_walk(pktq, skb) { > + memcpy(skb->data, glom_skb->data, skb->len); > + skb_pull(glom_skb, skb->len); > + } > } > + brcmu_pkt_buf_free_skb(glom_skb); Can we just add this one line instead or I'm missing something? > } else > err = brcmf_sdiod_sglist_rw(sdiodev, SDIO_FUNC_2, false, addr, > pktq);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index 9b970dc..30fb54e 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -727,15 +727,13 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev, return -ENOMEM; err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, false, addr, glom_skb); - if (err) { - brcmu_pkt_buf_free_skb(glom_skb); - goto done; - } - - skb_queue_walk(pktq, skb) { - memcpy(skb->data, glom_skb->data, skb->len); - skb_pull(glom_skb, skb->len); + if (!err) { + skb_queue_walk(pktq, skb) { + memcpy(skb->data, glom_skb->data, skb->len); + skb_pull(glom_skb, skb->len); + } } + brcmu_pkt_buf_free_skb(glom_skb); } else err = brcmf_sdiod_sglist_rw(sdiodev, SDIO_FUNC_2, false, addr, pktq);
An earlier change to this function (3bdae810721b) fixed a leak in the case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the glom_skb buffer, used for emulating a scattering read, is never used or referenced after its contents are copied into the destination buffers, and therefore always needs to be freed by the end of the function. Fixes: 3bdae810721b ("brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain") Fixes: a413e39a38573 ("brcmfmac: fix brcmf_sdcard_recv_chain() for host without sg support") Signed-off-by: Peter S. Housel <housel@acm.org> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)