Re: [PATCH v2] wifi: mt76: mt7996: fix reading zeroed info->control.flags after mt76_tx_status_skb_add()

["lorenzo@kernel.org" <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7190 bytes --]

> On Sun, 2026-05-31 at 10:55 +0200, Lorenzo Bianconi wrote:
> > mt76_tx_status_skb_add() zeroes the mt76_tx_cb struct stored at
> > info->status.status_driver_data via memset(). Since info->control and
> > info->status are members of the same union in ieee80211_tx_info,
> > this overwrites info->control.flags.
> > In mt7996_tx_prepare_skb(), mt76_tx_status_skb_add() is called before
> > mt7996_mac_write_txwi(), which re-reads info->control.flags to
> > extract
> > IEEE80211_TX_CTRL_MLO_LINK. Because the field has been zeroed, the
> > link_id always resolves to 0 for frames using global_wcid, leading to
> > incorrect TXWI configuration.
> > Fix this by passing link_id as an explicit parameter to
> > mt7996_mac_write_txwi(). In mt7996_tx_prepare_skb(), the link_id is
> > already extracted from info->control.flags before the destructive
> > mt76_tx_status_skb_add() call. For the beacon and inband discovery
> > callers in mcu.c, use link_conf->link_id directly.
> > 
> > Fixes: f0b0b239b8f36 ("wifi: mt76: mt7996: rework
> > mt7996_mac_write_txwi() for MLO support")
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> > Changes in v2:
> > - Do not use link_id in mt7996_mac_write_txwi if it is
> > IEEE80211_LINK_UNSPECIFIED
> > - In mt7996_mac_write_txwi() rely on link_id calculated in
> >   mt7996_tx_prepare_skb().
> > - Link to v1:
> > https://lore.kernel.org/r/20260530-mt76_tx_status_skb_add-overwrite-fix-v1-1-e2c3151c391a@kernel.org
> >  
> > ---
> >  drivers/net/wireless/mediatek/mt76/mt7996/mac.c    | 14 ++++--------
> > --
> >  drivers/net/wireless/mediatek/mt76/mt7996/mcu.c    |  5 +++--
> >  drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h |  3 ++-
> >  3 files changed, 9 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
> > b/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
> > index c98446057282..95b3078d9667 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
> > @@ -856,7 +856,8 @@ mt7996_mac_write_txwi_80211(struct mt7996_dev
> > *dev, __le32 *txwi,
> >  void mt7996_mac_write_txwi(struct mt7996_dev *dev, __le32 *txwi,
> >  			   struct sk_buff *skb, struct mt76_wcid
> > *wcid,
> >  			   struct ieee80211_key_conf *key, int pid,
> > -			   enum mt76_txq_id qid, u32 changed)
> > +			   enum mt76_txq_id qid, u32 changed,
> > +			   unsigned int link_id)
> >  {
> >  	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb-
> > >data;
> >  	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> > @@ -866,7 +867,6 @@ void mt7996_mac_write_txwi(struct mt7996_dev
> > *dev, __le32 *txwi,
> >  	bool is_8023 = info->flags &
> > IEEE80211_TX_CTL_HW_80211_ENCAP;
> >  	struct mt76_vif_link *mlink = NULL;
> >  	struct mt7996_vif *mvif;
> > -	unsigned int link_id;
> >  	u16 tx_count = 15;
> >  	u32 val;
> >  	bool inband_disc = !!(changed &
> > (BSS_CHANGED_UNSOL_BCAST_PROBE_RESP |
> > @@ -874,17 +874,11 @@ void mt7996_mac_write_txwi(struct mt7996_dev
> > *dev, __le32 *txwi,
> >  	bool beacon = !!(changed & (BSS_CHANGED_BEACON |
> >  				    BSS_CHANGED_BEACON_ENABLED)) &&
> > (!inband_disc);
> >  
> > -	if (wcid != &dev->mt76.global_wcid)
> > -		link_id = wcid->link_id;
> > -	else
> > -		link_id = u32_get_bits(info->control.flags,
> > -				       IEEE80211_TX_CTRL_MLO_LINK);
> > -
> >  	mvif = vif ? (struct mt7996_vif *)vif->drv_priv : NULL;
> >  	if (mvif) {
> >  		if (wcid->offchannel)
> >  			mlink = rcu_dereference(mvif-
> > >mt76.offchannel_link);
> > -		if (!mlink)
> > +		if (!mlink && link_id != IEEE80211_LINK_UNSPECIFIED)
> >  			mlink = rcu_dereference(mvif-
> > >mt76.link[link_id]);
> >  	}
> >  
> > @@ -1096,7 +1090,7 @@ int mt7996_tx_prepare_skb(struct mt76_dev
> > *mdev, void *txwi_ptr,
> >  	/* Transmit non qos data by 802.11 header and need to fill
> > txd by host*/
> >  	if (!is_8023 || pid >= MT_PACKET_ID_FIRST)
> >  		mt7996_mac_write_txwi(dev, txwi_ptr, tx_info->skb,
> > wcid, key,
> > -				      pid, qid, 0);
> > +				      pid, qid, 0, link_id);
> >  
> >  	/* MT7996 and MT7992 require driver to provide the MAC TXP
> > for AddBA
> >  	 * req
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/mcu.c
> > b/drivers/net/wireless/mediatek/mt76/mt7996/mcu.c
> > index 8be40d60ad29..a14c63438923 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7996/mcu.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7996/mcu.c
> > @@ -3103,7 +3103,7 @@ mt7996_mcu_beacon_cont(struct mt7996_dev *dev,
> >  
> >  	buf = (u8 *)bcn + sizeof(*bcn);
> >  	mt7996_mac_write_txwi(dev, (__le32 *)buf, skb, wcid, NULL,
> > 0, 0,
> > -			      BSS_CHANGED_BEACON);
> > +			      BSS_CHANGED_BEACON, link_conf-
> > >link_id);
> >  
> >  	memcpy(buf + MT_TXD_SIZE, skb->data, skb->len);
> >  }
> > @@ -3249,7 +3249,8 @@ int mt7996_mcu_beacon_inband_discov(struct
> > mt7996_dev *dev,
> >  
> >  	buf = (u8 *)tlv + sizeof(*discov);
> >  
> > -	mt7996_mac_write_txwi(dev, (__le32 *)buf, skb, wcid, NULL,
> > 0, 0, changed);
> > +	mt7996_mac_write_txwi(dev, (__le32 *)buf, skb, wcid, NULL,
> > 0, 0,
> > +			      changed, link_conf->link_id);
> >  
> >  	memcpy(buf + MT_TXD_SIZE, skb->data, skb->len);
> >  
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h
> > b/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h
> > index 0dc4198fcf8b..0d6488522ba7 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h
> > @@ -874,7 +874,8 @@ void mt7996_mac_enable_nf(struct mt7996_dev *dev,
> > u8 band);
> >  void mt7996_mac_write_txwi(struct mt7996_dev *dev, __le32 *txwi,
> >  			   struct sk_buff *skb, struct mt76_wcid
> > *wcid,
> >  			   struct ieee80211_key_conf *key, int pid,
> > -			   enum mt76_txq_id qid, u32 changed);
> > +			   enum mt76_txq_id qid, u32 changed,
> > +			   unsigned int link_id);
> >  void mt7996_mac_update_beacons(struct mt7996_phy *phy);
> >  void mt7996_mac_set_coverage_class(struct mt7996_phy *phy);
> >  void mt7996_mac_work(struct work_struct *work);
> > 
> > ---
> > base-commit: 4913f44167cf35a9536e9eec7352e15b2de0c573
> > change-id: 20260530-mt76_tx_status_skb_add-overwrite-fix-85818a9bb31f
> > 
> > Best regards,
> > 
> > 
> We might expand flags further so this still doesn't solve the issue of
> flags being cleared - it only works for MLO flag. And the developers
> still won't easily notice that the flags are being cleared.

My opinion is we should consider just upstream code and then change it as soon
as you post this new feature upstream, but I will let Felix comments on it.
Moreover, the proposed approach aligns link_id used in mt7996_tx_prepare_skb()
to the one used in mt7996_mac_write_txwi() and fix a possible OOB bug in
mt7996_mac_write_txwi().

Regards,
Lorenzo

>  
> Our current approach is to move memset after mt7996_mac_write_txwi().
> It's more flexible and we don't have to constantly change the function
> parameters just for the flags.
> 
> Ryder
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]