diff mbox series

[v7,2/3] nl80211: additional processing in NL80211_CMD_SET_BEACON

Message ID 20221109214720.6097-3-quic_alokad@quicinc.com
State Superseded
Headers show
Series Additional processing in NL80211_CMD_SET_BEACON | expand

Commit Message

Aloka Dixit Nov. 9, 2022, 9:47 p.m. UTC
FILS discovery and unsolicited broadcast probe response transmissions
are configured as part of NL80211_CMD_START_AP, however both stop
after userspace uses the NL80211_CMD_SET_BEACON command as these
attributes are not processed in that command.
Add the missing implementation.

Signed-off-by: Aloka Dixit <quic_alokad@quicinc.com>
---
 net/wireless/nl80211.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Johannes Berg Jan. 18, 2023, 3:57 p.m. UTC | #1
On Wed, 2022-11-09 at 13:47 -0800, Aloka Dixit wrote:
> FILS discovery and unsolicited broadcast probe response transmissions
> are configured as part of NL80211_CMD_START_AP, however both stop
> after userspace uses the NL80211_CMD_SET_BEACON command as these
> attributes are not processed in that command.

That seems odd. Why would that *stop*? Nothing in the driver should
actually update it to _remove_ it during change_beacon()?

Are you sure you didn't mean to "just" say "however both cannot be
updated as these attributes ..."?

johannes
Aloka Dixit Jan. 19, 2023, 7:40 p.m. UTC | #2
On 1/18/2023 7:57 AM, Johannes Berg wrote:
> On Wed, 2022-11-09 at 13:47 -0800, Aloka Dixit wrote:
>> FILS discovery and unsolicited broadcast probe response transmissions
>> are configured as part of NL80211_CMD_START_AP, however both stop
>> after userspace uses the NL80211_CMD_SET_BEACON command as these
>> attributes are not processed in that command.
> 
> That seems odd. Why would that *stop*? Nothing in the driver should
> actually update it to _remove_ it during change_beacon()?
> > Are you sure you didn't mean to "just" say "however both cannot be
> updated as these attributes ..."?
> 
> johannes

FILS discovery has primary channel, center frequency in the frame format 
which should be changed depending on why the beacon changed. Hence the 
current design, at least ath11k, assumes that BSS_CHANGED_FILS_DISCOVERY 
and BSS_CHANGED_UNSOL_BCAST_PROBE_RESP "not being set", when beacon is 
changed, means disable these features.
What do you think?

Like you said, we still need this code to update these templates if 
provided by the userspace, e.g. in case of channel switch.

Should I send a follow-up with a different commit log?

Thanks.
Johannes Berg Jan. 19, 2023, 8:12 p.m. UTC | #3
On Thu, 2023-01-19 at 11:40 -0800, Aloka Dixit wrote:
> On 1/18/2023 7:57 AM, Johannes Berg wrote:
> > On Wed, 2022-11-09 at 13:47 -0800, Aloka Dixit wrote:
> > > FILS discovery and unsolicited broadcast probe response transmissions
> > > are configured as part of NL80211_CMD_START_AP, however both stop
> > > after userspace uses the NL80211_CMD_SET_BEACON command as these
> > > attributes are not processed in that command.
> > 
> > That seems odd. Why would that *stop*? Nothing in the driver should
> > actually update it to _remove_ it during change_beacon()?
> > > Are you sure you didn't mean to "just" say "however both cannot be
> > updated as these attributes ..."?
> > 
> > johannes
> 
> FILS discovery has primary channel, center frequency in the frame format 
> which should be changed depending on why the beacon changed. 

Hmm? Sure, the frequency is important, but beacon can change for so many
other reasons? Even just the greenfield bit in HT would cause a beacon
change as far as cfg80211/mac80211 are concerned.

> Hence the 
> current design, at least ath11k, assumes that BSS_CHANGED_FILS_DISCOVERY 
> and BSS_CHANGED_UNSOL_BCAST_PROBE_RESP "not being set", when beacon is 
> changed, means disable these features.
> What do you think?

I think that makes no sense? If mac80211 didn't clear struct
ieee80211_bss_conf::fils_discovery, then surely it should stick around
even if the beacon changes???

Surely you can't be right - that'd basically mean the whole feature is
useless right now because even the greenfield update or similar that
basically *always* happens in hostapd would disable it, since the beacon
changes and we don't have these patches?

> Should I send a follow-up with a different commit log?
> 

Well seems like we need to first figure out the correct semantics here,
and possibly fix things...

johannes
Aloka Dixit Jan. 19, 2023, 8:43 p.m. UTC | #4
On 1/19/2023 12:12 PM, Johannes Berg wrote:
> On Thu, 2023-01-19 at 11:40 -0800, Aloka Dixit wrote:
>> On 1/18/2023 7:57 AM, Johannes Berg wrote:
>>> On Wed, 2022-11-09 at 13:47 -0800, Aloka Dixit wrote:
>>>> FILS discovery and unsolicited broadcast probe response transmissions
>>>> are configured as part of NL80211_CMD_START_AP, however both stop
>>>> after userspace uses the NL80211_CMD_SET_BEACON command as these
>>>> attributes are not processed in that command.
>>>
>>> That seems odd. Why would that *stop*? Nothing in the driver should
>>> actually update it to _remove_ it during change_beacon()?
>>>> Are you sure you didn't mean to "just" say "however both cannot be
>>> updated as these attributes ..."?
>>>
>>> johannes
>>
>> FILS discovery has primary channel, center frequency in the frame format
>> which should be changed depending on why the beacon changed.
> 
> Hmm? Sure, the frequency is important, but beacon can change for so many
> other reasons? Even just the greenfield bit in HT would cause a beacon
> change as far as cfg80211/mac80211 are concerned.
> 
>> Hence the
>> current design, at least ath11k, assumes that BSS_CHANGED_FILS_DISCOVERY
>> and BSS_CHANGED_UNSOL_BCAST_PROBE_RESP "not being set", when beacon is
>> changed, means disable these features.
>> What do you think?
> 
> I think that makes no sense? If mac80211 didn't clear struct
> ieee80211_bss_conf::fils_discovery, then surely it should stick around
> even if the beacon changes???
> 
"max_interval" was be used as the enable/disable knob for these 
features. Non-zero = enable, zero = disable.
But the side effect of that is if NL80211 does not receive these 
attributes then by default the interval is set to 0.

> Surely you can't be right - that'd basically mean the whole feature is
> useless right now because even the greenfield update or similar that
> basically *always* happens in hostapd would disable it, since the beacon
> changes and we don't have these patches?
>Pretty much, yeah.

>> Should I send a follow-up with a different commit log?
>>
> 
> Well seems like we need to first figure out the correct semantics here,
> and possibly fix things...
> 
> johannes

I can think of following:
(1) max_interval cannot be the enable/disable knob because then every 
code path in the userspace would still need to set it to non-zero to 
continue transmission. Are you okay with having extra members in enum 
nl80211_fils_discovery_attributes to ENABLE/DISABLE? I think that is 
what you suggested in your comment for the next patch in this series as 
well.

(2) If the template needs changing for any reason then the userspace 
will be responsible to send a new one. Until then the driver will 
continue the transmission with existing template and interval unless 
DISABLE is used.

Thanks.
Johannes Berg Jan. 19, 2023, 8:47 p.m. UTC | #5
On Thu, 2023-01-19 at 12:43 -0800, Aloka Dixit wrote:
> > 
> > > Hence the
> > > current design, at least ath11k, assumes that BSS_CHANGED_FILS_DISCOVERY
> > > and BSS_CHANGED_UNSOL_BCAST_PROBE_RESP "not being set", when beacon is
> > > changed, means disable these features.
> > > What do you think?
> > 
> > I think that makes no sense? If mac80211 didn't clear struct
> > ieee80211_bss_conf::fils_discovery, then surely it should stick around
> > even if the beacon changes???
> > 
> "max_interval" was be used as the enable/disable knob for these 
> features. Non-zero = enable, zero = disable.
> But the side effect of that is if NL80211 does not receive these 
> attributes then by default the interval is set to 0.


But it doesn't change in bss_conf if you send change beacon, at least
not before these patches?

Or am I confusing myself?

> I can think of following:
> (1) max_interval cannot be the enable/disable knob because then every 
> code path in the userspace would still need to set it to non-zero to 
> continue transmission. Are you okay with having extra members in enum 
> nl80211_fils_discovery_attributes to ENABLE/DISABLE? I think that is 
> what you suggested in your comment for the next patch in this series as 
> well.
> 
> (2) If the template needs changing for any reason then the userspace 
> will be responsible to send a new one. Until then the driver will 
> continue the transmission with existing template and interval unless 
> DISABLE is used.
> 

Were those meant to be mutually exclusive, because it doesn't seem like
that to me? I think (2) must be what happens now, before these patches,
because I don't see where it would be changed? Like I said above.

I agree that we'd now need an explicit way to indicate "disable", but we
could for example say you disable by adding a nested
NL80211_ATTR_FILS_DISCOVERY attribute without any of the sub-attributes,
which logically kind of makes sense - you're changing
NL80211_ATTR_FILS_DISCOVERY, but you're not including a new set of
parameters, so logically that means disable?

johannes
Aloka Dixit Jan. 19, 2023, 9:19 p.m. UTC | #6
On 1/19/2023 12:47 PM, Johannes Berg wrote:
> On Thu, 2023-01-19 at 12:43 -0800, Aloka Dixit wrote:
>>>
>>>> Hence the
>>>> current design, at least ath11k, assumes that BSS_CHANGED_FILS_DISCOVERY
>>>> and BSS_CHANGED_UNSOL_BCAST_PROBE_RESP "not being set", when beacon is
>>>> changed, means disable these features.
>>>> What do you think?
>>>
>>> I think that makes no sense? If mac80211 didn't clear struct
>>> ieee80211_bss_conf::fils_discovery, then surely it should stick around
>>> even if the beacon changes???
>>>
>> "max_interval" was be used as the enable/disable knob for these
>> features. Non-zero = enable, zero = disable.
>> But the side effect of that is if NL80211 does not receive these
>> attributes then by default the interval is set to 0.
> 
> 
> But it doesn't change in bss_conf if you send change beacon, at least
> not before these patches?
> 
> Or am I confusing myself?
> 

Your understanding is correct, however, without these patches there is a 
cascading effect.
-> NL80211: If the attribute is not sent by user-space/processed in this 
layer then cfg80211_ap_settings->fils_discovery->max_interval is 0 
(default).
-> MAC80211: max_interval=0, hence BSS_CHANGED_FILS_DISCOVERY is not set
-> ath11k: BSS_CHANGED_FILS_DISCOVERY is not set hence driver doesn't 
configure/re-configure. Unless target doesn't receive it every beacon 
change, it disables it.

I can make changes up to the driver to fix this part.

>> I can think of following:
>> (1) max_interval cannot be the enable/disable knob because then every
>> code path in the userspace would still need to set it to non-zero to
>> continue transmission. Are you okay with having extra members in enum
>> nl80211_fils_discovery_attributes to ENABLE/DISABLE? I think that is
>> what you suggested in your comment for the next patch in this series as
>> well.
>>
>> (2) If the template needs changing for any reason then the userspace
>> will be responsible to send a new one. Until then the driver will
>> continue the transmission with existing template and interval unless
>> DISABLE is used.
>>
> 
> Were those meant to be mutually exclusive, because it doesn't seem like
> that to me? I think (2) must be what happens now, before these patches,
> because I don't see where it would be changed? Like I said above.
> 

Not exclusive. I meant I can make both the changes above to not have the 
above mentioned cascading effect

> I agree that we'd now need an explicit way to indicate "disable", but we
> could for example say you disable by adding a nested
> NL80211_ATTR_FILS_DISCOVERY attribute without any of the sub-attributes,
> which logically kind of makes sense - you're changing
> NL80211_ATTR_FILS_DISCOVERY, but you're not including a new set of
> parameters, so logically that means disable?
> 
> johannes

Sure, will update nl80211_parse_fils_discovery() to allow this case and 
reset all to 0/null etc.

I can start a new series with all the changes, and include current 
patches last.

Will that work?
Johannes Berg Feb. 14, 2023, 2:11 p.m. UTC | #7
Hi Aloka,

Sorry, clearly I dropped the ball on this.


On Thu, 2023-01-19 at 13:19 -0800, Aloka Dixit wrote:
> > But it doesn't change in bss_conf if you send change beacon, at least
> > not before these patches?
> > 
> > Or am I confusing myself?
> > 
> 
> Your understanding is correct, however, without these patches there is a 
> cascading effect.
> -> NL80211: If the attribute is not sent by user-space/processed in this 
> layer then cfg80211_ap_settings->fils_discovery->max_interval is 0 
> (default).
> -> MAC80211: max_interval=0, hence BSS_CHANGED_FILS_DISCOVERY is not set
> -> ath11k: BSS_CHANGED_FILS_DISCOVERY is not set hence driver doesn't 
> configure/re-configure. Unless target doesn't receive it every beacon 
> change, it disables it.

Hmm. But if max_interval is 0 in bss_conf then the driver might still
look at it even if BSS_CHANGED_FILS_DISCOVERY is not set, for example
for firmware restart? It seems bad to rely on the change flag only for
all this.

> I can make changes up to the driver to fix this part.

Not sure it's a driver change?

> > > I can think of following:
> > > (1) max_interval cannot be the enable/disable knob because then every
> > > code path in the userspace would still need to set it to non-zero to
> > > continue transmission. Are you okay with having extra members in enum
> > > nl80211_fils_discovery_attributes to ENABLE/DISABLE? I think that is
> > > what you suggested in your comment for the next patch in this series as
> > > well.
> > > 
> > > (2) If the template needs changing for any reason then the userspace
> > > will be responsible to send a new one. Until then the driver will
> > > continue the transmission with existing template and interval unless
> > > DISABLE is used.
> > > 
> > 
> > Were those meant to be mutually exclusive, because it doesn't seem like
> > that to me? I think (2) must be what happens now, before these patches,
> > because I don't see where it would be changed? Like I said above.
> > 
> 
> Not exclusive. I meant I can make both the changes above to not have the 
> above mentioned cascading effect

Right ok.

> 
> > I agree that we'd now need an explicit way to indicate "disable", but we
> > could for example say you disable by adding a nested
> > NL80211_ATTR_FILS_DISCOVERY attribute without any of the sub-attributes,
> > which logically kind of makes sense - you're changing
> > NL80211_ATTR_FILS_DISCOVERY, but you're not including a new set of
> > parameters, so logically that means disable?
> > 
> > johannes
> 
> Sure, will update nl80211_parse_fils_discovery() to allow this case and 
> reset all to 0/null etc.
> 
> I can start a new series with all the changes, and include current 
> patches last.
> 
> Will that work?

I think yes, seems like we have to fix up some things here first.

johannes
diff mbox series

Patch

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 4b0b02fc822c..95de9e006444 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -6069,6 +6069,7 @@  static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info)
 	struct net_device *dev = info->user_ptr[1];
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	struct cfg80211_ap_settings *params;
+	struct nlattr *attrs;
 	int err;
 
 	if (dev->ieee80211_ptr->iftype != NL80211_IFTYPE_AP &&
@@ -6089,6 +6090,20 @@  static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info)
 	if (err)
 		goto out;
 
+	attrs = info->attrs[NL80211_ATTR_FILS_DISCOVERY];
+	if (attrs) {
+		err = nl80211_parse_fils_discovery(rdev, attrs, params);
+		if (err)
+			goto out;
+	}
+
+	attrs = info->attrs[NL80211_ATTR_UNSOL_BCAST_PROBE_RESP];
+	if (attrs) {
+		err = nl80211_parse_unsol_bcast_probe_resp(rdev, attrs, params);
+		if (err)
+			goto out;
+	}
+
 	wdev_lock(wdev);
 	err = rdev_change_beacon(rdev, dev, params);
 	wdev_unlock(wdev);