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

[Johannes Berg <johannes@sipsolutions.net>]

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?

johannes