Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c177e3cc-ae69-b8c8-bb72-be85b2262149@quicinc.com>
Date:   Fri, 26 Aug 2022 08:48:16 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: [PATCH 1/1] cfg80211: Update Transition Disable policy during
 port authorization
Content-Language: en-US
To:     Vinayak Yadawad <vinayak.yadawad@broadcom.com>,
        <johannes@sipsolutions.net>
CC:     <linux-wireless@vger.kernel.org>, <jithu.jance@broadcom.com>
References: <e8fb69386675e5d128db141dabb9e6dad75eb841.1661516424.git.vinayak.yadawad@broadcom.com>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <e8fb69386675e5d128db141dabb9e6dad75eb841.1661516424.git.vinayak.yadawad@broadcom.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/26/2022 5:55 AM, Vinayak Yadawad wrote:

if this is a v2 it should have been annotated as such with a change log

> In case of 4way handshake offload, transition disable policy
> updated by the AP during EAPOL 3/4 is not updated to the upper layer.
> This results in mismatch between transition disable policy
> between the upper layer and the driver. This patch addresses this
> issue by updating transition disable policy as part of port
> authorization indiation.

s/indiation/indication/

> 
> Signed-off-by: Vinayak Yadawad <vinayak.yadawad@broadcom.com>
> ---
>   .../net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c   | 2 +-
>   include/net/cfg80211.h                                    | 3 ++-
>   include/uapi/linux/nl80211.h                              | 3 +++
>   net/wireless/core.h                                       | 4 +++-
>   net/wireless/nl80211.c                                    | 7 ++++++-
>   net/wireless/nl80211.h                                    | 3 ++-
>   net/wireless/sme.c                                        | 8 +++++---
>   net/wireless/util.c                                       | 2 +-
>   8 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index db45da33adfd..6654c4167c36 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -6028,7 +6028,7 @@ brcmf_bss_roaming_done(struct brcmf_cfg80211_info *cfg,
>   	brcmf_dbg(CONN, "Report roaming result\n");
>   
>   	if (profile->use_fwsup == BRCMF_PROFILE_FWSUP_1X && profile->is_ft) {
> -		cfg80211_port_authorized(ndev, profile->bssid, GFP_KERNEL);
> +		cfg80211_port_authorized(ndev, profile->bssid, -1, GFP_KERNEL);
>   		brcmf_dbg(CONN, "Report port authorized\n");
>   	}
>   
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 908d58393484..2fc173e88d31 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -7659,6 +7659,7 @@ void cfg80211_roamed(struct net_device *dev, struct cfg80211_roam_info *info,
>    *
>    * @dev: network device
>    * @bssid: the BSSID of the AP
> + * @td_bitmap: transition disable policy
>    * @gfp: allocation flags
>    *
>    * This function should be called by a driver that supports 4 way handshake
> @@ -7669,7 +7670,7 @@ void cfg80211_roamed(struct net_device *dev, struct cfg80211_roam_info *info,
>    * indicate the 802.11 association.
>    */
>   void cfg80211_port_authorized(struct net_device *dev, const u8 *bssid,
> -			      gfp_t gfp);
> +			      s16 td_bitmap, gfp_t gfp);
>   
>   /**
>    * cfg80211_disconnected - notify cfg80211 that connection was dropped
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index ffb7c573e299..c81cdc6d6c86 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -2741,6 +2741,8 @@ enum nl80211_commands {
>    *	When used with %NL80211_CMD_FRAME_TX_STATUS, indicates the ack RX
>    *	timestamp. When used with %NL80211_CMD_FRAME RX notification, indicates
>    *	the incoming frame RX timestamp.
> + * @NL80211_ATTR_TD_BITMAP: Transition Disable bitmap, for subsequent
> + *  (re)associations.
>    * @NUM_NL80211_ATTR: total number of nl80211_attrs available
>    * @NL80211_ATTR_MAX: highest attribute number currently defined
>    * @__NL80211_ATTR_AFTER_LAST: internal use
> @@ -3268,6 +3270,7 @@ enum nl80211_attrs {
>   
>   	NL80211_ATTR_TX_HW_TIMESTAMP,
>   	NL80211_ATTR_RX_HW_TIMESTAMP,
> +	NL80211_ATTR_TD_BITMAP,
>   
>   	/* add attributes here, update the policy in nl80211.c */

Johannes, do you want the policy updated even thought this is 
driver->userspace and hence the policy is never applied to it?

>   
> diff --git a/net/wireless/core.h b/net/wireless/core.h
> index 775e16cb99ed..d36d4b775284 100644
> --- a/net/wireless/core.h
> +++ b/net/wireless/core.h
> @@ -271,6 +271,7 @@ struct cfg80211_event {
>   		} ij;
>   		struct {
>   			u8 bssid[ETH_ALEN];
> +			s16 td_bitmap;

I know you are using -1 as an indication that the bitmap is not used, 
but using signed with a bitmap seems strange since bitops can be 
affected by sign extension. Just something that set off my Spider-Sense.

>   		} pa;
>   	};
>   };
> @@ -409,7 +410,8 @@ int cfg80211_disconnect(struct cfg80211_registered_device *rdev,
>   			bool wextev);
>   void __cfg80211_roamed(struct wireless_dev *wdev,
>   		       struct cfg80211_roam_info *info);
> -void __cfg80211_port_authorized(struct wireless_dev *wdev, const u8 *bssid);
> +void __cfg80211_port_authorized(struct wireless_dev *wdev, const u8 *bssid,
> +				s16 td_bitmap);
>   int cfg80211_mgd_wext_connect(struct cfg80211_registered_device *rdev,
>   			      struct wireless_dev *wdev);
>   void cfg80211_autodisconnect_wk(struct work_struct *work);
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 2705e3ee8fc4..176aa7b3bc6c 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -17841,7 +17841,8 @@ void nl80211_send_roamed(struct cfg80211_registered_device *rdev,
>   }
>   
>   void nl80211_send_port_authorized(struct cfg80211_registered_device *rdev,
> -				  struct net_device *netdev, const u8 *bssid)
> +				  struct net_device *netdev, const u8 *bssid,
> +				  s16 td_bitmap)
>   {
>   	struct sk_buff *msg;
>   	void *hdr;
> @@ -17861,6 +17862,10 @@ void nl80211_send_port_authorized(struct cfg80211_registered_device *rdev,
>   	    nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, bssid))
>   		goto nla_put_failure;
>   
> +	if (td_bitmap >= 0)
> +		if (nla_put_s16(msg, NL80211_ATTR_TD_BITMAP, td_bitmap))
> +			goto nla_put_failure;
> +
>   	genlmsg_end(msg, hdr);
>   
>   	genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
> diff --git a/net/wireless/nl80211.h b/net/wireless/nl80211.h
> index 855d540ddfb9..17abb0a46d8c 100644
> --- a/net/wireless/nl80211.h
> +++ b/net/wireless/nl80211.h
> @@ -83,7 +83,8 @@ void nl80211_send_roamed(struct cfg80211_registered_device *rdev,
>   			 struct net_device *netdev,
>   			 struct cfg80211_roam_info *info, gfp_t gfp);
>   void nl80211_send_port_authorized(struct cfg80211_registered_device *rdev,
> -				  struct net_device *netdev, const u8 *bssid);
> +				  struct net_device *netdev, const u8 *bssid,
> +				  s16 td_bitmap);
>   void nl80211_send_disconnected(struct cfg80211_registered_device *rdev,
>   			       struct net_device *netdev, u16 reason,
>   			       const u8 *ie, size_t ie_len, bool from_ap);
> diff --git a/net/wireless/sme.c b/net/wireless/sme.c
> index 27fb2a0c4052..e3150b9edb88 100644
> --- a/net/wireless/sme.c
> +++ b/net/wireless/sme.c
> @@ -1234,7 +1234,8 @@ void cfg80211_roamed(struct net_device *dev, struct cfg80211_roam_info *info,
>   }
>   EXPORT_SYMBOL(cfg80211_roamed);
>   
> -void __cfg80211_port_authorized(struct wireless_dev *wdev, const u8 *bssid)
> +void __cfg80211_port_authorized(struct wireless_dev *wdev, const u8 *bssid,
> +					s16 td_bitmap)
>   {
>   	ASSERT_WDEV_LOCK(wdev);
>   
> @@ -1247,11 +1248,11 @@ void __cfg80211_port_authorized(struct wireless_dev *wdev, const u8 *bssid)
>   		return;
>   
>   	nl80211_send_port_authorized(wiphy_to_rdev(wdev->wiphy), wdev->netdev,
> -				     bssid);
> +				     bssid, td_bitmap);
>   }
>   
>   void cfg80211_port_authorized(struct net_device *dev, const u8 *bssid,
> -			      gfp_t gfp)
> +			      s16 td_bitmap, gfp_t gfp)
>   {
>   	struct wireless_dev *wdev = dev->ieee80211_ptr;
>   	struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
> @@ -1267,6 +1268,7 @@ void cfg80211_port_authorized(struct net_device *dev, const u8 *bssid,
>   
>   	ev->type = EVENT_PORT_AUTHORIZED;
>   	memcpy(ev->pa.bssid, bssid, ETH_ALEN);
> +	ev->pa.td_bitmap = td_bitmap;
>   
>   	/*
>   	 * Use the wdev event list so that if there are pending
> diff --git a/net/wireless/util.c b/net/wireless/util.c
> index 2c127951764a..6a1d82c43766 100644
> --- a/net/wireless/util.c
> +++ b/net/wireless/util.c
> @@ -988,7 +988,7 @@ void cfg80211_process_wdev_events(struct wireless_dev *wdev)
>   			__cfg80211_leave(wiphy_to_rdev(wdev->wiphy), wdev);
>   			break;
>   		case EVENT_PORT_AUTHORIZED:
> -			__cfg80211_port_authorized(wdev, ev->pa.bssid);
> +			__cfg80211_port_authorized(wdev, ev->pa.bssid, ev->pa.td_bitmap);
>   			break;
>   		}
>   		wdev_unlock(wdev);