[PATCH v3] wifi: rsi: Fix handling of 802.3 EAPOL frames sent via control port

[PATCH v3] wifi: rsi: Fix handling of 802.3 EAPOL frames sent via control port

[Marek Vasut]

When using wpa_supplicant v2.10, this driver is no longer able to
associate with any AP and fails in the EAPOL 4-way handshake while
sending the 2/4 message to the AP. The problem is not present in
wpa_supplicant v2.9 or older. The problem stems from HostAP commit
144314eaa ("wpa_supplicant: Send EAPOL frames over nl80211 where available")
which changes the way EAPOL frames are sent, from them being send
at L2 frames to them being sent via nl80211 control port.

An EAPOL frame sent as L2 frame is passed to the WiFi driver with
skb->protocol ETH_P_PAE, while EAPOL frame sent via nl80211 control
port has skb->protocol set to ETH_P_802_3 . The later happens in
ieee80211_tx_control_port(), where the EAPOL frame is encapsulated
into 802.3 frame.

The rsi_91x driver handles ETH_P_PAE EAPOL frames as high-priority
frames and sends them via highest-priority transmit queue, while
the ETH_P_802_3 frames are sent as regular frames. The EAPOL 4-way
handshake frames must be sent as highest-priority, otherwise the
4-way handshake times out.

Therefore, to fix this problem, inspect the ETH_P_802_3 frames in
the rsi_91x driver, check the ethertype of the encapsulated frame,
and in case it is ETH_P_PAE, transmit the frame via high-priority
queue just like other ETH_P_PAE frames.

Fixes: 0eb42586cf87 ("rsi: data packet descriptor enhancements")
Signed-off-by: Marek Vasut <marex@denx.de>
---
NOTE: I am really unsure about the method of finding out the exact
      place of ethernet header in the encapsulated packet and then
      extracting the ethertype from it. Is there maybe some sort of
      helper function for that purpose ?
---
V2: - Turn the duplicated code into common function
V3: - Simplify the TX EAPOL detection (Johannes)
---
Cc: Amitkumar Karwar <amit.karwar@redpinesignals.com>
Cc: Angus Ainslie <angus@akkea.ca>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: Martin Fuzzey <martin.fuzzey@flowbird.group>
Cc: Martin Kepplinger <martink@posteo.de>
Cc: Prameela Rani Garnepudi <prameela.j04cs@gmail.com>
Cc: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
Cc: Siva Rebbagondla <siva8118@gmail.com>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
 drivers/net/wireless/rsi/rsi_91x_core.c | 9 ++++++++-
 drivers/net/wireless/rsi/rsi_91x_hal.c  | 5 ++++-
 drivers/net/wireless/rsi/rsi_common.h   | 1 +
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_core.c b/drivers/net/wireless/rsi/rsi_91x_core.c
index 0f3a80f66b61c..efc504042754b 100644
--- a/drivers/net/wireless/rsi/rsi_91x_core.c
+++ b/drivers/net/wireless/rsi/rsi_91x_core.c
@@ -364,6 +364,12 @@ struct ieee80211_vif *rsi_get_vif(struct rsi_hw *adapter, u8 *mac)
 	return NULL;
 }
 
+bool rsi_is_tx_eapol(struct sk_buff *skb)
+{
+	return !!(IEEE80211_SKB_CB(skb)->control.flags &
+		  IEEE80211_TX_CTRL_PORT_CTRL_PROTO);
+}
+
 /**
  * rsi_core_xmit() - This function transmits the packets received from mac80211
  * @common: Pointer to the driver private structure.
@@ -466,7 +472,8 @@ void rsi_core_xmit(struct rsi_common *common, struct sk_buff *skb)
 							      tid, 0);
 			}
 		}
-		if (skb->protocol == cpu_to_be16(ETH_P_PAE)) {
+
+		if (rsi_is_tx_eapol(skb)) {
 			q_num = MGMT_SOFT_Q;
 			skb->priority = q_num;
 		}
diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c b/drivers/net/wireless/rsi/rsi_91x_hal.c
index c61f83a7333b6..a35866e9161e5 100644
--- a/drivers/net/wireless/rsi/rsi_91x_hal.c
+++ b/drivers/net/wireless/rsi/rsi_91x_hal.c
@@ -159,6 +159,7 @@ int rsi_prepare_data_desc(struct rsi_common *common, struct sk_buff *skb)
 	struct rsi_data_desc *data_desc;
 	struct rsi_xtended_desc *xtend_desc;
 	u8 ieee80211_size = MIN_802_11_HDR_LEN;
+	bool tx_eapol = false;
 	u8 header_size;
 	u8 vap_id = 0;
 	u8 dword_align_bytes;
@@ -168,6 +169,8 @@ int rsi_prepare_data_desc(struct rsi_common *common, struct sk_buff *skb)
 	vif = info->control.vif;
 	tx_params = (struct skb_info *)info->driver_data;
 
+	tx_eapol = rsi_is_tx_eapol(skb);
+
 	header_size = FRAME_DESC_SZ + sizeof(struct rsi_xtended_desc);
 	if (header_size > skb_headroom(skb)) {
 		rsi_dbg(ERR_ZONE, "%s: Unable to send pkt\n", __func__);
@@ -231,7 +234,7 @@ int rsi_prepare_data_desc(struct rsi_common *common, struct sk_buff *skb)
 		}
 	}
 
-	if (skb->protocol == cpu_to_be16(ETH_P_PAE)) {
+	if (tx_eapol) {
 		rsi_dbg(INFO_ZONE, "*** Tx EAPOL ***\n");
 
 		data_desc->frame_info = cpu_to_le16(RATE_INFO_ENABLE);
diff --git a/drivers/net/wireless/rsi/rsi_common.h b/drivers/net/wireless/rsi/rsi_common.h
index 7aa5124575cfe..8843c7634e2f9 100644
--- a/drivers/net/wireless/rsi/rsi_common.h
+++ b/drivers/net/wireless/rsi/rsi_common.h
@@ -83,6 +83,7 @@ u16 rsi_get_connected_channel(struct ieee80211_vif *vif);
 struct rsi_hw *rsi_91x_init(u16 oper_mode);
 void rsi_91x_deinit(struct rsi_hw *adapter);
 int rsi_read_pkt(struct rsi_common *common, u8 *rx_pkt, s32 rcv_pkt_len);
+bool rsi_is_tx_eapol(struct sk_buff *skb);
 #ifdef CONFIG_PM
 int rsi_config_wowlan(struct rsi_hw *adapter, struct cfg80211_wowlan *wowlan);
 #endif
-- 
2.35.1

Re: [PATCH v3] wifi: rsi: Fix handling of 802.3 EAPOL frames sent via control port

[Johannes Berg]

On Fri, 2022-11-04 at 16:58 +0100, Marek Vasut wrote:
> 
> Therefore, to fix this problem, inspect the ETH_P_802_3 frames in
> the rsi_91x driver, check the ethertype of the encapsulated frame,
> and in case it is ETH_P_PAE, transmit the frame via high-priority
> queue just like other ETH_P_PAE frames.

This part seems wrong now.

> +bool rsi_is_tx_eapol(struct sk_buff *skb)
> +{
> +	return !!(IEEE80211_SKB_CB(skb)->control.flags &
> +		  IEEE80211_TX_CTRL_PORT_CTRL_PROTO);
> +}

For how trivial this is now, maybe make it an inline? Feels fairly
pointless to have it as an out-of-line function to call in another file
when it's a simple bit check.

You can also drop the !! since the return value is bool.

johannes

Re: [PATCH v3] wifi: rsi: Fix handling of 802.3 EAPOL frames sent via control port

[Marek Vasut]

On 11/4/22 17:01, Johannes Berg wrote:
> On Fri, 2022-11-04 at 16:58 +0100, Marek Vasut wrote:
>>
>> Therefore, to fix this problem, inspect the ETH_P_802_3 frames in
>> the rsi_91x driver, check the ethertype of the encapsulated frame,
>> and in case it is ETH_P_PAE, transmit the frame via high-priority
>> queue just like other ETH_P_PAE frames.
> 
> This part seems wrong now.

OK

>> +bool rsi_is_tx_eapol(struct sk_buff *skb)
>> +{
>> +	return !!(IEEE80211_SKB_CB(skb)->control.flags &
>> +		  IEEE80211_TX_CTRL_PORT_CTRL_PROTO);
>> +}
> 
> For how trivial this is now, maybe make it an inline? Feels fairly
> pointless to have it as an out-of-line function to call in another file
> when it's a simple bit check.

In V2 it was suggested I deduplicate this into a separate function, 
since the test is done in multiple places. I would like to keep it 
deduplicated.

> You can also drop the !! since the return value is bool.

OK

Re: [PATCH v3] wifi: rsi: Fix handling of 802.3 EAPOL frames sent via control port

[Johannes Berg]

On Fri, 2022-11-04 at 17:09 +0100, Marek Vasut wrote:
> 
> In V2 it was suggested I deduplicate this into a separate function, 
> since the test is done in multiple places. I would like to keep it 
> deduplicated.

Well, it's now a lot simpler, so one might argue that it's not needed.

But anyway you're touching the hot-path of this driver, and making it an
inline still keeps it de-duplicated, so not sure why you wouldn't just
move the rsi_is_tx_eapol into the header file as static inline?

johannes

Re: [PATCH v3] wifi: rsi: Fix handling of 802.3 EAPOL frames sent via control port

[Johannes Berg]

(removing Amitkumar Karwar <amit.karwar@redpinesignals.com> as that
keeps bouncing)

On Fri, 2022-11-04 at 17:23 +0100, Johannes Berg wrote:
> On Fri, 2022-11-04 at 17:09 +0100, Marek Vasut wrote:
> > 
> > In V2 it was suggested I deduplicate this into a separate function, 
> > since the test is done in multiple places. I would like to keep it 
> > deduplicated.
> 
> Well, it's now a lot simpler, so one might argue that it's not needed.
> 
> But anyway you're touching the hot-path of this driver, and making it an
> inline still keeps it de-duplicated, so not sure why you wouldn't just
> move the rsi_is_tx_eapol into the header file as static inline?
> 

Though honestly, if we thought it was worth deduplicating a simple bit
check like that, we could define an inline helper in mac80211.h. I'm not
really convinced it is :)

But hey, that's not my driver, so ultimately I don't _really_ care so
much.

johannes