| Message ID | 1497264382-2290-1-git-send-email-arend.vanspriel@broadcom.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5ea59db8a375216e6c915c5586f556766673b5a7 |
| Delegated to: | Kalle Valo |
| Headers | show |
On 6/12/2017 12:46 PM, Arend van Spriel wrote: > From: "Peter S. Housel" <housel@acm.org> > > 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. > Kalle, Can you add stable tag: Cc: stable@vger.kernel.org # 4.9.x- > 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> > Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> > --- > changes: > V2: > - avoid goto using if (!err) {}. > V3: > - free glom_skb at done label. > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 7 +++---- > 1 file changed, 3 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..844c1e6 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > @@ -706,7 +706,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; > @@ -727,10 +727,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); > @@ -741,6 +739,7 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev, > pktq); > > done: > + brcmu_pkt_buf_free_skb(glom_skb); > return err; > } > >
Arend van Spriel <arend.vanspriel@broadcom.com> writes: > On 6/12/2017 12:46 PM, Arend van Spriel wrote: >> From: "Peter S. Housel" <housel@acm.org> >> >> 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. >> > > Kalle, > > Can you add stable tag: > > Cc: stable@vger.kernel.org # 4.9.x- Yes, will do.
Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: > From: "Peter S. Housel" <housel@acm.org> > > 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") > Cc: stable@vger.kernel.org # 4.9.x- > Signed-off-by: Peter S. Housel <housel@acm.org> > Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> Patch applied to wireless-drivers-next.git, thanks. 5ea59db8a375 brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain
On 13-06-17 09:00, Kalle Valo wrote: > Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: > >> From: "Peter S. Housel" <housel@acm.org> >> >> 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") >> Cc: stable@vger.kernel.org # 4.9.x- >> Signed-off-by: Peter S. Housel <housel@acm.org> >> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> > > Patch applied to wireless-drivers-next.git, thanks. Yikes. You say wireless-drivers-next? I should have tagged it better, but I would like to get this fix in 4.12 and stable. Regards, Arend > 5ea59db8a375 brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain >
Arend van Spriel <arend.vanspriel@broadcom.com> writes: > On 13-06-17 09:00, Kalle Valo wrote: >> Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: >> >>> From: "Peter S. Housel" <housel@acm.org> >>> >>> 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") >>> Cc: stable@vger.kernel.org # 4.9.x- >>> Signed-off-by: Peter S. Housel <housel@acm.org> >>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> >> >> Patch applied to wireless-drivers-next.git, thanks. > > Yikes. You say wireless-drivers-next? I should have tagged it better, > but I would like to get this fix in 4.12 and stable. Yes, always document clearly your intentions. I have so many patches (and emails) to go through that I do not have much time for each patch to figure out which tree it should go. And in this case the commit log didn't mention any major breakage so I assumed this is for -next. In theory I could cherry-pick the commit to wireless-drivers, but as this doesn't look like a serious issue (no crashes or anything like that), is it enough that this goes to 4.12 via stable tree? Just takes a little longer, nothing else.
On 6/14/2017 11:21 AM, Kalle Valo wrote: > Arend van Spriel <arend.vanspriel@broadcom.com> writes: > >> On 13-06-17 09:00, Kalle Valo wrote: >>> Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: >>> >>>> From: "Peter S. Housel" <housel@acm.org> >>>> >>>> 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") >>>> Cc: stable@vger.kernel.org # 4.9.x- >>>> Signed-off-by: Peter S. Housel <housel@acm.org> >>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> >>> >>> Patch applied to wireless-drivers-next.git, thanks. >> >> Yikes. You say wireless-drivers-next? I should have tagged it better, >> but I would like to get this fix in 4.12 and stable. > > Yes, always document clearly your intentions. I have so many patches > (and emails) to go through that I do not have much time for each patch > to figure out which tree it should go. And in this case the commit log > didn't mention any major breakage so I assumed this is for -next. > > In theory I could cherry-pick the commit to wireless-drivers, but as > this doesn't look like a serious issue (no crashes or anything like > that), is it enough that this goes to 4.12 via stable tree? Just takes a > little longer, nothing else. It is for you to decide. This is what Peter wrote: """ 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. """ Regards, Arend
Arend van Spriel <arend.vanspriel@broadcom.com> writes: > On 6/14/2017 11:21 AM, Kalle Valo wrote: >> Arend van Spriel <arend.vanspriel@broadcom.com> writes: >> >>> On 13-06-17 09:00, Kalle Valo wrote: >>>> Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: >>>> >>>>> From: "Peter S. Housel" <housel@acm.org> >>>>> >>>>> 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") >>>>> Cc: stable@vger.kernel.org # 4.9.x- >>>>> Signed-off-by: Peter S. Housel <housel@acm.org> >>>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> >>>> >>>> Patch applied to wireless-drivers-next.git, thanks. >>> >>> Yikes. You say wireless-drivers-next? I should have tagged it better, >>> but I would like to get this fix in 4.12 and stable. >> >> Yes, always document clearly your intentions. I have so many patches >> (and emails) to go through that I do not have much time for each patch >> to figure out which tree it should go. And in this case the commit log >> didn't mention any major breakage so I assumed this is for -next. >> >> In theory I could cherry-pick the commit to wireless-drivers, but as >> this doesn't look like a serious issue (no crashes or anything like >> that), is it enough that this goes to 4.12 via stable tree? Just takes a >> little longer, nothing else. > > It is for you to decide. This is what Peter wrote: > > """ > 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. > """ Ok, if there's just one report, and even that on a special device, having the fix go via the stable tree should be fine.
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index 9b970dc..844c1e6 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -706,7 +706,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; @@ -727,10 +727,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); @@ -741,6 +739,7 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev, pktq); done: + brcmu_pkt_buf_free_skb(glom_skb); return err; }