[PATCH] brcmfmac: support AP isolation to restrict reachability between stations

[PATCH] brcmfmac: support AP isolation to restrict reachability between stations

[Gokul Sivakumar]

From: Wright Feng <wright.feng@cypress.com>

hostapd & wpa_supplicant userspace daemons exposes an AP mode specific
config file parameter "ap_isolate" to the user, which is used to control
low-level bridging of frames between the stations associated in the BSS.

In driver, handle this user setting in the newly defined cfg80211_ops
function brcmf_cfg80211_change_bss() by enabling "ap_isolate" IOVAR in
the firmware.

In AP mode, the "ap_isolate" value from the cfg80211 layer represents,
 0 = allow low-level bridging of frames between associated stations
 1 = restrict low-level bridging of frames to isolate associated stations
-1 = do not change existing setting

Signed-off-by: Wright Feng <wright.feng@cypress.com>
Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
Signed-off-by: Gokul Sivakumar <gokulkumar.sivakumar@infineon.com>
---
 .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 4b70845e1a26..bd72d8df2a22 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5931,6 +5931,26 @@ static int brcmf_cfg80211_del_pmk(struct wiphy *wiphy, struct net_device *dev,
 	return brcmf_set_pmk(ifp, NULL, 0);
 }
 
+static int brcmf_cfg80211_change_bss(struct wiphy *wiphy, struct net_device *dev,
+				     struct bss_parameters *params)
+{
+	struct brcmf_if *ifp = netdev_priv(dev);
+	int ret = 0;
+
+	/* In AP mode, the "ap_isolate" value represents
+	 *  0 = allow low-level bridging of frames between associated stations
+	 *  1 = restrict low-level bridging of frames to isolate associated stations
+	 * -1 = do not change existing setting
+	 */
+	if (params->ap_isolate >= 0) {
+		ret = brcmf_fil_iovar_int_set(ifp, "ap_isolate", params->ap_isolate);
+		if (ret < 0)
+			brcmf_err("ap_isolate iovar failed: ret=%d\n", ret);
+	}
+
+	return ret;
+}
+
 static struct cfg80211_ops brcmf_cfg80211_ops = {
 	.add_virtual_intf = brcmf_cfg80211_add_iface,
 	.del_virtual_intf = brcmf_cfg80211_del_iface,
@@ -5978,6 +5998,7 @@ static struct cfg80211_ops brcmf_cfg80211_ops = {
 	.update_connect_params = brcmf_cfg80211_update_conn_params,
 	.set_pmk = brcmf_cfg80211_set_pmk,
 	.del_pmk = brcmf_cfg80211_del_pmk,
+	.change_bss = brcmf_cfg80211_change_bss,
 };
 
 struct cfg80211_ops *brcmf_cfg80211_get_ops(struct brcmf_mp_device *settings)
-- 
2.47.0

Re: [PATCH] brcmfmac: support AP isolation to restrict reachability between stations

[Johannes Berg]

On Wed, 2025-04-23 at 23:21 +0530, Gokul Sivakumar wrote:
> 
> +static int brcmf_cfg80211_change_bss(struct wiphy *wiphy, struct net_device *dev,
> +				     struct bss_parameters *params)
> +{
> +	struct brcmf_if *ifp = netdev_priv(dev);
> +	int ret = 0;
> +
> +	/* In AP mode, the "ap_isolate" value represents
> +	 *  0 = allow low-level bridging of frames between associated stations
> +	 *  1 = restrict low-level bridging of frames to isolate associated stations
> +	 * -1 = do not change existing setting
> +	 */
> +	if (params->ap_isolate >= 0) {
> +		ret = brcmf_fil_iovar_int_set(ifp, "ap_isolate", params->ap_isolate);
> +		if (ret < 0)
> +			brcmf_err("ap_isolate iovar failed: ret=%d\n", ret);
> +	}
> +
> +	return ret;
> +}

Seems like a terrible idea to accept any other changes silently without
doing anything at all.

Also, please pay attention to the linux-wireless list. Like, at all. We
started using tree tags months ago, we started using a different subject
prefix _years_ ago.

johannes

Re: [PATCH] brcmfmac: support AP isolation to restrict reachability between stations

[Arend van Spriel]

On 4/24/2025 12:15 AM, Johannes Berg wrote:
> On Wed, 2025-04-23 at 23:21 +0530, Gokul Sivakumar wrote:
>>
>> +static int brcmf_cfg80211_change_bss(struct wiphy *wiphy, struct net_device *dev,
>> +				     struct bss_parameters *params)
>> +{
>> +	struct brcmf_if *ifp = netdev_priv(dev);
>> +	int ret = 0;
>> +
>> +	/* In AP mode, the "ap_isolate" value represents
>> +	 *  0 = allow low-level bridging of frames between associated stations
>> +	 *  1 = restrict low-level bridging of frames to isolate associated stations
>> +	 * -1 = do not change existing setting
>> +	 */
>> +	if (params->ap_isolate >= 0) {
>> +		ret = brcmf_fil_iovar_int_set(ifp, "ap_isolate", params->ap_isolate);
>> +		if (ret < 0)
>> +			brcmf_err("ap_isolate iovar failed: ret=%d\n", ret);
>> +	}
>> +
>> +	return ret;
>> +}
> 
> Seems like a terrible idea to accept any other changes silently without
> doing anything at all.

Hi Johannes,

Agree. That would indeed give the wrong impression to user-space. 
However, what if the firmware does not support some of them that 
user-space actually want to change. Seems like we are missing a feedback 
mechanism here to inform user-space about partial failure to apply the 
requested parameters?

Looked at other drivers implementing this callback and here are the results:

[wil6210] wil_cfg80211_change_bss(): does exactly the same thing.
[wilc1000] change_bss(): worse! it accepts everything and does nothing.
[rtl8723bs] cfg80211_rtw_change_bss(): same. just an empty callback.
[mac80211] ieee80211_change_bss(): not surprising this looks pretty good

The mac80211 implementation fills a changed bitmask, but that is to 
inform the mac80211 driver what configuration changes to look for.

> Also, please pay attention to the linux-wireless list. Like, at all. We
> started using tree tags months ago, we started using a different subject
> prefix _years_ ago.

If this patch means Infineon is (mildly) regaining interest in upstream 
wifi development let's not discourage them. I do watch the 
linux-wireless list on occasion but I am a bit lost on your remark. What 
do you mean by tree tags. You mean the "wifi:" prefix? But then I am 
confused about the "subject prefix" remark.

Digging a bit further maybe you are referring to the "Tree labels" 
section [1]? I always considered a patch with only [PATCH] as being for 
-next implicitly. If it makes maintainer life easier I am happy to 
comply and add it explicit ;-)

Regards,
Arend

[1] 
https://wireless.docs.kernel.org/en/latest/en/developers/documentation/submittingpatches.html#tree-labels

Re: [PATCH] brcmfmac: support AP isolation to restrict reachability between stations

[Johannes Berg]

On Thu, 2025-04-24 at 11:50 +0200, Arend van Spriel wrote:
> 
> Looked at other drivers implementing this callback and here are the results:
> 
> [wil6210] wil_cfg80211_change_bss(): does exactly the same thing.
> [wilc1000] change_bss(): worse! it accepts everything and does nothing.
> [rtl8723bs] cfg80211_rtw_change_bss(): same. just an empty callback.

OK, though I guess other drivers being bad doesn't mean this one should
be :)

> If this patch means Infineon is (mildly) regaining interest in upstream 
> wifi development let's not discourage them.

Fair. I didn't mean to discourage. I just think to meaningfully
contribute upstream people should follow the list. And even review other
people's patches. I've been meaning to make that more of a requirement,
since I can't possibly meaningfully review everything I now need to
merge.

https://lore.kernel.org/linux-wireless/21896d2788b8bc6c7fcb534cd43e75671a57f494.camel@sipsolutions.net/

> I do watch the 
> linux-wireless list on occasion but I am a bit lost on your remark. What 
> do you mean by tree tags. You mean the "wifi:" prefix? But then I am 
> confused about the "subject prefix" remark.

Oh, well I guess terminology:

https://lore.kernel.org/linux-wireless/ec3a3d891acfe5ed8763271a1df4151d75daf25f.camel@sipsolutions.net/

johannes

Re: [PATCH] brcmfmac: support AP isolation to restrict reachability between stations

[Arend van Spriel]

On 4/24/2025 12:22 PM, Johannes Berg wrote:
> On Thu, 2025-04-24 at 11:50 +0200, Arend van Spriel wrote:
>>
>> Looked at other drivers implementing this callback and here are the results:
>>
>> [wil6210] wil_cfg80211_change_bss(): does exactly the same thing.
>> [wilc1000] change_bss(): worse! it accepts everything and does nothing.
>> [rtl8723bs] cfg80211_rtw_change_bss(): same. just an empty callback.
> 
> OK, though I guess other drivers being bad doesn't mean this one should
> be :)

Sure. I am on your team in this. Can you recommend a plan of attack 
here? Should we add a mechanism to expose what BSS parameter changes the 
driver can handle similar to what is used for struct 
station_info::bss_params?

>> If this patch means Infineon is (mildly) regaining interest in upstream
>> wifi development let's not discourage them.
> 
> Fair. I didn't mean to discourage. I just think to meaningfully
> contribute upstream people should follow the list. And even review other
> people's patches. I've been meaning to make that more of a requirement,
> since I can't possibly meaningfully review everything I now need to
> merge.
> 
> https://lore.kernel.org/linux-wireless/21896d2788b8bc6c7fcb534cd43e75671a57f494.camel@sipsolutions.net/

Yeah. It's a bit of a tough spot I reckon.

>> I do watch the
>> linux-wireless list on occasion but I am a bit lost on your remark. What
>> do you mean by tree tags. You mean the "wifi:" prefix? But then I am
>> confused about the "subject prefix" remark.
> 
> Oh, well I guess terminology:

Ack.

Regards,
Arend

Re: [PATCH] brcmfmac: support AP isolation to restrict reachability between stations

[Johannes Berg]

On Thu, 2025-04-24 at 13:36 +0200, Arend van Spriel wrote:
> On 4/24/2025 12:22 PM, Johannes Berg wrote:
> > On Thu, 2025-04-24 at 11:50 +0200, Arend van Spriel wrote:
> > > 
> > > Looked at other drivers implementing this callback and here are the results:
> > > 
> > > [wil6210] wil_cfg80211_change_bss(): does exactly the same thing.
> > > [wilc1000] change_bss(): worse! it accepts everything and does nothing.
> > > [rtl8723bs] cfg80211_rtw_change_bss(): same. just an empty callback.
> > 
> > OK, though I guess other drivers being bad doesn't mean this one should
> > be :)
> 
> Sure. I am on your team in this. Can you recommend a plan of attack 
> here? Should we add a mechanism to expose what BSS parameter changes the 
> driver can handle similar to what is used for struct 
> station_info::bss_params?

I don't even know off the top of my head what's there and what isn't,
just thought the code looked odd. I guess mac80211 just mostly supports
all of them anyway.

Today the change is simply rejected by cfg80211 with -EOPNOTSUPP:

static int nl80211_set_bss(struct sk_buff *skb, struct genl_info *info)
{
...
        if (!rdev->ops->change_bss)
                return -EOPNOTSUPP;


why not keep doing that for everything but ap_isolate?

Oh, I see what you mean, there's no way to keep that updated since you'd
have to check each value for being changed and reject that ...

Hmm, yeah, that's not great. I guess ideally then we'd have a bitmap of
what changed, and what the driver can support?

enum {
	CFG80211_BSS_PARAM_CHANGE_CTS_PROT	= BIT(0),
	CFG80211_BSS_PARAM_CHANGE_SHORT_PRE	= BIT(1),
	CFG80211_BSS_PARAM_CHANGE_SHORT_SLOT	= BIT(2),
...
	CFG80211_BSS_PARAM_CHANGE_AP_ISOLATE	= BIT(N),
...
};

and

struct bss_parameters {
	int link_id;
	u32 changed;
	...
}

and then this driver can just do

	if (params->changed & ~CFG80211_BSS_PARAM_CHANGE_AP_ISOLATE)
		return -EOPNOTSUPP;

or so?


Not sure how that'd be related to station_info::bss_param, that's just
output from the driver.

johannes

Re: [PATCH] brcmfmac: support AP isolation to restrict reachability between stations

[Arend van Spriel]

On 4/24/2025 1:46 PM, Johannes Berg wrote:
> On Thu, 2025-04-24 at 13:36 +0200, Arend van Spriel wrote:
>> On 4/24/2025 12:22 PM, Johannes Berg wrote:
>>> On Thu, 2025-04-24 at 11:50 +0200, Arend van Spriel wrote:
>>>>
>>>> Looked at other drivers implementing this callback and here are the results:
>>>>
>>>> [wil6210] wil_cfg80211_change_bss(): does exactly the same thing.
>>>> [wilc1000] change_bss(): worse! it accepts everything and does nothing.
>>>> [rtl8723bs] cfg80211_rtw_change_bss(): same. just an empty callback.
>>>
>>> OK, though I guess other drivers being bad doesn't mean this one should
>>> be :)
>>
>> Sure. I am on your team in this. Can you recommend a plan of attack
>> here? Should we add a mechanism to expose what BSS parameter changes the
>> driver can handle similar to what is used for struct
>> station_info::bss_params?
> 
> I don't even know off the top of my head what's there and what isn't,
> just thought the code looked odd. I guess mac80211 just mostly supports
> all of them anyway.
> 
> Today the change is simply rejected by cfg80211 with -EOPNOTSUPP:
> 
> static int nl80211_set_bss(struct sk_buff *skb, struct genl_info *info)
> {
> ...
>          if (!rdev->ops->change_bss)
>                  return -EOPNOTSUPP;
> 
> 
> why not keep doing that for everything but ap_isolate?
> 
> Oh, I see what you mean, there's no way to keep that updated since you'd
> have to check each value for being changed and reject that ...
> 
> Hmm, yeah, that's not great. I guess ideally then we'd have a bitmap of
> what changed, and what the driver can support?
> 
> enum {
> 	CFG80211_BSS_PARAM_CHANGE_CTS_PROT	= BIT(0),
> 	CFG80211_BSS_PARAM_CHANGE_SHORT_PRE	= BIT(1),
> 	CFG80211_BSS_PARAM_CHANGE_SHORT_SLOT	= BIT(2),
> ...
> 	CFG80211_BSS_PARAM_CHANGE_AP_ISOLATE	= BIT(N),
> ...
> };
> 
> and
> 
> struct bss_parameters {
> 	int link_id;
> 	u32 changed;
> 	...
> }
> 
> and then this driver can just do
> 
> 	if (params->changed & ~CFG80211_BSS_PARAM_CHANGE_AP_ISOLATE)
> 		return -EOPNOTSUPP;
> 
> or so?
> 
> 
> Not sure how that'd be related to station_info::bss_param, that's just
> output from the driver.

It kinda looked similar. At least there is a bitmap in place for it that 
is a subset of the enum you listed above. I was wondering if we should 
convey the bitmask to usesr-space beforehand in struct wiphy.

Gr. AvS

Re: [PATCH] brcmfmac: support AP isolation to restrict reachability between stations

[Gokul Sivakumar]

On 04/24, Arend van Spriel wrote:
> On 4/24/2025 1:46 PM, Johannes Berg wrote:
> > On Thu, 2025-04-24 at 13:36 +0200, Arend van Spriel wrote:
> > > On 4/24/2025 12:22 PM, Johannes Berg wrote:
> > > > On Thu, 2025-04-24 at 11:50 +0200, Arend van Spriel wrote:
> > > > > 
> > > > > Looked at other drivers implementing this callback and here are the results:
> > > > > 
> > > > > [wil6210] wil_cfg80211_change_bss(): does exactly the same thing.
> > > > > [wilc1000] change_bss(): worse! it accepts everything and does nothing.
> > > > > [rtl8723bs] cfg80211_rtw_change_bss(): same. just an empty callback.
> > > > 
> > > > OK, though I guess other drivers being bad doesn't mean this one should
> > > > be :)
> > > 
> > > Sure. I am on your team in this. Can you recommend a plan of attack
> > > here? Should we add a mechanism to expose what BSS parameter changes the
> > > driver can handle similar to what is used for struct
> > > station_info::bss_params?
> > 
> > I don't even know off the top of my head what's there and what isn't,
> > just thought the code looked odd. I guess mac80211 just mostly supports
> > all of them anyway.
> > 
> > Today the change is simply rejected by cfg80211 with -EOPNOTSUPP:
> > 
> > static int nl80211_set_bss(struct sk_buff *skb, struct genl_info *info)
> > {
> > ...
> >          if (!rdev->ops->change_bss)
> >                  return -EOPNOTSUPP;
> > 
> > 
> > why not keep doing that for everything but ap_isolate?
> > 
> > Oh, I see what you mean, there's no way to keep that updated since you'd
> > have to check each value for being changed and reject that ...
> > 
> > Hmm, yeah, that's not great. I guess ideally then we'd have a bitmap of
> > what changed, and what the driver can support?
> > 
> > enum {
> >       CFG80211_BSS_PARAM_CHANGE_CTS_PROT      = BIT(0),
> >       CFG80211_BSS_PARAM_CHANGE_SHORT_PRE     = BIT(1),
> >       CFG80211_BSS_PARAM_CHANGE_SHORT_SLOT    = BIT(2),
> > ...
> >       CFG80211_BSS_PARAM_CHANGE_AP_ISOLATE    = BIT(N),
> > ...
> > };
> > 
> > and
> > 
> > struct bss_parameters {
> >       int link_id;
> >       u32 changed;
> >       ...
> > }
> > 
> > and then this driver can just do
> > 
> >       if (params->changed & ~CFG80211_BSS_PARAM_CHANGE_AP_ISOLATE)
> >               return -EOPNOTSUPP;
> > 
> > or so?

IMHO, In CFG80211, if we introduce a bitmap to track which BSS Parameter
is changed by the userspace in the SET_BSS operation and then used this
bitmap while handling the request, then the WLAN Driver would reject the
operation with "EOPTNOTSUPP", instead of doing AP Isolation, because it
does not support setting the other BSS params in the request.

For Example, considering hostapd (iwd doesn't support SET BSS operation)
if the user only enabled the config file param "ap_isolate", but didn't
explicitly set "preamble", hostapd would implicitly set default value
(0 - LONG_PREAMBLE) in the preamble param while sending the SET_BSS
operation request to CFG80211.

CFG80211 would then mark the bit corresponding to the SHORT_PREAMBLE BSS
param as "changed" in the bitmap. WLAN Driver on receiving this SET_BSS
request, would then have to reject the entire operation without enabling
the user requested "ap_isolation" param, because of the preamble param
that is not even explicitly requested by the user.

In a way, we can consider this as an implementation specific behaviour
of the userspace application. But what i'm trying to convey is that the
userspace currently does not have a mechanism to know which subset of
BSS parameters are supported by the WLAN Drivers, so it is enabling
all the params while doing the SET_BSS operation. This increases the
chances for the entire SET_BSS operation getting rejected,
 
> > Not sure how that'd be related to station_info::bss_param, that's just
> > output from the driver.
> 
> It kinda looked similar. At least there is a bitmap in place for it that
> is a subset of the enum you listed above.

I guess, you are referring to the enum bss_param_flags. Since it is
currently being used by drivers to set the fields specific to STA BSS
while returning the STATION info to cfg80211, we may not be able to
extended this enum with the new AP specific BSS params like ap_isolate.

enum bss_param_flags {
        BSS_PARAM_FLAGS_CTS_PROT        = BIT(0),
        BSS_PARAM_FLAGS_SHORT_PREAMBLE  = BIT(1),
        BSS_PARAM_FLAGS_SHORT_SLOT_TIME = BIT(2)
};

Ideally renaming this from "bss_param_flags" -> "sta_bss_param_flags"
would be more appropriate, because these flags are only being used by
the struct member "station_info::bss::flags".

> I was wondering if we should convey the bitmask to usesr-space
> beforehand in struct wiphy.

Yes, if the WLAN driver is able to advertise the list of AP BSS params
that can be set bv userpace, then userspace can attempt to configure
only the BSS params that the driver can support, increasing the chances
for the SET_BSS operation to become successful.

And I can see the following two NL80211 feature flags already existing
which corresponds to the members in the AP/P2P-GO BSS Parameter struct.

@NL80211_FEATURE_P2P_GO_CTWIN: P2P GO implementation supports
	CT Window setting                                                                
@NL80211_FEATURE_P2P_GO_OPPPS: P2P GO implementation supports
	opportunistic powersave                                                              

However, for other AP BSS Parammeters, we don't have the corresponding
NL80211 feature flags.

Will wait for Johannes's opinion on this.

Regards,
Gokul

Re: [PATCH] brcmfmac: support AP isolation to restrict reachability between stations

[Johannes Berg]

On Sat, 2025-04-26 at 13:21 +0530, Gokul Sivakumar wrote:
> IMHO, In CFG80211, if we introduce a bitmap to track which BSS Parameter
> is changed by the userspace in the SET_BSS operation and then used this
> bitmap while handling the request, then the WLAN Driver would reject the
> operation with "EOPTNOTSUPP", instead of doing AP Isolation, because it
> does not support setting the other BSS params in the request.

Not necessarily?

> For Example, considering hostapd (iwd doesn't support SET BSS operation)
> if the user only enabled the config file param "ap_isolate", but didn't
> explicitly set "preamble", hostapd would implicitly set default value
> (0 - LONG_PREAMBLE) in the preamble param while sending the SET_BSS
> operation request to CFG80211.

But presumably that's still the default value in the driver too?

Hostapd could also be fixed too, to not change it if not requested.

> CFG80211 would then mark the bit corresponding to the SHORT_PREAMBLE BSS
> param as "changed" in the bitmap. WLAN Driver on receiving this SET_BSS
> request, would then have to reject the entire operation without enabling
> the user requested "ap_isolation" param, because of the preamble param
> that is not even explicitly requested by the user.

Or the driver can accept short-preamble setting, and reject it only if
it's set to short-preamble, rather than long-preamble.

> However, for other AP BSS Parammeters, we don't have the corresponding
> NL80211 feature flags.

Uh such a long time ago :) I don't remember why we had these. Given the
above do we need new ones? We can I guess, but I'm not sure it's needed
even if we change hostapd - if we do change it then it can just set only
the ones that were set in the config file?

johannes

Re: [PATCH] brcmfmac: support AP isolation to restrict reachability between stations

[Arend van Spriel]

On 4/26/2025 10:37 AM, Johannes Berg wrote:
> On Sat, 2025-04-26 at 13:21 +0530, Gokul Sivakumar wrote:
>> IMHO, In CFG80211, if we introduce a bitmap to track which BSS Parameter
>> is changed by the userspace in the SET_BSS operation and then used this
>> bitmap while handling the request, then the WLAN Driver would reject the
>> operation with "EOPTNOTSUPP", instead of doing AP Isolation, because it
>> does not support setting the other BSS params in the request.
> 
> Not necessarily?
> 
>> For Example, considering hostapd (iwd doesn't support SET BSS operation)
>> if the user only enabled the config file param "ap_isolate", but didn't
>> explicitly set "preamble", hostapd would implicitly set default value
>> (0 - LONG_PREAMBLE) in the preamble param while sending the SET_BSS
>> operation request to CFG80211.
> 
> But presumably that's still the default value in the driver too?
> 
> Hostapd could also be fixed too, to not change it if not requested.

To me this raises the question as to when this SET_BSS primitive should 
be used. As I recall one of the goals of nl80211 was to get rid of doing 
individual configuration change like we had in wext which even sometimes 
required specific order to get what we want. So why are these parameters 
not in the START_AP primitive (actually the P2P parameter are). Some of 
the parameters are probably implicit as they are represented in the 
beacon data passed in START_AP. The ap_isolate however is not expressed 
in the beacon afaik.

When all parameters would be handled by START_AP what would be the need 
for SET_BSS. I think it would be to modify the parameters for a BSS that 
is active, ie. beaconing, and possibly serving client STAs. However, 
this is where I get a bit confused. There is also a SET_BEACON primitive 
which looks like it supports a similar scenario.

>> CFG80211 would then mark the bit corresponding to the SHORT_PREAMBLE BSS
>> param as "changed" in the bitmap. WLAN Driver on receiving this SET_BSS
>> request, would then have to reject the entire operation without enabling
>> the user requested "ap_isolation" param, because of the preamble param
>> that is not even explicitly requested by the user.
> 
> Or the driver can accept short-preamble setting, and reject it only if
> it's set to short-preamble, rather than long-preamble.
> 
>> However, for other AP BSS Parammeters, we don't have the corresponding
>> NL80211 feature flags.
> 
> Uh such a long time ago :) I don't remember why we had these. Given the
> above do we need new ones? We can I guess, but I'm not sure it's needed
> even if we change hostapd - if we do change it then it can just set only
> the ones that were set in the config file?

The drivers that implement .change_bss() today have a relaxed policy. If 
we add restrictions in .change_bss() for brcmfmac that means from 
user-space perspective the API is not driver agnostic. Restricting the 
.change_bss() in the other drivers will likely cause user-space 
regression. So there should be a capability exchange between driver and 
user-space. My idea here is to have the driver add a nested attribute in 
NL80211_CMD_GET_WIPHY listing the bss parameters supported. Probably can 
use the same attribute as flag in NL80211_CMD_SET_BSS to indicate 
user-space does handle the bss parameter support info from the driver. 
Will try to get something implemented to see how that goes.

Regards,
Arend

Re: [PATCH] brcmfmac: support AP isolation to restrict reachability between stations

[Johannes Berg]

Hi,

> To me this raises the question as to when this SET_BSS primitive should 
> be used. As I recall one of the goals of nl80211 was to get rid of doing 
> individual configuration change like we had in wext which even sometimes 
> required specific order to get what we want. So why are these parameters 
> not in the START_AP primitive (actually the P2P parameter are). Some of 
> the parameters are probably implicit as they are represented in the 
> beacon data passed in START_AP. The ap_isolate however is not expressed 
> in the beacon afaik.

Yeah, I can't say I remember anything about it. Perhaps just because it
was NEW_BEACON rather than START_AP originally...

> When all parameters would be handled by START_AP what would be the need 
> for SET_BSS. I think it would be to modify the parameters for a BSS that 
> is active, ie. beaconing, and possibly serving client STAs. However, 
> this is where I get a bit confused. There is also a SET_BEACON primitive 
> which looks like it supports a similar scenario.

You can obviously change the beacon during the operation of the AP, I
can't tell you now why we didn't change it to SET_AP or CHANGE_AP when
we changed NEW_BEACON/DEL_BEACON to START_AP/STOP_AP ...

But I agree it'd make sense today to have it all with these operations.

But do we want to change everything now and have hostapd be able to do
both paths? Is the cleaner future in the kernel worth that complexity?

> The drivers that implement .change_bss() today have a relaxed policy. If 
> we add restrictions in .change_bss() for brcmfmac that means from 
> user-space perspective the API is not driver agnostic. 

Also true, mostly, though it'd depend on what you actually want to do.

I'd argue that the "default" settings should indeed be accepted by the
driver, but maybe if you set something that _isn't_ supported by
brcmfmac (and isn't default in hostapd) then refusing it would be more
user-friendly than ignoring it?

> Restricting the 
> .change_bss() in the other drivers will likely cause user-space 
> regression. 

Depends how much, in some ways the "regression" could be beneficial if
it would otherwise ignore settings the user wanted? But yeah, it could.

> So there should be a capability exchange between driver and 
> user-space. My idea here is to have the driver add a nested attribute in 
> NL80211_CMD_GET_WIPHY listing the bss parameters supported. Probably can 
> use the same attribute as flag in NL80211_CMD_SET_BSS to indicate 
> user-space does handle the bss parameter support info from the driver. 
> Will try to get something implemented to see how that goes.
> 

Cool, thanks! :)

johannes