Re: [PATCH] wifi: move action code from per-type frame structs

[Johannes Berg <johannes@sipsolutions.net>]

Hi all,

Couple of things I noticed while doing this, for driver maintainers:


> --- a/drivers/net/wireless/marvell/mwl8k.c
> +++ b/drivers/net/wireless/marvell/mwl8k.c
> @@ -1985,9 +1985,9 @@ mwl8k_txq_xmit(struct ieee80211_hw *hw,
>  	 */
>  	if (unlikely(ieee80211_is_action(wh->frame_control) &&
>  	    mgmt->u.action.category == WLAN_CATEGORY_BACK &&
> -	    mgmt->u.action.u.addba_req.action_code == WLAN_ACTION_ADDBA_REQ &&
> +	    mgmt->u.action.action_code == WLAN_ACTION_ADDBA_REQ &&
>  	    priv->ap_fw)) {
> -		u16 capab = le16_to_cpu(mgmt->u.action.u.addba_req.capab);
> +		u16 capab = le16_to_cpu(mgmt->u.action.addba_req.capab);
>  		tid = (capab & IEEE80211_ADDBA_PARAM_TID_MASK) >> 2;
>  		index = mwl8k_tid_queue_mapping(tid);


I'm not convinced this isn't broken because there's no length check for
the frame, and monitor mode injection could happen?

> --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mac.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mac.c
> @@ -414,8 +414,8 @@ mt76_connac2_mac_write_txwi_80211(struct mt76_dev *dev, __le32 *txwi,
>  
>  	if (ieee80211_is_action(fc) &&
>  	    mgmt->u.action.category == WLAN_CATEGORY_BACK &&
> -	    mgmt->u.action.u.addba_req.action_code == WLAN_ACTION_ADDBA_REQ) {
> -		u16 capab = le16_to_cpu(mgmt->u.action.u.addba_req.capab);
> +	    mgmt->u.action.action_code == WLAN_ACTION_ADDBA_REQ) {
> +		u16 capab = le16_to_cpu(mgmt->u.action.addba_req.capab);

Same here and in the other mt76 drivers, but

>  		txwi[5] |= cpu_to_le32(MT_TXD5_ADD_BA);
>  		tid = (capab >> 2) & IEEE80211_QOS_CTL_TID_MASK;

maybe this kind of thing shouldn't happen for injected frames anyway
(there's a flag you can check)

> +++ b/drivers/net/wireless/realtek/rtl8xxxu/core.c
> @@ -5146,10 +5146,10 @@ static void rtl8xxxu_dump_action(struct device *dev,
>  	if (!(rtl8xxxu_debug & RTL8XXXU_DEBUG_ACTION))
>  		return;
>  
> -	switch (mgmt->u.action.u.addba_resp.action_code) {
> +	switch (mgmt->u.action.action_code) {

This seems broken the same way.


> +++ b/drivers/net/wireless/realtek/rtlwifi/base.c
> @@ -1409,7 +1409,7 @@ bool rtl_action_proc(struct ieee80211_hw *hw, struct sk_buff *skb, u8 is_tx)
>  				sta_entry =
>  					(struct rtl_sta_info *)sta->drv_priv;
>  				capab =
> -				  le16_to_cpu(mgmt->u.action.u.addba_req.capab);
> +				  le16_to_cpu(mgmt->u.action.addba_req.capab);
>  				tid = (capab &
>  				       IEEE80211_ADDBA_PARAM_TID_MASK) >> 2;
>  				if (tid >= MAX_TID_COUNT) {

This one is worse because it's not even just local, it's over-the-air
(invoked on RX frames)

> @@ -2519,25 +2519,25 @@ struct sk_buff *rtl_make_del_ba(struct ieee80211_hw *hw,
>  	struct ieee80211_mgmt *action_frame;
>  	u16 params;
>  
> -	/* 27 = header + category + action + smps mode */
> -	skb = dev_alloc_skb(34 + hw->extra_tx_headroom);
> +	skb = dev_alloc_skb(IEEE80211_MIN_ACTION_SIZE(delba) +
> +			    hw->extra_tx_headroom);

This one's weird ... it got the comment copy/pasted from SMPS, so that's
completely wrong anyway, but if I'm counting correctly then
IEEE80211_MIN_ACTION_SIZE(delba) is actually 30 not 34, so was it
sending frames that are too long?

>  	if (!skb)
>  		return NULL;
>  
>  	skb_reserve(skb, hw->extra_tx_headroom);
> -	action_frame = skb_put_zero(skb, 34);
> +	action_frame = skb_put_zero(skb, IEEE80211_MIN_ACTION_SIZE(delba));
>  	memcpy(action_frame->sa, sa, ETH_ALEN);
>  	memcpy(action_frame->da, rtlefuse->dev_addr, ETH_ALEN);
>  	memcpy(action_frame->bssid, bssid, ETH_ALEN);
>  	action_frame->frame_control = cpu_to_le16(IEEE80211_FTYPE_MGMT |
>  						  IEEE80211_STYPE_ACTION);
>  	action_frame->u.action.category = WLAN_CATEGORY_BACK;
> -	action_frame->u.action.u.delba.action_code = WLAN_ACTION_DELBA;
> +	action_frame->u.action.action_code = WLAN_ACTION_DELBA;
>  	params = (u16)(1 << 11);	/* bit 11 initiator */
>  	params |= (u16)(tid << 12);	/* bit 15:12 TID number */
>  
> -	action_frame->u.action.u.delba.params = cpu_to_le16(params);
> -	action_frame->u.action.u.delba.reason_code =
> +	action_frame->u.action.delba.params = cpu_to_le16(params);
> +	action_frame->u.action.delba.reason_code =
>  		cpu_to_le16(WLAN_REASON_QSTA_TIMEOUT);

It's not putting anything beyond the normal DelBA frame ...

> +++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
> @@ -507,7 +507,7 @@ static void _rtl_pci_tx_isr(struct ieee80211_hw *hw, int prio)
>  		if (ieee80211_is_action(fc)) {
>  			struct ieee80211_mgmt *action_frame =
>  				(struct ieee80211_mgmt *)skb->data;
> -			if (action_frame->u.action.u.ht_smps.action ==
> +			if (action_frame->u.action.action_code ==
>  			    WLAN_HT_ACTION_SMPS) {

Same issue with frame length I think.

johannes