diff mbox series

wifi: nl80211: allow MBSSID Tx VAP bringup without MBSSID IEs

Message ID 20240621074038.3938005-1-quic_ssreeela@quicinc.com (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series wifi: nl80211: allow MBSSID Tx VAP bringup without MBSSID IEs | expand

Commit Message

Sowmiya Sree Elavalagan June 21, 2024, 7:40 a.m. UTC 
From: Rameshkumar Sundaram <quic_ramess@quicinc.com>

Current implementation of MBSSID configuration parsing mandates
MBSSID elements for Tx BSS (index 0). However with ML link addition
it is possible that Non-Tx BSS'es can be added at a later point in
time after Tx BSS is brought up. Hence allow bring up of MBSSID Tx
BSS even if no Non-Tx BSS are present at that time. Later when new
Non-TX BSS are added TX BSS beacon can be updated with MBSSID IEs.

Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
Signed-off-by: Sowmiya Sree Elavalagan <quic_ssreeela@quicinc.com>
---
 net/wireless/nl80211.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Johannes Berg June 26, 2024, 12:15 p.m. UTC | #1 
On Fri, 2024-06-21 at 13:10 +0530, Sowmiya Sree Elavalagan wrote:
> From: Rameshkumar Sundaram <quic_ramess@quicinc.com>
> 
> Current implementation of MBSSID configuration parsing mandates
> MBSSID elements for Tx BSS (index 0). However with ML link addition
> it is possible that Non-Tx BSS'es can be added at a later point in
> time after Tx BSS is brought up. Hence allow bring up of MBSSID Tx
> BSS even if no Non-Tx BSS are present at that time. Later when new
> Non-TX BSS are added TX BSS beacon can be updated with MBSSID IEs.

nit: I tend to think we should mostly use "element" instead of "IE"
since the spec changed (subject and text), except where historically we
have variable names etc.

I'm also not convinced this actually works without further changes down
the stack? Think ath11k/mac80211 for example, where
ieee80211_beacon_get_template_ema_list() is called but would now return
NULL because 

                if (ema_beacons) {
                        *ema_beacons =
                                ieee80211_beacon_get_ap_ema_list(hw, vif, link,

but the list is empty.

But you can still set NL80211_MBSSID_CONFIG_ATTR_EMA so it would be an
EMA AP, and have config->tx_wdev set ...

So I don't think this can be correct?

johannes
Sowmiya Sree Elavalagan July 4, 2024, 7:31 a.m. UTC | #2 
On 6/26/2024 5:45 PM, Johannes Berg wrote:
> On Fri, 2024-06-21 at 13:10 +0530, Sowmiya Sree Elavalagan wrote:
>> From: Rameshkumar Sundaram <quic_ramess@quicinc.com>
>>
>> Current implementation of MBSSID configuration parsing mandates
>> MBSSID elements for Tx BSS (index 0). However with ML link addition
>> it is possible that Non-Tx BSS'es can be added at a later point in
>> time after Tx BSS is brought up. Hence allow bring up of MBSSID Tx
>> BSS even if no Non-Tx BSS are present at that time. Later when new
>> Non-TX BSS are added TX BSS beacon can be updated with MBSSID IEs.
> 
> nit: I tend to think we should mostly use "element" instead of "IE"
> since the spec changed (subject and text), except where historically we
> have variable names etc.
> 
> I'm also not convinced this actually works without further changes down
> the stack? Think ath11k/mac80211 for example, where
> ieee80211_beacon_get_template_ema_list() is called but would now return
> NULL because 
> 
>                 if (ema_beacons) {
>                         *ema_beacons =
>                                 ieee80211_beacon_get_ap_ema_list(hw, vif, link,
> 
> but the list is empty.
> 
> But you can still set NL80211_MBSSID_CONFIG_ATTR_EMA so it would be an
> EMA AP, and have config->tx_wdev set ...
> 
> So I don't think this can be correct?
> 
> johannes

Hi Johannes,
 
I agree, but based on the current hostapd implementation, this flag NL80211_MBSSID_CONFIG_ATTR_EMA is set only when num_bss > 1. This flag will not be set when we do not have any non Tx BSS.  
Even if this NL80211_MBSSID_CONFIG_ATTR_EMA is set when no TX BSS is present, can just fill beacon template in 0th index of ieee80211_ema_beacons structure, if mbssid_ies are not present.  
Shall we handle this in mac80211 layer in ieee80211_beacon_get_ap_ema_list function like below
 
ieee80211_beacon_get_ap_ema_list(struct ieee80211_hw *hw,
                                 struct ieee80211_chanctx_conf *chanctx_conf)
{
         ...

-       if (!beacon->mbssid_ies || !beacon->mbssid_ies->cnt)
-               return NULL;
-
-       ema = kzalloc(struct_size(ema, bcn, beacon->mbssid_ies->cnt),
+       if (!beacon->mbssid_ies || !beacon->mbssid_ies->cnt) {
+               ema = kzalloc(struct_size(ema, bcn, 1), GFP_ATOMIC);
+               total_beacons = 1;
+
+       } else {
+               ema = kzalloc(struct_size(ema, bcn, beacon->mbssid_ies->cnt),
                      GFP_ATOMIC);
+               total_beacons = beacon->mbssid_ies->cnt;
+       }
+
        if (!ema)
                return NULL;
 
-       for (ema->cnt = 0; ema->cnt < beacon->mbssid_ies->cnt; ema->cnt++) {
+       for (ema->cnt = 0; ema->cnt < total_beacons; ema->cnt++) {
           .....


Thanks,
Sowmiya Sree
Johannes Berg July 4, 2024, 8:58 a.m. UTC | #3 
On Thu, 2024-07-04 at 13:01 +0530, Sowmiya Sree Elavalagan wrote:
> 
> I agree, but based on the current hostapd implementation, this flag NL80211_MBSSID_CONFIG_ATTR_EMA is set only when num_bss > 1. This flag will not be set when we do not have any non Tx BSS.
> 

Sure, but "based on the current hostapd implementation" isn't really
enough for the kernel to protect itself from userspace doing weird
things that it isn't prepared to handle :-)

It is, however, an argument for simply prohibiting it, I guess? If no
userspace is going to do it anyway?

> Even if this NL80211_MBSSID_CONFIG_ATTR_EMA is set when no TX BSS is present, can just fill beacon template in 0th index of ieee80211_ema_beacons structure, if mbssid_ies are not present.  
> Shall we handle this in mac80211 layer in ieee80211_beacon_get_ap_ema_list function like below
>  
> ieee80211_beacon_get_ap_ema_list(struct ieee80211_hw *hw,
>                                  struct ieee80211_chanctx_conf *chanctx_conf)
> {
>          ...
> 
> -       if (!beacon->mbssid_ies || !beacon->mbssid_ies->cnt)
> -               return NULL;
> -
> -       ema = kzalloc(struct_size(ema, bcn, beacon->mbssid_ies->cnt),
> +       if (!beacon->mbssid_ies || !beacon->mbssid_ies->cnt) {
> +               ema = kzalloc(struct_size(ema, bcn, 1), GFP_ATOMIC);
> +               total_beacons = 1;
> +
> +       } else {
> +               ema = kzalloc(struct_size(ema, bcn, beacon->mbssid_ies->cnt),
>                       GFP_ATOMIC);
> +               total_beacons = beacon->mbssid_ies->cnt;
> +       }
> +
>         if (!ema)
>                 return NULL;
>  
> -       for (ema->cnt = 0; ema->cnt < beacon->mbssid_ies->cnt; ema->cnt++) {
> +       for (ema->cnt = 0; ema->cnt < total_beacons; ema->cnt++) {
>            .....
> 

I don't know, is that really the only place? I didn't audit _all_ of it,
just looked at the first plausible code path that might be broken ...

Why can't we just prohibit it?

johannes
Sowmiya Sree Elavalagan July 5, 2024, 12:26 p.m. UTC | #4 
On 7/4/2024 2:28 PM, Johannes Berg wrote:
> On Thu, 2024-07-04 at 13:01 +0530, Sowmiya Sree Elavalagan wrote:
>>
>> I agree, but based on the current hostapd implementation, this flag NL80211_MBSSID_CONFIG_ATTR_EMA is set only when num_bss > 1. This flag will not be set when we do not have any non Tx BSS.
>>
> 
> Sure, but "based on the current hostapd implementation" isn't really
> enough for the kernel to protect itself from userspace doing weird
> things that it isn't prepared to handle :-)
> 
> It is, however, an argument for simply prohibiting it, I guess? If no
> userspace is going to do it anyway?
> 
>> Even if this NL80211_MBSSID_CONFIG_ATTR_EMA is set when no TX BSS is present, can just fill beacon template in 0th index of ieee80211_ema_beacons structure, if mbssid_ies are not present.  
>> Shall we handle this in mac80211 layer in ieee80211_beacon_get_ap_ema_list function like below
>>  
>> ieee80211_beacon_get_ap_ema_list(struct ieee80211_hw *hw,
>>                                  struct ieee80211_chanctx_conf *chanctx_conf)
>> {
>>          ...
>>
>> -       if (!beacon->mbssid_ies || !beacon->mbssid_ies->cnt)
>> -               return NULL;
>> -
>> -       ema = kzalloc(struct_size(ema, bcn, beacon->mbssid_ies->cnt),
>> +       if (!beacon->mbssid_ies || !beacon->mbssid_ies->cnt) {
>> +               ema = kzalloc(struct_size(ema, bcn, 1), GFP_ATOMIC);
>> +               total_beacons = 1;
>> +
>> +       } else {
>> +               ema = kzalloc(struct_size(ema, bcn, beacon->mbssid_ies->cnt),
>>                       GFP_ATOMIC);
>> +               total_beacons = beacon->mbssid_ies->cnt;
>> +       }
>> +
>>         if (!ema)
>>                 return NULL;
>>  
>> -       for (ema->cnt = 0; ema->cnt < beacon->mbssid_ies->cnt; ema->cnt++) {
>> +       for (ema->cnt = 0; ema->cnt < total_beacons; ema->cnt++) {
>>            .....
>>
> 
> I don't know, is that really the only place? I didn't audit _all_ of it,
> just looked at the first plausible code path that might be broken ...
> 
> Why can't we just prohibit it?
> 
> johannes

Hi Johannes,

You are right, better to handle this in kernel. Shall we mandate num_elems and mbssid index check if EMA is enabled in nl80211_parse_mbssid_config function.
But we will have to revisit this when dynamic link addition is supported along with EMA. 

Thanks,
Sowmiya Sree
diff mbox series

Patch

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index fcac7dedcd61..e579cc0c860c 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -5400,8 +5400,7 @@  static int nl80211_parse_mbssid_config(struct wiphy *wiphy,
 	}
 
 	config->index = nla_get_u8(tb[NL80211_MBSSID_CONFIG_ATTR_INDEX]);
-	if (config->index >= wiphy->mbssid_max_interfaces ||
-	    (!config->index && !num_elems))
+	if (config->index >= wiphy->mbssid_max_interfaces)
 		return -EINVAL;
 
 	if (tb[NL80211_MBSSID_CONFIG_ATTR_TX_IFINDEX]) {