From: Peter S. Housel <hidden> Date: 2017-06-02 17:37:52
An earlier change to this function (3bdae810721b) fixed a leak in the
case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
glob_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.
Signed-off-by: Peter S. Housel <redacted>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
An earlier change to this function (3bdae810721b) fixed a leak in the
case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
glob_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.
That looks correct, could you add the relevant Fixes tag for this?
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")
BTW, you made the same typo that I did, it's actually glom_skb ;)
quoted hunk
Signed-off-by: Peter S. Housel <redacted>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
From: Franky Lin <hidden> Date: 2017-06-02 18:52:53
On Fri, Jun 2, 2017 at 10:22 AM, Peter S. Housel [off-list ref] wrote:
quoted hunk
An earlier change to this function (3bdae810721b) fixed a leak in the
case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
glob_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.
Signed-off-by: Peter S. Housel <redacted>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
@@ -727,15 +727,16 @@ 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);-gotodone;-}+if(err)+gotofree_glom_skb;skb_queue_walk(pktq,skb){memcpy(skb->data,glom_skb->data,skb->len);skb_pull(glom_skb,skb->len);}++free_glom_skb:+brcmu_pkt_buf_free_skb(glom_skb);
What about
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);
Then no goto is needed.
Thanks,
Franky
From: Peter S. Housel <hidden> Date: 2017-06-02 22:33:59
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 <redacted>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
From: Andy Shevchenko <hidden> Date: 2017-06-03 15:36:37
On Sat, Jun 3, 2017 at 1:29 AM, Peter S. Housel [off-list ref] 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.
What about
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);
Then no goto is needed.
For my point of view it has two subtle inconveniences:
1. Not so usual pattern in use if (!ret)
2. Less error prone in case someone decides to expand the code and
missed ! or something else there.
Since both makes an approach less error prone I wouldn't suggest doing
that as I commented in new version.
--
With Best Regards,
Andy Shevchenko
From: Arend van Spriel <arend.vanspriel@broadcom.com> Date: 2017-06-10 19:27:17
On 03-06-17 17:36, Andy Shevchenko wrote:
On Sat, Jun 3, 2017 at 1:29 AM, Peter S. Housel [off-list ref] wrote:
quoted
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.
Can we just add this one line instead or I'm missing something?
I guess. We don't want to walk the packet queue if glom_skb is not
carrying data due to brcmf_sdiod_buffrw() failure.
So I would go with the patch below as brcmu_pkt_buf_free_skb() simply
ignores null pointer.
Regards,
Arend
---