| Message ID | 20190409114333.24342-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a927e8d8ab57e696800e20cf09a72b7dfe3bbebb |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [V2] brcmfmac: fix leak of mypkt on error return path | expand |
On 4/9/2019 1:43 PM, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Currently if the call to brcmf_sdiod_set_backplane_window fails then > then error return path leaks mypkt. Fix this by returning by a new > error path labelled 'out' that calls brcmu_pkt_buf_free_skb to free > mypkt. Also remove redundant check on err before calling > brcmf_sdiod_skbuff_write. > > Addresses-Coverity: ("Resource Leak") > Fixes: a7c3aa1509e2 ("brcmfmac: Remove brcmf_sdiod_addrprep()") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > > V2: Remove redundant check on err before calling > brcmf_sdiod_skbuff_write, kudos to Dan Carpenter and Arend Van Spriel > for spotting this. > > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > index ec129864cc9c..60aede5abb4d 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > @@ -628,15 +628,13 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 *buf, uint nbytes) > > err = brcmf_sdiod_set_backplane_window(sdiodev, addr); > if (err) > - return err; > + goto out; > > addr &= SBSDIO_SB_OFT_ADDR_MASK; > addr |= SBSDIO_SB_ACCESS_2_4B_FLAG; > > - if (!err) > - err = brcmf_sdiod_skbuff_write(sdiodev, sdiodev->func2, addr, > - mypkt); > - > + err = brcmf_sdiod_skbuff_write(sdiodev, sdiodev->func2, addr, mypkt); > +out: I am fine with it, but my suggestion was a bit different: err = brcmf_sdiod_set_backplane_window(sdiodev, addr); if (!err) { addr &= SBSDIO_SB_OFT_ADDR_MASK; addr |= SBSDIO_SB_ACCESS_2_4B_FLAG; err = brcmf_sdiod_skbuff_write(sdiodev, sdiodev->func2, addr, mypkt); } > brcmu_pkt_buf_free_skb(mypkt); > > return err; >
On Tue, Apr 09, 2019 at 01:50:38PM +0200, Arend Van Spriel wrote: > On 4/9/2019 1:43 PM, Colin King wrote: > > From: Colin Ian King <colin.king@canonical.com> > > > > Currently if the call to brcmf_sdiod_set_backplane_window fails then > > then error return path leaks mypkt. Fix this by returning by a new > > error path labelled 'out' that calls brcmu_pkt_buf_free_skb to free > > mypkt. Also remove redundant check on err before calling > > brcmf_sdiod_skbuff_write. > > > > Addresses-Coverity: ("Resource Leak") > > Fixes: a7c3aa1509e2 ("brcmfmac: Remove brcmf_sdiod_addrprep()") > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > --- > > > > V2: Remove redundant check on err before calling > > brcmf_sdiod_skbuff_write, kudos to Dan Carpenter and Arend Van Spriel > > for spotting this. > > > > --- > > drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > > index ec129864cc9c..60aede5abb4d 100644 > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > > @@ -628,15 +628,13 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 *buf, uint nbytes) > > err = brcmf_sdiod_set_backplane_window(sdiodev, addr); > > if (err) > > - return err; > > + goto out; > > addr &= SBSDIO_SB_OFT_ADDR_MASK; > > addr |= SBSDIO_SB_ACCESS_2_4B_FLAG; > > - if (!err) > > - err = brcmf_sdiod_skbuff_write(sdiodev, sdiodev->func2, addr, > > - mypkt); > > - > > + err = brcmf_sdiod_skbuff_write(sdiodev, sdiodev->func2, addr, mypkt); > > +out: > > I am fine with it, but my suggestion was a bit different: > > err = brcmf_sdiod_set_backplane_window(sdiodev, addr); > if (!err) { > addr &= SBSDIO_SB_OFT_ADDR_MASK; > addr |= SBSDIO_SB_ACCESS_2_4B_FLAG; > err = brcmf_sdiod_skbuff_write(sdiodev, sdiodev->func2, > addr, mypkt); > } Success handling always leads to extra indenting like this... It's less confusing to do failure handling keep the normal/success path at indent level 1. regards, dan carpenter
On 09.04.2019 14:43, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Currently if the call to brcmf_sdiod_set_backplane_window fails then > then error return path leaks mypkt. Fix this by returning by a new One "then" is enough. :-) > error path labelled 'out' that calls brcmu_pkt_buf_free_skb to free > mypkt. Also remove redundant check on err before calling > brcmf_sdiod_skbuff_write. > > Addresses-Coverity: ("Resource Leak") > Fixes: a7c3aa1509e2 ("brcmfmac: Remove brcmf_sdiod_addrprep()") > Signed-off-by: Colin Ian King <colin.king@canonical.com> [...] MBR, Sergei
On 4/9/2019 5:13 PM, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Currently if the call to brcmf_sdiod_set_backplane_window fails then Fix this one extra then pointed by Sergie. otherwise looks good to me. Reviewed-by: Mukesh Ojha <mojha@codeaurora.org> Cheers, -Mukesh > then error return path leaks mypkt. Fix this by returning by a new > error path labelled 'out' that calls brcmu_pkt_buf_free_skb to free > mypkt. Also remove redundant check on err before calling > brcmf_sdiod_skbuff_write. > > Addresses-Coverity: ("Resource Leak") > Fixes: a7c3aa1509e2 ("brcmfmac: Remove brcmf_sdiod_addrprep()") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > > V2: Remove redundant check on err before calling > brcmf_sdiod_skbuff_write, kudos to Dan Carpenter and Arend Van Spriel > for spotting this. > > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > index ec129864cc9c..60aede5abb4d 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > @@ -628,15 +628,13 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 *buf, uint nbytes) > > err = brcmf_sdiod_set_backplane_window(sdiodev, addr); > if (err) > - return err; > + goto out; > > addr &= SBSDIO_SB_OFT_ADDR_MASK; > addr |= SBSDIO_SB_ACCESS_2_4B_FLAG; > > - if (!err) > - err = brcmf_sdiod_skbuff_write(sdiodev, sdiodev->func2, addr, > - mypkt); > - > + err = brcmf_sdiod_skbuff_write(sdiodev, sdiodev->func2, addr, mypkt); > +out: > brcmu_pkt_buf_free_skb(mypkt); > > return err;
Colin King <colin.king@canonical.com> wrote: > From: Colin Ian King <colin.king@canonical.com> > > Currently if the call to brcmf_sdiod_set_backplane_window fails then > error return path leaks mypkt. Fix this by returning by a new > error path labelled 'out' that calls brcmu_pkt_buf_free_skb to free > mypkt. Also remove redundant check on err before calling > brcmf_sdiod_skbuff_write. > > Addresses-Coverity: ("Resource Leak") > Fixes: a7c3aa1509e2 ("brcmfmac: Remove brcmf_sdiod_addrprep()") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > Reviewed-by: Mukesh Ojha <mojha@codeaurora.org> Patch applied to wireless-drivers-next.git, thanks. a927e8d8ab57 brcmfmac: fix leak of mypkt on error return path
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index ec129864cc9c..60aede5abb4d 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -628,15 +628,13 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 *buf, uint nbytes) err = brcmf_sdiod_set_backplane_window(sdiodev, addr); if (err) - return err; + goto out; addr &= SBSDIO_SB_OFT_ADDR_MASK; addr |= SBSDIO_SB_ACCESS_2_4B_FLAG; - if (!err) - err = brcmf_sdiod_skbuff_write(sdiodev, sdiodev->func2, addr, - mypkt); - + err = brcmf_sdiod_skbuff_write(sdiodev, sdiodev->func2, addr, mypkt); +out: brcmu_pkt_buf_free_skb(mypkt); return err;