Re: [PATCH] wifi: brcmfmac: Fix memory leak in brcmf_sdio_read_control()

On 03/08/2026 10:35, Johannes Berg wrote:
> On Mon, 2026-08-03 at 10:21 +0200, Arend van Spriel wrote:
>>>
>>> I looked at moving the vfree() to the done: label, but the code also
>>> gets there in the success path to wakeup waiters. In the success path
>>> the buf is stored for further processing so it should not be freed.
> 
> Hah, my bad, sorry!
> 
>>> Maybe better to do:
>>>
>>>          spin_unlock_bh(&bus->rxctl_lock);
>>>          brcmf_sdio_dcmd_resp_wake(bus);
>>>          return;
>>>
>>> fail:
>>>          vfree(buf);
>>>          /* Awake any waiters */
>>>          brcmf_sdio_dcmd_resp_wake(bus);
>>> }
>> Actually it turns out brcmf_sdio_read_control() is only called once so
>> the brcmf_sdio_dcmd_resp_wake() can be taken outside the function.
> 
> Or just something like this?
> 
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -1827,15 +1827,17 @@ brcmf_sdio_read_control(struct brcmf_sdio *bus, u8 *hdr, uint len, uint doff)
>          if (bus->rxctl) {
>                  brcmf_err("last control frame is being processed.\n");
>                  spin_unlock_bh(&bus->rxctl_lock);
> -               vfree(buf);
>                  goto done;
>          }
>          bus->rxctl = buf + doff;
>          bus->rxctl_orig = buf;
>          bus->rxlen = len - doff;
>          spin_unlock_bh(&bus->rxctl_lock);
> +       /* buffer is queued */
> +       buf = NULL;
>   
>   done:
> +       vfree(buf);
>          /* Awake any waiters */
>          brcmf_sdio_dcmd_resp_wake(bus);
>   }
> 
> 
> It's already using vmalloc() so seems it wouldn't care that much about
> calling vfree(NULL) on the success path?
True. And yes, the existing vfree() should go, but I figured that goes 
without saying so I did not mention that. I am fine with your proposal, 
but I guess the author will have to submit a v2 patch, right?

Gr. AvS