On Thu, Jun 29, 2023 at 11:31:33AM +0200, Martin Pieuchot wrote:
> On 29/06/23(Thu) 11:17, Stefan Sperling wrote:
> >
> > iwm_intr already runs at IPL_NET. What else would be required?
>
> Are we sure all accesses to `ic_tree' are run under KERNEL_LOCK()+splnet()?
A quick look through net80211 doesn't suggest any problem here.
I assume the KERNEL_LOCK is implied since interrupt handlers in wireless
drivers should not be marked MPSAFE. Userland has an ioctl to query the
nodes tree but cannot modify it. New nodes are only added during scans n
interrupt context under SPLNET. (Ignoring the timeouts in hostap mode for
now, since this crash is in iwm which does not support hostap mode).
Nodes can be removed in ieee80211_free_allnodes() via iwm_newstate_task()
and sc->sc_newstate, which runs in a dedicated task queue which is not
marked MPSAFE and thus runs with the kernel lock held.
ieee80211_free_allnodes() raises to IPL_NET before modyfing the tree.
While looking at this I spotted an unrelated problem: iwm/iwx add and delete
bgscan_done_task via different task queues. This means bgscan_done_task will
not be cancelled properly in iwm_newstate(). This bug originated in a commit
I made in on December 2 2021 and hasn't been noticed yet.
I would just move this task to the systq, where it gets deleted from. ok?
diff /usr/src
commit - 0c5a9349528207d3a937cab66be92baf3c29da40
path + /usr/src
blob - 1547120ea8f2bc861af585999863012e6ce6655c
file + sys/dev/pci/if_iwm.c
--- sys/dev/pci/if_iwm.c
+++ sys/dev/pci/if_iwm.c
@@ -8574,7 +8574,7 @@ iwm_bgscan_done(struct ieee80211com *ic,
free(sc->bgscan_unref_arg, M_DEVBUF, sc->bgscan_unref_arg_size);
sc->bgscan_unref_arg = arg;
sc->bgscan_unref_arg_size = arg_size;
- iwm_add_task(sc, sc->sc_nswq, &sc->bgscan_done_task);
+ iwm_add_task(sc, systq, &sc->bgscan_done_task);
}
void
blob - 8aa8740bcf864ee470b7284da02d0c862e2bee99
file + sys/dev/pci/if_iwx.c
--- sys/dev/pci/if_iwx.c
+++ sys/dev/pci/if_iwx.c
@@ -7607,7 +7607,7 @@ iwx_bgscan_done(struct ieee80211com *ic,
free(sc->bgscan_unref_arg, M_DEVBUF, sc->bgscan_unref_arg_size);
sc->bgscan_unref_arg = arg;
sc->bgscan_unref_arg_size = arg_size;
- iwx_add_task(sc, sc->sc_nswq, &sc->bgscan_done_task);
+ iwx_add_task(sc, systq, &sc->bgscan_done_task);
}
void
