On Sun, Dec 05, 2021 at 06:21:50PM +0100, Stefan Sperling wrote:
> On Sun, Dec 05, 2021 at 11:05:32AM -0600, Scott Cheloha wrote:
> > > diff 0b61c8235787960f0010ef627ea5b2c6309a81f0
> > > de98c050ea709bdb8e26be40ab0cc82ef9afed80
> > > blob - 7bb68194dd78417b06c59f81d1ebbff4165203d8
> > > blob + 5b9a969258074fde29e21a33ac035cf170ec3b03
> > > --- sys/net80211/ieee80211.c
> > > +++ sys/net80211/ieee80211.c
> > > @@ -193,6 +193,7 @@ ieee80211_ifattach(struct ifnet *ifp)
> > > if_addgroup(ifp, "wlan");
> > > ifp->if_priority = IF_WIRELESS_DEFAULT_PRIORITY;
> > >
> > > + task_set(&ic->ic_rtm_80211info_task, ieee80211_rtm_80211info_task, ic);
> > > ieee80211_set_link_state(ic, LINK_STATE_DOWN);
> > >
> > > timeout_set(&ic->ic_bgscan_timeout, ieee80211_bgscan_timeout, ifp);
> > > @@ -203,6 +204,7 @@ ieee80211_ifdetach(struct ifnet *ifp)
> > > {
> > > struct ieee80211com *ic = (void *)ifp;
> > >
> > > + task_del(systq, &ic->ic_rtm_80211info_task);
> > > timeout_del(&ic->ic_bgscan_timeout);
> >
> > Suppose the ic_bgscan_timeout timeout is running at the moment we're
> > running ieee80211_ifdetach(). Ignore the kernel lock for the moment,
> > think about the future.
>
> There are many more places in the wireless stack that do such things.
> But I am not interested in doing MP work on wireless myself. That is
> simply asking too much on top of everything else I do.
>
> > If we delete the task before we delete the timeout and the timeout
> > then adds the task back onto the task queue, what happens?
> >
> > My guess is you need to ensure the timeout is no longer running
> > *before* you delete the task. Can you do timeout_del_barrier()
> > here? See the attached patch.
>
> Yes, sure we can. There are dozens of other timeouts in the wireless
> stack and drivers, so your patch is a small step on a very long road.
It probably isn't even the right patch.
I don't know the relationship between ieee80211_rx_ba and the
interface.
> > > /*
> > > blob - 447a2676bfb250b7f917206549679d6ae68de1f6
> > > blob + 7e10fc1336067542c13d5607602e658ce2b3926b
> > > --- sys/net80211/ieee80211_proto.c
> > > +++ sys/net80211/ieee80211_proto.c
> > > @@ -1288,6 +1288,31 @@ justcleanup:
> > > }
> > >
> > > void
> > > +ieee80211_rtm_80211info_task(void *arg)
> > > +{
> > > + struct ieee80211com *ic = arg;
> > > + struct ifnet *ifp = &ic->ic_if;
> > > + struct if_ieee80211_data ifie;
> > > + int s = splnet();
> > > +
> > > + if (LINK_STATE_IS_UP(ifp->if_link_state)) {
> >
> > Does this mean userspace can "miss" state transitions if the task runs
> > and the state has already changed back to not-up?
> >
> > I don't know whether this would matter in practice, but it would be a
> > behavior change.
>
> If this task doesn't find link state up for some reason then it is
> running in an unexpected situation. What else should it do?
Probably nothing, just doing due-diligence.
> I think it makes sense for a scheduled task to check that its precondition
> still applies. We have had several bugs in iwm(4) where tasks did their
> thing even though their reason for being scheduled no longer applied.
> In that case we ended up sending firmware commands that appeared our
> of order from the device's point of view. I don't know if this really
> matters here, it will depend on what userland expects.
This sounds fine to me.
> > Unsure how `route monitor` exercises this path, but I've left it
> > running, too.
>
> That was just to see whether the routing message is still being generated.
Gotcha.
