diff mbox series

[v2] wifi: nl80211: allow MBSSID Tx VAP bringup without MBSSID Elements

Message ID 20240731122201.2635010-1-quic_ssreeela@quicinc.com (mailing list archive)
State Under Review
Delegated to: Johannes Berg
Headers show
Series [v2] wifi: nl80211: allow MBSSID Tx VAP bringup without MBSSID Elements | expand

Commit Message

Sowmiya Sree Elavalagan July 31, 2024, 12:22 p.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 when EMA is
disabled. Later when new Non-TX BSS are added TX BSS beacon can be
updated with MBSSID Elements.

Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
Signed-off-by: Sowmiya Sree Elavalagan <quic_ssreeela@quicinc.com>
---
v2:
  - Mandate num elements check if EMA is enabled
  - Rebased and changed 'IEs' to 'elements' in commit message
---
 net/wireless/nl80211.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Johannes Berg Nov. 8, 2024, 8:53 a.m. UTC | #1 
On Wed, 2024-07-31 at 17:52 +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 when EMA is
> disabled. Later when new Non-TX BSS are added TX BSS beacon can be
> updated with MBSSID Elements.

So ... I've been deliberating on this for a while, because I really
couldn't convince myself quickly that it's correct.

And now that I look at it again, I'm again unsure whether or not it's
correct, because now you can have MBSSID w/o params->beacon.mbssid_ies
set, which has certain consequences, one of them being not setting
link_conf->bssid_indicator in mac80211.

This might not matter too much, depending on the driver, but it feels a
bit inconsistent? Also I worry about something expecting the pointer, I
guess.

(There might also be a bug in mac80211? The bssid_indicator isn't
updated when MBSSID disappears entirely?)

I feel the MBSSID code here is probably too fragile overall, maybe you
can find some time to go over it.

That said, I was discussing it with Benjamin briefly, and he realized
that you could just include an empty MBSSID element, or I think in fact
zero-length element data in the nl80211 API. That way, your code also
works with older versions, because you have num_elems, just that the
data in the elems is 3 bytes (empty MBSSID element) or 0 bytes (none at
all).

johannes
diff mbox series

Patch

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 7397a372c78e..f1b0274681b9 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -5496,8 +5496,10 @@  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 (config->ema && !config->index && !num_elems)
 		return -EINVAL;
 
 	if (tb[NL80211_MBSSID_CONFIG_ATTR_TX_IFINDEX]) {