Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1662998 > unrolled thread
| Started by | Arend van Spriel <arend.vanspriel@broadcom.com> |
|---|---|
| First post | 2017-06-10 21:30 +0200 |
| Last post | 2017-06-11 16:00 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v2] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain Arend van Spriel <arend.vanspriel@broadcom.com> - 2017-06-10 21:30 +0200
Re: [PATCH v2] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain Arend van Spriel <arend.vanspriel@broadcom.com> - 2017-06-11 10:10 +0200
Re: [PATCH v2] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-11 16:00 +0200
| From | Arend van Spriel <arend.vanspriel@broadcom.com> |
|---|---|
| Date | 2017-06-10 21:30 +0200 |
| Subject | Re: [PATCH v2] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain |
| Message-ID | <tQUf7-31I-11@gated-at.bofh.it> |
On 03-06-17 17:36, Andy Shevchenko wrote:
> 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.
[snip]
>> + 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?
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
---
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 5bc2ba2..3722f23 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -705,7 +705,7 @@ int brcmf_sdiod_recv_pkt(struct brcmf_sdio_dev
*sdiodev, struct sk_buff *pkt)
int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev,
struct sk_buff_head *pktq, uint totlen)
{
- struct sk_buff *glom_skb;
+ struct sk_buff *glom_skb = NULL;
struct sk_buff *skb;
u32 addr = sdiodev->sbwad;
int err = 0;
@@ -726,10 +726,8 @@ 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);
+ if (err)
goto done;
- }
skb_queue_walk(pktq, skb) {
memcpy(skb->data, glom_skb->data, skb->len);
@@ -740,6 +738,7 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev
*sdiodev,
pktq);
done:
+ brcmu_pkt_buf_free_skb(glom_skb);
return err;
}
[toc] | [next] | [standalone]
| From | Arend van Spriel <arend.vanspriel@broadcom.com> |
|---|---|
| Date | 2017-06-11 10:10 +0200 |
| Message-ID | <tR66B-2DE-3@gated-at.bofh.it> |
| In reply to | #1662998 |
On 11-06-17 02:18, Peter Housel wrote:
>
>> On Jun 10, 2017, at 12:27 PM, Arend van Spriel <arend.vanspriel@broadcom.com> wrote:
>>
>> On 03-06-17 17:36, Andy Shevchenko wrote:
>>> 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.
>>
>> [snip]
>>
>>>> + 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?
>>
>> 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.
>
> I’m fine with this, or indeed most of the other proposed solutions. The important thing is that the leak is fixed; in the driver's current state I was able to run our wearable device out of memory in just over 20 seconds running iperf.
Sure. The reason behind the suggestion from Franky was to get rid of the
label inside branch and I agree with that. To address Andy's comment I
think my proposal should tackle that.
Just out of curiosity, we added the broken-sg-support thing for OMAP
platform. So what platform/mmc-host are you using. I try to keep an
overview where this workaround is needed.
Regards,
Arend
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-06-11 16:00 +0200 |
| Message-ID | <tRbzj-5Kd-3@gated-at.bofh.it> |
| In reply to | #1662998 |
On Sat, Jun 10, 2017 at 10:27 PM, Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 03-06-17 17:36, Andy Shevchenko wrote:
>> On Sat, Jun 3, 2017 at 1:29 AM, Peter S. Housel <housel@acm.org> wrote:
The following looks good to me.
Feel free to add
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -705,7 +705,7 @@ int brcmf_sdiod_recv_pkt(struct brcmf_sdio_dev
> *sdiodev, struct sk_buff *pkt)
> int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev,
> struct sk_buff_head *pktq, uint totlen)
> {
> - struct sk_buff *glom_skb;
> + struct sk_buff *glom_skb = NULL;
> struct sk_buff *skb;
> u32 addr = sdiodev->sbwad;
> int err = 0;
> @@ -726,10 +726,8 @@ 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);
> + if (err)
> goto done;
> - }
>
> skb_queue_walk(pktq, skb) {
> memcpy(skb->data, glom_skb->data, skb->len);
> @@ -740,6 +738,7 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev
> *sdiodev,
> pktq);
>
> done:
> + brcmu_pkt_buf_free_skb(glom_skb);
> return err;
> }
>
--
With Best Regards,
Andy Shevchenko
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web