[PATCH] mac80211: fix channel type recalculation with HT and non-HT interfaces

[PATCH] mac80211: fix channel type recalculation with HT and non-HT interfaces

[Felix Fietkau]

When running an AP interface along with the cooked monitor interface created
by hostapd, adding an interface and deleting it again triggers a channel type
recalculation during which the (non-HT) monitor interface takes precedence
over the HT AP interface, thus causing the channel type to be set to non-HT.
Fix this by not overriding HT interfaces with a non-HT channel type.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 net/mac80211/chan.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 5b24740..8511e60 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -76,6 +76,7 @@ bool ieee80211_set_channel_type(struct ieee80211_local *local,
 
 		switch (tmp->vif.bss_conf.channel_type) {
 		case NL80211_CHAN_NO_HT:
+			break;
 		case NL80211_CHAN_HT20:
 			superchan = tmp->vif.bss_conf.channel_type;
 			break;
-- 
1.7.3.2

Re: [PATCH] mac80211: fix channel type recalculation with HT and non-HT interfaces

[Ben Greear]

On 03/11/2011 12:05 PM, Felix Fietkau wrote:
> When running an AP interface along with the cooked monitor interface created
> by hostapd, adding an interface and deleting it again triggers a channel type
> recalculation during which the (non-HT) monitor interface takes precedence
> over the HT AP interface, thus causing the channel type to be set to non-HT.
> Fix this by not overriding HT interfaces with a non-HT channel type.

After staring at that code a bit more, should that list-for-each-entry
loop be entirely different such that it better calculates a proper
super-channel for mixed interface types?

If seems like a bad case of last-one-wins as currently written.

The code probably (mostly?) works right because we limit the
bss_conf.channel_type based on existing super-chan when
adding new interfaces, but it still seems like brittle code to me.

Thanks,
Ben


>
> Signed-off-by: Felix Fietkau<nbd@openwrt.org>
> ---
>   net/mac80211/chan.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
> index 5b24740..8511e60 100644
> --- a/net/mac80211/chan.c
> +++ b/net/mac80211/chan.c
> @@ -76,6 +76,7 @@ bool ieee80211_set_channel_type(struct ieee80211_local *local,
>
>   		switch (tmp->vif.bss_conf.channel_type) {
>   		case NL80211_CHAN_NO_HT:
> +			break;
>   		case NL80211_CHAN_HT20:
>   			superchan = tmp->vif.bss_conf.channel_type;
>   			break;


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] mac80211: fix channel type recalculation with HT and non-HT interfaces

[Johannes Berg]

On Fri, 2011-03-11 at 12:17 -0800, Ben Greear wrote:
> On 03/11/2011 12:05 PM, Felix Fietkau wrote:
> > When running an AP interface along with the cooked monitor interface created
> > by hostapd, adding an interface and deleting it again triggers a channel type
> > recalculation during which the (non-HT) monitor interface takes precedence
> > over the HT AP interface, thus causing the channel type to be set to non-HT.
> > Fix this by not overriding HT interfaces with a non-HT channel type.

The fix seems right.

> After staring at that code a bit more, should that list-for-each-entry
> loop be entirely different such that it better calculates a proper
> super-channel for mixed interface types?
> 
> If seems like a bad case of last-one-wins as currently written.
> 
> The code probably (mostly?) works right because we limit the
> bss_conf.channel_type based on existing super-chan when
> adding new interfaces, but it still seems like brittle code to me.

But I'm not sure what you mean?

With the fix, we go through and use the widest possible channel of all
interfaces. The only conflict can happen with HT40- and HT40+, but we
should never get into that situation since we can't have one interface
with HT40- and another with HT40+ at the same time to start with. Am I
missing something?

johannes

Re: [PATCH] mac80211: fix channel type recalculation with HT and non-HT interfaces

[Ben Greear]

On 03/11/2011 12:24 PM, Johannes Berg wrote:
> On Fri, 2011-03-11 at 12:17 -0800, Ben Greear wrote:
>> On 03/11/2011 12:05 PM, Felix Fietkau wrote:
>>> When running an AP interface along with the cooked monitor interface created
>>> by hostapd, adding an interface and deleting it again triggers a channel type
>>> recalculation during which the (non-HT) monitor interface takes precedence
>>> over the HT AP interface, thus causing the channel type to be set to non-HT.
>>> Fix this by not overriding HT interfaces with a non-HT channel type.
>
> The fix seems right.
>
>> After staring at that code a bit more, should that list-for-each-entry
>> loop be entirely different such that it better calculates a proper
>> super-channel for mixed interface types?
>>
>> If seems like a bad case of last-one-wins as currently written.
>>
>> The code probably (mostly?) works right because we limit the
>> bss_conf.channel_type based on existing super-chan when
>> adding new interfaces, but it still seems like brittle code to me.
>
> But I'm not sure what you mean?
>
> With the fix, we go through and use the widest possible channel of all
> interfaces. The only conflict can happen with HT40- and HT40+, but we
> should never get into that situation since we can't have one interface
> with HT40- and another with HT40+ at the same time to start with. Am I
> missing something?

Maybe I'm missing something, but from what I can tell:

If we have two interfaces:

first interface is HT40, second is HT20:
   -> superchan == HT20

first interface is HT20, second is HT40:
   -> superchan == HT40

It seems wrong that the ordering would influence superchan before we go into
the "switch (superchan)" logic.

Thanks,
Ben

>
> johannes


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] mac80211: fix channel type recalculation with HT and non-HT interfaces

[Johannes Berg]

On Fri, 2011-03-11 at 12:28 -0800, Ben Greear wrote:

> > But I'm not sure what you mean?
> >
> > With the fix, we go through and use the widest possible channel of all
> > interfaces. The only conflict can happen with HT40- and HT40+, but we
> > should never get into that situation since we can't have one interface
> > with HT40- and another with HT40+ at the same time to start with. Am I
> > missing something?
> 
> Maybe I'm missing something, but from what I can tell:
> 
> If we have two interfaces:
> 
> first interface is HT40, second is HT20:
>    -> superchan == HT20
> 
> first interface is HT20, second is HT40:
>    -> superchan == HT40
> 
> It seems wrong that the ordering would influence superchan before we go into
> the "switch (superchan)" logic.

Ah, you're right -- the real fix isn't this patch, but probably
something like this:

...
case NL..._NO_HT:
case NL_...HT20:
	if (channel_type < super_chan)
		break;
	superchan = channel_type;
	break;
...

johannes