On 7/4/2022 4:49 PM, Danny van Heumen wrote:
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;
Actually this is wrong. Before the function
brcmf_sdiod_intr_unregister() was called for every sdio function
instance. That function does exactly the same as the above and more. On
some platforms the device does not used the SDIO interrupt, but instead
it uses what we call an OOB interrupt (out-of-band). So your change does
not add anything for devices/platforms employing the SDIO interrupt, but
it does break those using OOB interrupt.
+ }
+
+ / 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.
I think you are. The function brcmf_sdiod_remove() does end-up freeing
the sdiodev instance. brcmf_sdiod_remove() for func 1, but in
brcmf_ops_sdio_remove() we do dereference sdiodev instance for all
functions.
In the portion above you have:
sdio_claim_host(sdiodev->func1);
When brcmf_ops_sdio_remove is called for func 2 (and even func 3) after
func 1 has been removed sdiodev points to memory already free.
Regards,
Arend
