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

[Arend van Spriel <arend.vanspriel@broadcom.com>]

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