* [PATCH V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain
@ 2017-06-12 10:46 Arend van Spriel
2017-06-12 11:32 ` Arend van Spriel
2017-06-13 7:00 ` [V3] " Kalle Valo
0 siblings, 2 replies; 8+ messages in thread
From: Arend van Spriel @ 2017-06-12 10:46 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, Peter S. Housel, Arend van Spriel
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")
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;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain
2017-06-12 10:46 [PATCH V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain Arend van Spriel
@ 2017-06-12 11:32 ` Arend van Spriel
2017-06-12 13:47 ` Kalle Valo
2017-06-13 7:00 ` [V3] " Kalle Valo
1 sibling, 1 reply; 8+ messages in thread
From: Arend van Spriel @ 2017-06-12 11:32 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, Peter S. Housel
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;
> }
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain
2017-06-12 11:32 ` Arend van Spriel
@ 2017-06-12 13:47 ` Kalle Valo
0 siblings, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2017-06-12 13:47 UTC (permalink / raw)
To: Arend van Spriel; +Cc: linux-wireless, Peter S. Housel
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.
--
Kalle Valo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain
2017-06-12 10:46 [PATCH V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain Arend van Spriel
2017-06-12 11:32 ` Arend van Spriel
@ 2017-06-13 7:00 ` Kalle Valo
2017-06-13 11:29 ` Arend van Spriel
1 sibling, 1 reply; 8+ messages in thread
From: Kalle Valo @ 2017-06-13 7:00 UTC (permalink / raw)
To: Arend Van Spriel; +Cc: linux-wireless, Peter S. Housel, Arend van Spriel
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
--
https://patchwork.kernel.org/patch/9780735/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain
2017-06-13 7:00 ` [V3] " Kalle Valo
@ 2017-06-13 11:29 ` Arend van Spriel
2017-06-14 9:21 ` Kalle Valo
0 siblings, 1 reply; 8+ messages in thread
From: Arend van Spriel @ 2017-06-13 11:29 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, Peter S. Housel
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain
2017-06-13 11:29 ` Arend van Spriel
@ 2017-06-14 9:21 ` Kalle Valo
2017-06-14 9:50 ` Arend van Spriel
0 siblings, 1 reply; 8+ messages in thread
From: Kalle Valo @ 2017-06-14 9:21 UTC (permalink / raw)
To: Arend van Spriel; +Cc: linux-wireless, Peter S. Housel
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.
--
Kalle Valo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain
2017-06-14 9:21 ` Kalle Valo
@ 2017-06-14 9:50 ` Arend van Spriel
2017-06-15 14:27 ` Kalle Valo
0 siblings, 1 reply; 8+ messages in thread
From: Arend van Spriel @ 2017-06-14 9:50 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, Peter S. Housel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain
2017-06-14 9:50 ` Arend van Spriel
@ 2017-06-15 14:27 ` Kalle Valo
0 siblings, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2017-06-15 14:27 UTC (permalink / raw)
To: Arend van Spriel; +Cc: linux-wireless, Peter S. Housel
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=E2=80=99m fine with this, or indeed most of the other proposed solution=
s.
> 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.
--=20
Kalle Valo
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-06-15 14:27 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-12 10:46 [PATCH V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain Arend van Spriel
2017-06-12 11:32 ` Arend van Spriel
2017-06-12 13:47 ` Kalle Valo
2017-06-13 7:00 ` [V3] " Kalle Valo
2017-06-13 11:29 ` Arend van Spriel
2017-06-14 9:21 ` Kalle Valo
2017-06-14 9:50 ` Arend van Spriel
2017-06-15 14:27 ` Kalle Valo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.