Re: [RFC 4/4] brcmfmac: support AP isolation to restrict reachability between stations

On June 4, 2025 4:37:02 PM Jeff Johnson <[email protected]> wrote:

> On 6/4/2025 1:55 AM, Arend van Spriel wrote:
>> From: Wright Feng <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Chi-hsien Lin <[email protected]>
>> Signed-off-by: Gokul Sivakumar <[email protected]>
>> [arend: indicate ap_isolate support in struct wiphy::bss_param_support]
>> Signed-off-by: Arend van Spriel <[email protected]>
>> ---
>> .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 24 +++++++++++++++++++
>> 1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c 
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> index dc2383faddd1..d6c8ad7ebced 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> @@ -5933,6 +5933,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,
>> @@ -5980,6 +6000,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,
>> };
>
> So the real question is do we really *need* all of the other patches, or is it
> OK for the driver to just process the one attribute it wants without
> indicating that is the only attribute it handles to userspace? I know of one
> out-of-tree cfg80211 driver that has only handled AP isolate for a long time,
> and AFAIK there have not been any issues.
>
> In other words, why isn't just the two diffs above enough to satisfy the need?
>
>>
>> struct cfg80211_ops *brcmf_cfg80211_get_ops(struct brcmf_mp_device *settings)
>> @@ -7634,6 +7655,9 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, 
>> struct brcmf_if *ifp)
>> BIT(NL80211_BSS_SELECT_ATTR_BAND_PREF) |
>> BIT(NL80211_BSS_SELECT_ATTR_RSSI_ADJUST);
>>
>> +
>> + wiphy->bss_param_support = WIPHY_BSS_PARAM_AP_ISOLATE;
>> +
>> wiphy->flags |= WIPHY_FLAG_NETNS_OK |
>> WIPHY_FLAG_PS_ON_BY_DEFAULT |
>> WIPHY_FLAG_HAVE_AP_SME |
>
> Ultimately that out-of-tree driver I know so well would adopt this scheme if
> it lands, but it currently support AP isolate without all of the other changes.

The first patch that initiated this was a brcmfmac patch which probably 
followed the same approach as the oot driver you know so well. Here is the 
patchwork link with the discussion that followed:

https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

Regards,
Arend