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

On 11-06-17 02:18, Peter Housel wrote:

quoted

On Jun 10, 2017, at 12:27 PM, Arend van Spriel [off-list ref] wrote:

On 03-06-17 17:36, Andy Shevchenko wrote:

quoted

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.

[snip]

quoted

quoted

+                       skb_queue_walk(pktq, skb) {
+                               memcpy(skb->data, glom_skb->data, skb->len);
+                               skb_pull(glom_skb, skb->len);
+                       }
               }

quoted

+               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