Hi Arend,
------- Original Message -------
On Monday, July 4th, 2022 at 11:43, Arend Van Spriel <aspriel@xxxxxxxxx> wrote:
> [..]
>
> It is good practice to throw in a changelog here so people know what has
> changed since earlier version of the patch.
That's fair enough. The commit message is updated.
Changes compared to v3:
- brcmf_sdiod_remove(..) disables functions in reverse order. It also claims
'func2' when disabling 'func2'. However, operations on 'func2' are always
performed after claiming 'func1'. So this corrects mistake that deviates
from convention.
- furthermore, following feedback from the kernel, irq is released for each
individual function, but only func1 performs removal operations. This
prevents the ordering issue from occurring.
> ---
>
> > .../broadcom/brcm80211/brcmfmac/bcmsdh.c | 31 ++++++++++++-------
> > .../broadcom/brcm80211/brcmfmac/sdio.c | 10 +-----
> > 2 files changed, 21 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> > index ac02244a6fdf..dd634edaa0b3 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
>
>
> [...]
>
> > @@ -1096,12 +1093,24 @@ static void brcmf_ops_sdio_remove(struct sdio_func *func)
> > if (bus_if) {
> > sdiodev = bus_if->bus_priv.sdio;
> >
> > + if (func->num != 1) {
> > + /* Satisfy kernel expectation that the irq is released once the
> > + * '.remove' callback has executed, while respecting the design
> > + * that removal is executed for 'sdiodev', instead of individual
> > + * function.
> > + /
> > + brcmf_dbg(SDIO, "Only release irq for function %d", func->num);
> > + sdio_claim_host(sdiodev->func1);
> > + sdio_release_irq(func);
> > + sdio_release_host(sdiodev->func1);
> > + return;
> > + }
> > +
> > + / func 1: so do full clean-up and removal */
> > +
>
>
> The problem is that upon driver unload we get remove for function 2 and
> then for function 1. Upon mmc_hw_reset() we get a remove for function 1
> and then for function 2. So in the scenario of mmc_hw_reset() we free
> sdiodev upon func 1 removal and then for func 2 removal we have a
> use-after-free of sdiodev.
I understood this. I recognize the different orders. However, there is a
false assumption regarding double-freeing. The removal logic in
'brcmf_ops_sdio_remove' is conditional on function number. Little is done
for any function that is not `func->num == 1`. The proposed patch V4 fine-
tunes this behavior slightly. In this fine-tuning it mostly (completely)
negates order differences.
> The code currently relies on the order in
> which remove callback is done. To make it more robust we could throw in
> a refcount for sdiodev and only do the full clean-up when refcount hits
> zero.
Am I missing something else, maybe? If not, I think I have your concerns
covered.
Note that I have (and am) testing this patch on my own system. So far
without issue. I have run a script that resets the mmc device every 90
seconds to see if repeated use would cause issues. So far without issues.
Regards,
Danny
