From: "Peter S. Housel" <[email protected]>
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 <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
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
On 13-06-17 09:00, Kalle Valo wrote:
> Arend Van Spriel <[email protected]> wrote:
>
>> From: "Peter S. Housel" <[email protected]>
>>
>> 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: [email protected] # 4.9.x-
>> Signed-off-by: Peter S. Housel <[email protected]>
>> Signed-off-by: Arend van Spriel <[email protected]>
>
> 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 <[email protected]> writes:
> On 13-06-17 09:00, Kalle Valo wrote:
>> Arend Van Spriel <[email protected]> wrote:
>>
>>> From: "Peter S. Housel" <[email protected]>
>>>
>>> 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: [email protected] # 4.9.x-
>>> Signed-off-by: Peter S. Housel <[email protected]>
>>> Signed-off-by: Arend van Spriel <[email protected]>
>>
>> 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
Arend van Spriel <[email protected]> writes:
> On 6/12/2017 12:46 PM, Arend van Spriel wrote:
>> From: "Peter S. Housel" <[email protected]>
>>
>> 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: [email protected] # 4.9.x-
Yes, will do.
--
Kalle Valo
On 6/12/2017 12:46 PM, Arend van Spriel wrote:
> From: "Peter S. Housel" <[email protected]>
>
> 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: [email protected] # 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 <[email protected]>
> Signed-off-by: Arend van Spriel <[email protected]>
> ---
> 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 <[email protected]> wrote:
> From: "Peter S. Housel" <[email protected]>
>
> 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: [email protected] # 4.9.x-
> Signed-off-by: Peter S. Housel <[email protected]>
> Signed-off-by: Arend van Spriel <[email protected]>
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
On 6/14/2017 11:21 AM, Kalle Valo wrote:
> Arend van Spriel <[email protected]> writes:
>
>> On 13-06-17 09:00, Kalle Valo wrote:
>>> Arend Van Spriel <[email protected]> wrote:
>>>
>>>> From: "Peter S. Housel" <[email protected]>
>>>>
>>>> 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: [email protected] # 4.9.x-
>>>> Signed-off-by: Peter S. Housel <[email protected]>
>>>> Signed-off-by: Arend van Spriel <[email protected]>
>>>
>>> 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 <[email protected]> writes:
> On 6/14/2017 11:21 AM, Kalle Valo wrote:
>> Arend van Spriel <[email protected]> writes:
>>
>>> On 13-06-17 09:00, Kalle Valo wrote:
>>>> Arend Van Spriel <[email protected]> wrote:
>>>>
>>>>> From: "Peter S. Housel" <[email protected]>
>>>>>
>>>>> 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: [email protected] # 4.9.x-
>>>>> Signed-off-by: Peter S. Housel <[email protected]>
>>>>> Signed-off-by: Arend van Spriel <[email protected]>
>>>>
>>>> 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