On Mon, 2025-01-27 at 17:26 +0100, Alexander Wetzel wrote:
> Virtual monitor interfaces should only be created when all of the
> following conditions are fulfilled:
> - All remaining enabled interfaces are monitor interface(s),
> - %IEEE80211_HW_NO_VIRTUAL_MONITOR isn't set by the driver and
> - %MONITOR_FLAG_ACTIVE and %MONITOR_FLAG_COOK_FRAMES isn't set on any
> of the enabled (monitor) interfaces.
>
> ieee80211_add_virtual_monitor() will then setup the virtual monitor
> interface for local->monitor_sdata and - when
> %IEEE80211_HW_WANT_MONITOR_VIF is set - inform the driver about the
> virtual monitor interface.
>
> But assuming %IEEE80211_HW_NO_VIRTUAL_MONITOR is not set, the current
> code still creates a virtual monitor interface when:
>
> 1) We have one non-monitor and one monitor interface with
> %MONITOR_FLAG_ACTIVE enabled and then delete the non-monitor
> interface.
That sounds like a bug, indeed. We shouldn't count monitors with ACTIVE
in local->monitors at all, I think.
> 2) We only have monitor interfaces enabled on resume while at least one
> has %MONITOR_FLAG_ACTIVE set.
Which would also address this, I'd guess?
> Fix the logic to follow the above stated rules and fixed/updated related
> checks not following the logic.
>
> Signed-off-by: Alexander Wetzel <Alexander@xxxxxxxxxxxxxx>
> ---
> I have problems understanding the current logic of how monitor
> interfaces are handled. For me it looks like we use local->monitors
> sometimes to count all monitor interfaces - except the ones using
> %MONITOR_FLAG_COOK_FRAMES - and at other times to count interfaces
> linked to the virtual monitor interface.
>
> This patch tries to fix the logic according to my understanding.
>
> Noteworthy here is the wording in 0d9c2beed116 ("wifi: mac80211:
> fix monitor channel with chanctx emulation"):
> ...
> Fix this by always allocating the virtual monitor sdata, and
> simply not telling the driver about it unless it wanted to.
> This way, we have an interface/sdata to bind the chanctx to,
> and the emulation can work correctly.
>
> But the commit with this statement is not changing the condition to call
> ieee80211_add_virtual_monitor(), contradicting these sentences.
>
> From ieee80211_do_open():
> case NL80211_IFTYPE_MONITOR:
> if (sdata->u.mntr.flags & MONITOR_FLAG_COOK_FRAMES) {
> local->cooked_mntrs++;
> break;
> }
>
> if (sdata->u.mntr.flags & MONITOR_FLAG_ACTIVE) {
> res = drv_add_interface(local, sdata);
> if (res)
> goto err_stop;
> } else if (local->monitors == 0 && local->open_count == 0) {
> res = ieee80211_add_virtual_monitor(local);
The "always" there perhaps should've said "whenever we have monitor
active", obviously we don't want a virtual monitor when there's no
monitor at all.
The "cooked" is completely different, and not really a monitor, it's
indeed used for ancient versions of hostapd that don't know nl80211 yet
and need to receive all frames not otherwise handled by the kernel via
monitor.
We can probably justify removing that, it was only used by hostapd
versions between April 2009 and December 2011, from
0915d02c3c08 ("wpa_supplicant AP: Add management frame RX for nl80211")
to
a11241fa1149 ("nl80211: Use nl80211 for mgmt TX/RX in AP mode")
The "active" monitor is something we still want, and indeed it doesn't
work for iwlwifi (which simply doesn't support it), but that should
indeed always be added to the driver - drivers that support it need the
vif so they have the MAC address to send ACKs for (to program it into
the device.)
> +++ b/net/mac80211/cfg.c
> @@ -4366,10 +4366,11 @@ static int ieee80211_cfg_get_channel(struct wiphy *wiphy,
> if (chanctx_conf) {
> *chandef = link->conf->chanreq.oper;
> ret = 0;
> - } else if (!ieee80211_hw_check(&local->hw, NO_VIRTUAL_MONITOR) &&
> - local->open_count > 0 &&
> - local->open_count == local->monitors &&
> - sdata->vif.type == NL80211_IFTYPE_MONITOR) {
> + } else if (local->virt_monitors &&
> + local->open_count == local->virt_monitors &&
> + sdata->vif.type == NL80211_IFTYPE_MONITOR &&
> + !(sdata->u.mntr.flags & (MONITOR_FLAG_ACTIVE |
> + MONITOR_FLAG_COOK_FRAMES))) {
> *chandef = local->monitor_chanreq.oper;
> ret = 0;
Not sure this is really all that relevant - if you have (only) active
monitors then it should still be correct, and having only cooked
monitors is pointless, not even sure it's supported at all. So I'm not
sure I see a need to change this?
Overall this seems needlessly complex.
I'd go for two changes:
* remove cooked monitor support entirely, unused since Dec 2011.
* not count active monitors in local->monitors, which should address
the other issues?
johannes
