[PATCH] brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain

[PATCH] brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain

[Peter S. Housel]

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 <housel-HInyCGIudOg@public.gmane.org>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 9b970dc..4c5064f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -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);
-			goto done;
-		}
+		if (err)
+			goto free_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);
 	} else
 		err = brcmf_sdiod_sglist_rw(sdiodev, SDIO_FUNC_2, false, addr,
 					    pktq);
-- 
2.7.4

Re: [PATCH] brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain

[Florian Fainelli]

On 06/02/2017 10:22 AM, Peter S. Housel wrote:
> 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 ;)

> 
> Signed-off-by: Peter S. Housel <housel@acm.org>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index 9b970dc..4c5064f 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -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);
> -			goto done;
> -		}
> +		if (err)
> +			goto free_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);
>  	} else
>  		err = brcmf_sdiod_sglist_rw(sdiodev, SDIO_FUNC_2, false, addr,
>  					    pktq);
> 


-- 
Florian

Re: [PATCH] brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain

[Franky Lin]

On Fri, Jun 2, 2017 at 10:22 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
> 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 <housel@acm.org>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index 9b970dc..4c5064f 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -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);
> -                       goto done;
> -               }
> +               if (err)
> +                       goto free_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

>         } else
>                 err = brcmf_sdiod_sglist_rw(sdiodev, SDIO_FUNC_2, false, addr,
>                                             pktq);
> --
> 2.7.4
>

[PATCH v2] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Peter S. Housel]

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(-)

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);
-- 
2.7.4

Re: [PATCH v2] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Andy Shevchenko]

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);



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Arend van Spriel]

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;
 }

Re: [PATCH v2] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Arend van Spriel]

On 11-06-17 02:18, Peter Housel wrote:
> 
>> On Jun 10, 2017, at 12:27 PM, Arend van Spriel <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>
>> On 03-06-17 17:36, Andy Shevchenko wrote:
>>> On Sat, Jun 3, 2017 at 1:29 AM, Peter S. Housel <housel-HInyCGIudOg@public.gmane.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

Re: [PATCH v2] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Andy Shevchenko]

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

Re: [PATCH] brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain

[Andy Shevchenko]

On Fri, Jun 2, 2017 at 9:52 PM, Franky Lin <franky.lin@broadcom.com> wrote:
> On Fri, Jun 2, 2017 at 10:22 AM, Peter S. Housel <housel@acm.org> wrote:

>>                 err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, false, addr,
>>                                          glom_skb);
>> -               if (err) {
>> -                       brcmu_pkt_buf_free_skb(glom_skb);
>> -                       goto done;
>> -               }

> 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