| Message ID | 20250423175125.7233-1-gokulkumar.sivakumar@infineon.com |
|---|---|
| State | New |
| Headers | show |
| Series | brcmfmac: support AP isolation to restrict reachability between stations | expand |
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
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
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
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
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
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
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
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
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)