* [PATCH] wifi: mt76: mt7996: avoid memset overwriting tx_info->control.flags
@ 2026-05-11 18:32 Ryder Lee
2026-05-15 16:29 ` Lorenzo Bianconi
0 siblings, 1 reply; 8+ messages in thread
From: Ryder Lee @ 2026-05-11 18:32 UTC (permalink / raw)
To: Felix Fietkau
Cc: linux-mediatek, linux-wireless, Shayne Chen, Roy Luo, Ryder Lee
mt76_tx_status_skb_add uses memset on status.status_driver_data, which
overwrite info->control.flags in ieee80211_tx_info. Copy tx_info before
calling mt76_tx_status_skb_add to ensure control fields are preserved
and remain valid for later processing.
Reported-By: Roy Luo <roy-ch.luo@mediatek.com>
Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
drivers/net/wireless/mediatek/mt76/mt7996/mac.c | 17 +++++++++--------
.../net/wireless/mediatek/mt76/mt7996/mt7996.h | 2 +-
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/mac.c b/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
index e2a83da3a09c..a59c14c8f2af 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
@@ -855,11 +855,12 @@ 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,
+ struct ieee80211_tx_info *tx_info, int pid,
enum mt76_txq_id qid, u32 changed)
{
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
- struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+ struct ieee80211_key_conf *key = tx_info ? tx_info->control.hw_key : NULL;
+ struct ieee80211_tx_info *info = tx_info ? tx_info : IEEE80211_SKB_CB(skb);
struct ieee80211_vif *vif = info->control.vif;
u8 band_idx = (info->hw_queue & MT_TX_HW_QUEUE_PHY) >> 2;
u8 p_fmt, q_idx, omac_idx = 0, wmm_idx = 0;
@@ -1006,15 +1007,15 @@ int mt7996_tx_prepare_skb(struct mt76_dev *mdev, void *txwi_ptr,
{
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx_info->skb->data;
struct mt7996_dev *dev = container_of(mdev, struct mt7996_dev, mt76);
- struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx_info->skb);
- struct ieee80211_key_conf *key = info->control.hw_key;
- struct ieee80211_vif *vif = info->control.vif;
+ struct ieee80211_tx_info info = *IEEE80211_SKB_CB(tx_info->skb);
+ struct ieee80211_key_conf *key = info.control.hw_key;
+ struct ieee80211_vif *vif = info.control.vif;
struct mt7996_vif *mvif = vif ? (struct mt7996_vif *)vif->drv_priv : NULL;
struct mt7996_sta *msta = sta ? (struct mt7996_sta *)sta->drv_priv : NULL;
struct mt76_vif_link *mlink = NULL;
struct mt76_txwi_cache *t;
int id, i, pid, nbuf = tx_info->nbuf - 1;
- bool is_8023 = info->flags & IEEE80211_TX_CTL_HW_80211_ENCAP;
+ bool is_8023 = info.flags & IEEE80211_TX_CTL_HW_80211_ENCAP;
__le32 *ptr = (__le32 *)txwi_ptr;
u8 *txwi = (u8 *)txwi_ptr;
u8 link_id;
@@ -1031,7 +1032,7 @@ int mt7996_tx_prepare_skb(struct mt76_dev *mdev, void *txwi_ptr,
link_id = (tid % 2) ? msta->seclink_id : msta->deflink_id;
} else {
- link_id = u32_get_bits(info->control.flags,
+ link_id = u32_get_bits(info.control.flags,
IEEE80211_TX_CTRL_MLO_LINK);
}
@@ -1095,7 +1096,7 @@ int mt7996_tx_prepare_skb(struct mt76_dev *mdev, void *txwi_ptr,
memset(txwi_ptr, 0, MT_TXD_SIZE);
/* 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,
+ mt7996_mac_write_txwi(dev, txwi_ptr, tx_info->skb, wcid, &info,
pid, qid, 0);
/* MT7996 and MT7992 require driver to provide the MAC TXP for AddBA
diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h b/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h
index bdcf72457954..319fcc4fb469 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h
+++ b/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h
@@ -854,7 +854,7 @@ void mt7996_mac_cca_stats_reset(struct mt7996_phy *phy);
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,
+ struct ieee80211_tx_info *tx_info, int pid,
enum mt76_txq_id qid, u32 changed);
void mt7996_mac_update_beacons(struct mt7996_phy *phy);
void mt7996_mac_set_coverage_class(struct mt7996_phy *phy);
--
2.45.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] wifi: mt76: mt7996: avoid memset overwriting tx_info->control.flags
2026-05-11 18:32 [PATCH] wifi: mt76: mt7996: avoid memset overwriting tx_info->control.flags Ryder Lee
@ 2026-05-15 16:29 ` Lorenzo Bianconi
2026-05-15 18:04 ` Cheng Hao Luo
0 siblings, 1 reply; 8+ messages in thread
From: Lorenzo Bianconi @ 2026-05-15 16:29 UTC (permalink / raw)
To: Ryder Lee
Cc: Felix Fietkau, linux-mediatek, linux-wireless, Shayne Chen,
Roy Luo
[-- Attachment #1: Type: text/plain, Size: 8864 bytes --]
> mt76_tx_status_skb_add uses memset on status.status_driver_data, which
> overwrite info->control.flags in ieee80211_tx_info. Copy tx_info before
> calling mt76_tx_status_skb_add to ensure control fields are preserved
> and remain valid for later processing.
>
> Reported-By: Roy Luo <roy-ch.luo@mediatek.com>
> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
Hi Ryder,
I have not completely got the issue here:
struct ieee80211_tx_info {
u32 flags; /* 0 4 */
u32 band:3; /* 4: 0 4 */
u32 status_data_idr:1; /* 4: 3 4 */
u32 status_data:13; /* 4: 4 4 */
u32 hw_queue:4; /* 4:17 4 */
u32 tx_time_est:10; /* 4:21 4 */
/* XXX 1 bit hole, try to pack */
union {
struct {
union {
struct {
struct ieee80211_tx_rate rates[4]; /* 8 12 */
s8 rts_cts_rate_idx; /* 20 1 */
u8 use_rts:1; /* 21: 0 1 */
u8 use_cts_prot:1; /* 21: 1 1 */
u8 short_preamble:1; /* 21: 2 1 */
u8 skip_table:1; /* 21: 3 1 */
u8 antennas:2; /* 21: 4 1 */
}; /* 8 14 */
long unsigned int jiffies; /* 8 8 */
}; /* 8 16 */
struct ieee80211_vif * vif; /* 24 8 */
struct ieee80211_key_conf * hw_key; /* 32 8 */
u32 flags; /* 40 4 */
codel_time_t enqueue_time; /* 44 4 */
} control; /* 8 40 */
struct {
u64 cookie; /* 8 8 */
} ack; /* 8 8 */
struct {
struct ieee80211_tx_rate rates[4]; /* 8 12 */
s32 ack_signal; /* 20 4 */
u8 ampdu_ack_len; /* 24 1 */
u8 ampdu_len; /* 25 1 */
u8 antenna; /* 26 1 */
u8 pad; /* 27 1 */
u16 tx_time; /* 28 2 */
u8 flags; /* 30 1 */
u8 pad2; /* 31 1 */
void * status_driver_data[2]; /* 32 16 */
} status; /* 8 40 */
struct {
struct ieee80211_tx_rate driver_rates[4]; /* 8 12 */
u8 pad[4]; /* 20 4 */
void * rate_driver_data[3]; /* 24 24 */
}; /* 8 40 */
void * driver_data[5]; /* 8 40 */
}; /* 8 40 */
/* size: 48, cachelines: 1, members: 7 */
/* sum members: 44 */
/* sum bitfield members: 31 bits, bit holes: 1, sum bit holes: 1 bits */
/* last cacheline: 48 bytes */
};
According to pahole, the size of the control inner union is actually 16 bytes
since the compiler adds 2 bytes of padding. Since mt76_tx_status_skb_add()
meset to 0 just mt76_tx_cb size (that is 16 bytes) I can't see how
control.flags is overwritten. Am I missing something?
struct mt76_tx_cb {
long unsigned int jiffies; /* 0 8 */
u16 wcid; /* 8 2 */
u8 pktid; /* 10 1 */
u8 flags; /* 11 1 */
/* size: 16, cachelines: 1, members: 4 */
/* padding: 4 */
/* last cacheline: 16 bytes */
};
Regards,
Lorenzo
> ---
> drivers/net/wireless/mediatek/mt76/mt7996/mac.c | 17 +++++++++--------
> .../net/wireless/mediatek/mt76/mt7996/mt7996.h | 2 +-
> 2 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/mac.c b/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
> index e2a83da3a09c..a59c14c8f2af 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
> @@ -855,11 +855,12 @@ 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,
> + struct ieee80211_tx_info *tx_info, int pid,
> enum mt76_txq_id qid, u32 changed)
> {
> struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
> - struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> + struct ieee80211_key_conf *key = tx_info ? tx_info->control.hw_key : NULL;
> + struct ieee80211_tx_info *info = tx_info ? tx_info : IEEE80211_SKB_CB(skb);
> struct ieee80211_vif *vif = info->control.vif;
> u8 band_idx = (info->hw_queue & MT_TX_HW_QUEUE_PHY) >> 2;
> u8 p_fmt, q_idx, omac_idx = 0, wmm_idx = 0;
> @@ -1006,15 +1007,15 @@ int mt7996_tx_prepare_skb(struct mt76_dev *mdev, void *txwi_ptr,
> {
> struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx_info->skb->data;
> struct mt7996_dev *dev = container_of(mdev, struct mt7996_dev, mt76);
> - struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx_info->skb);
> - struct ieee80211_key_conf *key = info->control.hw_key;
> - struct ieee80211_vif *vif = info->control.vif;
> + struct ieee80211_tx_info info = *IEEE80211_SKB_CB(tx_info->skb);
> + struct ieee80211_key_conf *key = info.control.hw_key;
> + struct ieee80211_vif *vif = info.control.vif;
> struct mt7996_vif *mvif = vif ? (struct mt7996_vif *)vif->drv_priv : NULL;
> struct mt7996_sta *msta = sta ? (struct mt7996_sta *)sta->drv_priv : NULL;
> struct mt76_vif_link *mlink = NULL;
> struct mt76_txwi_cache *t;
> int id, i, pid, nbuf = tx_info->nbuf - 1;
> - bool is_8023 = info->flags & IEEE80211_TX_CTL_HW_80211_ENCAP;
> + bool is_8023 = info.flags & IEEE80211_TX_CTL_HW_80211_ENCAP;
> __le32 *ptr = (__le32 *)txwi_ptr;
> u8 *txwi = (u8 *)txwi_ptr;
> u8 link_id;
> @@ -1031,7 +1032,7 @@ int mt7996_tx_prepare_skb(struct mt76_dev *mdev, void *txwi_ptr,
>
> link_id = (tid % 2) ? msta->seclink_id : msta->deflink_id;
> } else {
> - link_id = u32_get_bits(info->control.flags,
> + link_id = u32_get_bits(info.control.flags,
> IEEE80211_TX_CTRL_MLO_LINK);
> }
>
> @@ -1095,7 +1096,7 @@ int mt7996_tx_prepare_skb(struct mt76_dev *mdev, void *txwi_ptr,
> memset(txwi_ptr, 0, MT_TXD_SIZE);
> /* 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,
> + mt7996_mac_write_txwi(dev, txwi_ptr, tx_info->skb, wcid, &info,
> pid, qid, 0);
>
> /* MT7996 and MT7992 require driver to provide the MAC TXP for AddBA
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h b/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h
> index bdcf72457954..319fcc4fb469 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h
> @@ -854,7 +854,7 @@ void mt7996_mac_cca_stats_reset(struct mt7996_phy *phy);
> 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,
> + struct ieee80211_tx_info *tx_info, int pid,
> enum mt76_txq_id qid, u32 changed);
> void mt7996_mac_update_beacons(struct mt7996_phy *phy);
> void mt7996_mac_set_coverage_class(struct mt7996_phy *phy);
> --
> 2.45.2
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wifi: mt76: mt7996: avoid memset overwriting tx_info->control.flags
2026-05-15 16:29 ` Lorenzo Bianconi
@ 2026-05-15 18:04 ` Cheng Hao Luo
2026-05-18 12:31 ` Lorenzo Bianconi
0 siblings, 1 reply; 8+ messages in thread
From: Cheng Hao Luo @ 2026-05-15 18:04 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Ryder Lee, Felix Fietkau, linux-mediatek, linux-wireless,
Shayne Chen, Roy Luo
> struct ieee80211_tx_info {
> u32 flags; /* 0 4 */
> u32 band:3; /* 4: 0 4 */
> u32 status_data_idr:1; /* 4: 3 4 */
> u32 status_data:13; /* 4: 4 4 */
> u32 hw_queue:4; /* 4:17 4 */
> u32 tx_time_est:10; /* 4:21 4 */
>
> /* XXX 1 bit hole, try to pack */
>
> union {
> struct {
> union {
> struct {
> struct ieee80211_tx_rate rates[4]; /* 8 12 */
> s8 rts_cts_rate_idx; /* 20 1 */
> u8 use_rts:1; /* 21: 0 1 */
> u8 use_cts_prot:1; /* 21: 1 1 */
> u8 short_preamble:1; /* 21: 2 1 */
> u8 skip_table:1; /* 21: 3 1 */
> u8 antennas:2; /* 21: 4 1 */
> }; /* 8 14 */
> long unsigned int jiffies; /* 8 8 */
> }; /* 8 16 */
> struct ieee80211_vif * vif; /* 24 8 */
> struct ieee80211_key_conf * hw_key; /* 32 8 */
> u32 flags; /* 40 4 */
> codel_time_t enqueue_time; /* 44 4 */
> } control; /* 8 40 */
> struct {
> u64 cookie; /* 8 8 */
> } ack; /* 8 8 */
> struct {
> struct ieee80211_tx_rate rates[4]; /* 8 12 */
> s32 ack_signal; /* 20 4 */
> u8 ampdu_ack_len; /* 24 1 */
> u8 ampdu_len; /* 25 1 */
> u8 antenna; /* 26 1 */
> u8 pad; /* 27 1 */
> u16 tx_time; /* 28 2 */
> u8 flags; /* 30 1 */
> u8 pad2; /* 31 1 */
> void * status_driver_data[2]; /* 32 16 */
> } status; /* 8 40 */
> struct {
> struct ieee80211_tx_rate driver_rates[4]; /* 8 12 */
> u8 pad[4]; /* 20 4 */
> void * rate_driver_data[3]; /* 24 24 */
> }; /* 8 40 */
> void * driver_data[5]; /* 8 40 */
> }; /* 8 40 */
>
> /* size: 48, cachelines: 1, members: 7 */
> /* sum members: 44 */
> /* sum bitfield members: 31 bits, bit holes: 1, sum bit holes: 1 bits */
> /* last cacheline: 48 bytes */
> };
>
> According to pahole, the size of the control inner union is actually 16 bytes
> since the compiler adds 2 bytes of padding. Since mt76_tx_status_skb_add()
> meset to 0 just mt76_tx_cb size (that is 16 bytes) I can't see how
> control.flags is overwritten. Am I missing something?
>
> struct mt76_tx_cb {
> long unsigned int jiffies; /* 0 8 */
> u16 wcid; /* 8 2 */
> u8 pktid; /* 10 1 */
> u8 flags; /* 11 1 */
>
> /* size: 16, cachelines: 1, members: 4 */
> /* padding: 4 */
> /* last cacheline: 16 bytes */
> };
Hi Lorenzo,
The mt76_tx_cb is placed at status.status_driver_data (offset 32).
It overlaps with hw_key, flags and enqueue_time in the control union.
static inline struct mt76_tx_cb *mt76_tx_skb_cb(struct sk_buff *skb)
{
BUILD_BUG_ON(sizeof(struct mt76_tx_cb) >
sizeof(IEEE80211_SKB_CB(skb)->status.status_driver_data));
return ((void *)IEEE80211_SKB_CB(skb)->status.status_driver_data);
}
Regards,
Roy Luo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wifi: mt76: mt7996: avoid memset overwriting tx_info->control.flags
2026-05-15 18:04 ` Cheng Hao Luo
@ 2026-05-18 12:31 ` Lorenzo Bianconi
2026-05-18 18:23 ` Roy Luo
0 siblings, 1 reply; 8+ messages in thread
From: Lorenzo Bianconi @ 2026-05-18 12:31 UTC (permalink / raw)
To: Cheng Hao Luo
Cc: Ryder Lee, Felix Fietkau, linux-mediatek, linux-wireless,
Shayne Chen, Roy Luo
[-- Attachment #1: Type: text/plain, Size: 5379 bytes --]
On May 15, Cheng Hao Luo wrote:
> > struct ieee80211_tx_info {
> > u32 flags; /* 0 4 */
> > u32 band:3; /* 4: 0 4 */
> > u32 status_data_idr:1; /* 4: 3 4 */
> > u32 status_data:13; /* 4: 4 4 */
> > u32 hw_queue:4; /* 4:17 4 */
> > u32 tx_time_est:10; /* 4:21 4 */
> >
> > /* XXX 1 bit hole, try to pack */
> >
> > union {
> > struct {
> > union {
> > struct {
> > struct ieee80211_tx_rate rates[4]; /* 8 12 */
> > s8 rts_cts_rate_idx; /* 20 1 */
> > u8 use_rts:1; /* 21: 0 1 */
> > u8 use_cts_prot:1; /* 21: 1 1 */
> > u8 short_preamble:1; /* 21: 2 1 */
> > u8 skip_table:1; /* 21: 3 1 */
> > u8 antennas:2; /* 21: 4 1 */
> > }; /* 8 14 */
> > long unsigned int jiffies; /* 8 8 */
> > }; /* 8 16 */
> > struct ieee80211_vif * vif; /* 24 8 */
> > struct ieee80211_key_conf * hw_key; /* 32 8 */
> > u32 flags; /* 40 4 */
> > codel_time_t enqueue_time; /* 44 4 */
> > } control; /* 8 40 */
> > struct {
> > u64 cookie; /* 8 8 */
> > } ack; /* 8 8 */
> > struct {
> > struct ieee80211_tx_rate rates[4]; /* 8 12 */
> > s32 ack_signal; /* 20 4 */
> > u8 ampdu_ack_len; /* 24 1 */
> > u8 ampdu_len; /* 25 1 */
> > u8 antenna; /* 26 1 */
> > u8 pad; /* 27 1 */
> > u16 tx_time; /* 28 2 */
> > u8 flags; /* 30 1 */
> > u8 pad2; /* 31 1 */
> > void * status_driver_data[2]; /* 32 16 */
> > } status; /* 8 40 */
> > struct {
> > struct ieee80211_tx_rate driver_rates[4]; /* 8 12 */
> > u8 pad[4]; /* 20 4 */
> > void * rate_driver_data[3]; /* 24 24 */
> > }; /* 8 40 */
> > void * driver_data[5]; /* 8 40 */
> > }; /* 8 40 */
> >
> > /* size: 48, cachelines: 1, members: 7 */
> > /* sum members: 44 */
> > /* sum bitfield members: 31 bits, bit holes: 1, sum bit holes: 1 bits */
> > /* last cacheline: 48 bytes */
> > };
> >
> > According to pahole, the size of the control inner union is actually 16 bytes
> > since the compiler adds 2 bytes of padding. Since mt76_tx_status_skb_add()
> > meset to 0 just mt76_tx_cb size (that is 16 bytes) I can't see how
> > control.flags is overwritten. Am I missing something?
> >
> > struct mt76_tx_cb {
> > long unsigned int jiffies; /* 0 8 */
> > u16 wcid; /* 8 2 */
> > u8 pktid; /* 10 1 */
> > u8 flags; /* 11 1 */
> >
> > /* size: 16, cachelines: 1, members: 4 */
> > /* padding: 4 */
> > /* last cacheline: 16 bytes */
> > };
>
> Hi Lorenzo,
>
> The mt76_tx_cb is placed at status.status_driver_data (offset 32).
> It overlaps with hw_key, flags and enqueue_time in the control union.
>
> static inline struct mt76_tx_cb *mt76_tx_skb_cb(struct sk_buff *skb)
> {
> BUILD_BUG_ON(sizeof(struct mt76_tx_cb) >
> sizeof(IEEE80211_SKB_CB(skb)->status.status_driver_data));
> return ((void *)IEEE80211_SKB_CB(skb)->status.status_driver_data);
> }
Hi Roy,
I still do not understand since mt76_tx_status_skb_add() sets to 0 just sizeof
of mt76_tx_cb, that according to pahole is 16 bytes, so it can't overwrite
hw_key pointer (whose offset respect to the beginning of the control struct is
24, 32 - 8).
Regards,
Lorenzo
>
> Regards,
> Roy Luo
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wifi: mt76: mt7996: avoid memset overwriting tx_info->control.flags
2026-05-18 12:31 ` Lorenzo Bianconi
@ 2026-05-18 18:23 ` Roy Luo
2026-05-19 12:24 ` Lorenzo Bianconi
0 siblings, 1 reply; 8+ messages in thread
From: Roy Luo @ 2026-05-18 18:23 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Ryder Lee, Felix Fietkau, linux-mediatek, linux-wireless,
Shayne Chen, Roy Luo
On Mon, May 18, 2026 at 5:31 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> On May 15, Cheng Hao Luo wrote:
> > > struct ieee80211_tx_info {
> > > u32 flags; /* 0 4 */
> > > u32 band:3; /* 4: 0 4 */
> > > u32 status_data_idr:1; /* 4: 3 4 */
> > > u32 status_data:13; /* 4: 4 4 */
> > > u32 hw_queue:4; /* 4:17 4 */
> > > u32 tx_time_est:10; /* 4:21 4 */
> > >
> > > /* XXX 1 bit hole, try to pack */
> > >
> > > union {
> > > struct {
> > > union {
> > > struct {
> > > struct ieee80211_tx_rate rates[4]; /* 8 12 */
> > > s8 rts_cts_rate_idx; /* 20 1 */
> > > u8 use_rts:1; /* 21: 0 1 */
> > > u8 use_cts_prot:1; /* 21: 1 1 */
> > > u8 short_preamble:1; /* 21: 2 1 */
> > > u8 skip_table:1; /* 21: 3 1 */
> > > u8 antennas:2; /* 21: 4 1 */
> > > }; /* 8 14 */
> > > long unsigned int jiffies; /* 8 8 */
> > > }; /* 8 16 */
> > > struct ieee80211_vif * vif; /* 24 8 */
> > > struct ieee80211_key_conf * hw_key; /* 32 8 */
> > > u32 flags; /* 40 4 */
> > > codel_time_t enqueue_time; /* 44 4 */
> > > } control; /* 8 40 */
> > > struct {
> > > u64 cookie; /* 8 8 */
> > > } ack; /* 8 8 */
> > > struct {
> > > struct ieee80211_tx_rate rates[4]; /* 8 12 */
> > > s32 ack_signal; /* 20 4 */
> > > u8 ampdu_ack_len; /* 24 1 */
> > > u8 ampdu_len; /* 25 1 */
> > > u8 antenna; /* 26 1 */
> > > u8 pad; /* 27 1 */
> > > u16 tx_time; /* 28 2 */
> > > u8 flags; /* 30 1 */
> > > u8 pad2; /* 31 1 */
> > > void * status_driver_data[2]; /* 32 16 */
> > > } status; /* 8 40 */
> > > struct {
> > > struct ieee80211_tx_rate driver_rates[4]; /* 8 12 */
> > > u8 pad[4]; /* 20 4 */
> > > void * rate_driver_data[3]; /* 24 24 */
> > > }; /* 8 40 */
> > > void * driver_data[5]; /* 8 40 */
> > > }; /* 8 40 */
> > >
> > > /* size: 48, cachelines: 1, members: 7 */
> > > /* sum members: 44 */
> > > /* sum bitfield members: 31 bits, bit holes: 1, sum bit holes: 1 bits */
> > > /* last cacheline: 48 bytes */
> > > };
> > >
> > > According to pahole, the size of the control inner union is actually 16 bytes
> > > since the compiler adds 2 bytes of padding. Since mt76_tx_status_skb_add()
> > > meset to 0 just mt76_tx_cb size (that is 16 bytes) I can't see how
> > > control.flags is overwritten. Am I missing something?
> > >
> > > struct mt76_tx_cb {
> > > long unsigned int jiffies; /* 0 8 */
> > > u16 wcid; /* 8 2 */
> > > u8 pktid; /* 10 1 */
> > > u8 flags; /* 11 1 */
> > >
> > > /* size: 16, cachelines: 1, members: 4 */
> > > /* padding: 4 */
> > > /* last cacheline: 16 bytes */
> > > };
> >
> > Hi Lorenzo,
> >
> > The mt76_tx_cb is placed at status.status_driver_data (offset 32).
> > It overlaps with hw_key, flags and enqueue_time in the control union.
> >
> > static inline struct mt76_tx_cb *mt76_tx_skb_cb(struct sk_buff *skb)
> > {
> > BUILD_BUG_ON(sizeof(struct mt76_tx_cb) >
> > sizeof(IEEE80211_SKB_CB(skb)->status.status_driver_data));
> > return ((void *)IEEE80211_SKB_CB(skb)->status.status_driver_data);
> > }
>
> Hi Roy,
>
> I still do not understand since mt76_tx_status_skb_add() sets to 0 just sizeof
> of mt76_tx_cb, that according to pahole is 16 bytes, so it can't overwrite
> hw_key pointer (whose offset respect to the beginning of the control struct is
> 24, 32 - 8).
>
> Regards,
> Lorenzo
>
> >
> > Regards,
> > Roy Luo
Hi Lorenzo,
The mt76_tx_status_skb_add() memset zero the 16 bytes starting from
status.status_driver_data (please see the above inline function shared
in my last response) whose offset with respect to the beginning of
the control/status union is exactly 24 (32 - 8) instead of 0.
Regards,
Roy Luo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wifi: mt76: mt7996: avoid memset overwriting tx_info->control.flags
2026-05-18 18:23 ` Roy Luo
@ 2026-05-19 12:24 ` Lorenzo Bianconi
2026-05-19 20:42 ` Ryder Lee
0 siblings, 1 reply; 8+ messages in thread
From: Lorenzo Bianconi @ 2026-05-19 12:24 UTC (permalink / raw)
To: Roy Luo
Cc: Lorenzo Bianconi, Ryder Lee, Felix Fietkau, linux-mediatek,
linux-wireless, Shayne Chen, Roy Luo
>
> On Mon, May 18, 2026 at 5:31 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > On May 15, Cheng Hao Luo wrote:
> > > > struct ieee80211_tx_info {
> > > > u32 flags; /* 0 4 */
> > > > u32 band:3; /* 4: 0 4 */
> > > > u32 status_data_idr:1; /* 4: 3 4 */
> > > > u32 status_data:13; /* 4: 4 4 */
> > > > u32 hw_queue:4; /* 4:17 4 */
> > > > u32 tx_time_est:10; /* 4:21 4 */
> > > >
> > > > /* XXX 1 bit hole, try to pack */
> > > >
> > > > union {
> > > > struct {
> > > > union {
> > > > struct {
> > > > struct ieee80211_tx_rate rates[4]; /* 8 12 */
> > > > s8 rts_cts_rate_idx; /* 20 1 */
> > > > u8 use_rts:1; /* 21: 0 1 */
> > > > u8 use_cts_prot:1; /* 21: 1 1 */
> > > > u8 short_preamble:1; /* 21: 2 1 */
> > > > u8 skip_table:1; /* 21: 3 1 */
> > > > u8 antennas:2; /* 21: 4 1 */
> > > > }; /* 8 14 */
> > > > long unsigned int jiffies; /* 8 8 */
> > > > }; /* 8 16 */
> > > > struct ieee80211_vif * vif; /* 24 8 */
> > > > struct ieee80211_key_conf * hw_key; /* 32 8 */
> > > > u32 flags; /* 40 4 */
> > > > codel_time_t enqueue_time; /* 44 4 */
> > > > } control; /* 8 40 */
> > > > struct {
> > > > u64 cookie; /* 8 8 */
> > > > } ack; /* 8 8 */
> > > > struct {
> > > > struct ieee80211_tx_rate rates[4]; /* 8 12 */
> > > > s32 ack_signal; /* 20 4 */
> > > > u8 ampdu_ack_len; /* 24 1 */
> > > > u8 ampdu_len; /* 25 1 */
> > > > u8 antenna; /* 26 1 */
> > > > u8 pad; /* 27 1 */
> > > > u16 tx_time; /* 28 2 */
> > > > u8 flags; /* 30 1 */
> > > > u8 pad2; /* 31 1 */
> > > > void * status_driver_data[2]; /* 32 16 */
> > > > } status; /* 8 40 */
> > > > struct {
> > > > struct ieee80211_tx_rate driver_rates[4]; /* 8 12 */
> > > > u8 pad[4]; /* 20 4 */
> > > > void * rate_driver_data[3]; /* 24 24 */
> > > > }; /* 8 40 */
> > > > void * driver_data[5]; /* 8 40 */
> > > > }; /* 8 40 */
> > > >
> > > > /* size: 48, cachelines: 1, members: 7 */
> > > > /* sum members: 44 */
> > > > /* sum bitfield members: 31 bits, bit holes: 1, sum bit holes: 1 bits */
> > > > /* last cacheline: 48 bytes */
> > > > };
> > > >
> > > > According to pahole, the size of the control inner union is actually 16 bytes
> > > > since the compiler adds 2 bytes of padding. Since mt76_tx_status_skb_add()
> > > > meset to 0 just mt76_tx_cb size (that is 16 bytes) I can't see how
> > > > control.flags is overwritten. Am I missing something?
> > > >
> > > > struct mt76_tx_cb {
> > > > long unsigned int jiffies; /* 0 8 */
> > > > u16 wcid; /* 8 2 */
> > > > u8 pktid; /* 10 1 */
> > > > u8 flags; /* 11 1 */
> > > >
> > > > /* size: 16, cachelines: 1, members: 4 */
> > > > /* padding: 4 */
> > > > /* last cacheline: 16 bytes */
> > > > };
> > >
> > > Hi Lorenzo,
> > >
> > > The mt76_tx_cb is placed at status.status_driver_data (offset 32).
> > > It overlaps with hw_key, flags and enqueue_time in the control union.
> > >
> > > static inline struct mt76_tx_cb *mt76_tx_skb_cb(struct sk_buff *skb)
> > > {
> > > BUILD_BUG_ON(sizeof(struct mt76_tx_cb) >
> > > sizeof(IEEE80211_SKB_CB(skb)->status.status_driver_data));
> > > return ((void *)IEEE80211_SKB_CB(skb)->status.status_driver_data);
> > > }
> >
> > Hi Roy,
> >
> > I still do not understand since mt76_tx_status_skb_add() sets to 0 just sizeof
> > of mt76_tx_cb, that according to pahole is 16 bytes, so it can't overwrite
> > hw_key pointer (whose offset respect to the beginning of the control struct is
> > 24, 32 - 8).
> >
> > Regards,
> > Lorenzo
> >
> > >
> > > Regards,
> > > Roy Luo
>
> Hi Lorenzo,
>
> The mt76_tx_status_skb_add() memset zero the 16 bytes starting from
> status.status_driver_data (please see the above inline function shared
> in my last response) whose offset with respect to the beginning of
> the control/status union is exactly 24 (32 - 8) instead of 0.
>
> Regards,
> Roy Luo
Hi Roy,
I can see the issue now, I was confusing status.status_driver_data
with
driver_data. You are right, we have an issue here. However, copying
all the
ieee80211_tx_info struct seems a bit overkill, what do you think?
Moreover, we have the same issue for various chipsets (e.g. mt7925 and
mt7915). I guess we should try to find a global solution for the
problem.
Regards,
Lorenzo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wifi: mt76: mt7996: avoid memset overwriting tx_info->control.flags
2026-05-19 12:24 ` Lorenzo Bianconi
@ 2026-05-19 20:42 ` Ryder Lee
2026-05-22 20:35 ` Sean Wang
0 siblings, 1 reply; 8+ messages in thread
From: Ryder Lee @ 2026-05-19 20:42 UTC (permalink / raw)
To: lorenzo.bianconi83@gmail.com, roychl666@gmail.com
Cc: linux-wireless@vger.kernel.org, nbd@nbd.name, lorenzo@kernel.org,
Shayne Chen (陳軒丞),
linux-mediatek@lists.infradead.org, Roy-CH Luo
On Tue, 2026-05-19 at 14:24 +0200, Lorenzo Bianconi wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> >
> > On Mon, May 18, 2026 at 5:31 AM Lorenzo Bianconi
> > <lorenzo@kernel.org> wrote:
> > >
> > > On May 15, Cheng Hao Luo wrote:
> > > > > struct ieee80211_tx_info {
> > > > > u32 flags;
> > > > > /* 0 4 */
> > > > > u32 band:3;
> > > > > /* 4: 0 4 */
> > > > > u32 status_data_idr:1;
> > > > > /* 4: 3 4 */
> > > > > u32 status_data:13;
> > > > > /* 4: 4 4 */
> > > > > u32 hw_queue:4;
> > > > > /* 4:17 4 */
> > > > > u32 tx_time_est:10;
> > > > > /* 4:21 4 */
> > > > >
> > > > > /* XXX 1 bit hole, try to pack */
> > > > >
> > > > > union {
> > > > > struct {
> > > > > union {
> > > > > struct {
> > > > > struct
> > > > > ieee80211_tx_rate rates[4]; /* 8 12 */
> > > > > s8
> > > > > rts_cts_rate_idx; /* 20 1 */
> > > > > u8 use_rts:1;
> > > > > /* 21: 0 1 */
> > > > > u8
> > > > > use_cts_prot:1; /* 21: 1 1 */
> > > > > u8
> > > > > short_preamble:1; /* 21: 2 1 */
> > > > > u8 skip_table:1;
> > > > > /* 21: 3 1 */
> > > > > u8 antennas:2;
> > > > > /* 21: 4 1 */
> > > > > };
> > > > > /* 8 14 */
> > > > > long unsigned int jiffies;
> > > > > /* 8 8 */
> > > > > };
> > > > > /* 8 16 */
> > > > > struct ieee80211_vif * vif;
> > > > > /* 24 8 */
> > > > > struct ieee80211_key_conf * hw_key;
> > > > > /* 32 8 */
> > > > > u32 flags;
> > > > > /* 40 4 */
> > > > > codel_time_t enqueue_time;
> > > > > /* 44 4 */
> > > > > } control;
> > > > > /* 8 40 */
> > > > > struct {
> > > > > u64 cookie;
> > > > > /* 8 8 */
> > > > > } ack;
> > > > > /* 8 8 */
> > > > > struct {
> > > > > struct ieee80211_tx_rate rates[4];
> > > > > /* 8 12 */
> > > > > s32 ack_signal;
> > > > > /* 20 4 */
> > > > > u8 ampdu_ack_len;
> > > > > /* 24 1 */
> > > > > u8 ampdu_len;
> > > > > /* 25 1 */
> > > > > u8 antenna;
> > > > > /* 26 1 */
> > > > > u8 pad;
> > > > > /* 27 1 */
> > > > > u16 tx_time;
> > > > > /* 28 2 */
> > > > > u8 flags;
> > > > > /* 30 1 */
> > > > > u8 pad2;
> > > > > /* 31 1 */
> > > > > void * status_driver_data[2];
> > > > > /* 32 16 */
> > > > > } status;
> > > > > /* 8 40 */
> > > > > struct {
> > > > > struct ieee80211_tx_rate
> > > > > driver_rates[4]; /* 8 12 */
> > > > > u8 pad[4];
> > > > > /* 20 4 */
> > > > > void * rate_driver_data[3];
> > > > > /* 24 24 */
> > > > > };
> > > > > /* 8 40 */
> > > > > void * driver_data[5];
> > > > > /* 8 40 */
> > > > > };
> > > > > /* 8 40 */
> > > > >
> > > > > /* size: 48, cachelines: 1, members: 7 */
> > > > > /* sum members: 44 */
> > > > > /* sum bitfield members: 31 bits, bit holes: 1, sum
> > > > > bit holes: 1 bits */
> > > > > /* last cacheline: 48 bytes */
> > > > > };
> > > > >
> > > > > According to pahole, the size of the control inner union is
> > > > > actually 16 bytes
> > > > > since the compiler adds 2 bytes of padding. Since
> > > > > mt76_tx_status_skb_add()
> > > > > meset to 0 just mt76_tx_cb size (that is 16 bytes) I can't
> > > > > see how
> > > > > control.flags is overwritten. Am I missing something?
> > > > >
> > > > > struct mt76_tx_cb {
> > > > > long unsigned int jiffies;
> > > > > /* 0 8 */
> > > > > u16 wcid;
> > > > > /* 8 2 */
> > > > > u8 pktid;
> > > > > /* 10 1 */
> > > > > u8 flags;
> > > > > /* 11 1 */
> > > > >
> > > > > /* size: 16, cachelines: 1, members: 4 */
> > > > > /* padding: 4 */
> > > > > /* last cacheline: 16 bytes */
> > > > > };
> > > >
> > > > Hi Lorenzo,
> > > >
> > > > The mt76_tx_cb is placed at status.status_driver_data (offset
> > > > 32).
> > > > It overlaps with hw_key, flags and enqueue_time in the control
> > > > union.
> > > >
> > > > static inline struct mt76_tx_cb *mt76_tx_skb_cb(struct sk_buff
> > > > *skb)
> > > > {
> > > > BUILD_BUG_ON(sizeof(struct mt76_tx_cb) >
> > > > sizeof(IEEE80211_SKB_CB(skb)->status.status_driver_data));
> > > > return ((void *)IEEE80211_SKB_CB(skb)-
> > > > >status.status_driver_data);
> > > > }
> > >
> > > Hi Roy,
> > >
> > > I still do not understand since mt76_tx_status_skb_add() sets to
> > > 0 just sizeof
> > > of mt76_tx_cb, that according to pahole is 16 bytes, so it can't
> > > overwrite
> > > hw_key pointer (whose offset respect to the beginning of the
> > > control struct is
> > > 24, 32 - 8).
> > >
> > > Regards,
> > > Lorenzo
> > >
> > > >
> > > > Regards,
> > > > Roy Luo
> >
> > Hi Lorenzo,
> >
> > The mt76_tx_status_skb_add() memset zero the 16 bytes starting from
> > status.status_driver_data (please see the above inline function
> > shared
> > in my last response) whose offset with respect to the beginning of
> > the control/status union is exactly 24 (32 - 8) instead of 0.
> >
> > Regards,
> > Roy Luo
>
> Hi Roy,
>
> I can see the issue now, I was confusing status.status_driver_data
> with
> driver_data. You are right, we have an issue here. However, copying
> all the
> ieee80211_tx_info struct seems a bit overkill, what do you think?
> Moreover, we have the same issue for various chipsets (e.g. mt7925
> and
> mt7915). I guess we should try to find a global solution for the
> problem.
>
> Regards,
> Lorenzo
What about adding an helper for cb operation?
+void
+mt76_tx_status_skb_cb_add(struct mt76_dev *dev, struct sk_buff *skb,
+ struct mt76_wcid *wcid, int pid)
+{
+ struct mt76_tx_cb *cb = mt76_tx_skb_cb(skb);
+
+ memset(cb, 0, sizeof(*cb));
+
+ spin_lock_bh(&dev->status_lock);
+ cb->wcid = wcid->idx;
+ cb->pktid = pid;
+ spin_unlock_bh(&dev->status_lock);
+}
+EXPORT_SYMBOL_GPL(mt76_tx_status_skb_cb_add);
And add this for each chipset.
index 061ab66..d0b67a2 100644
--- a/mt7996/mac.c
+++ b/mt7996/mac.c
@@ -1108,6 +1108,7 @@ int mt7996_tx_prepare_skb(struct mt76_dev *mdev,
void *txwi_ptr,
if (!is_8023 || pid >= MT_PACKET_ID_FIRST)
mt7996_mac_write_txwi(dev, txwi_ptr, tx_info->skb,
wcid, key,
pid, qid, 0);
+ mt76_tx_status_skb_cb_add(dev, tx_info->skb, wcid, pid);
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] wifi: mt76: mt7996: avoid memset overwriting tx_info->control.flags
2026-05-19 20:42 ` Ryder Lee
@ 2026-05-22 20:35 ` Sean Wang
0 siblings, 0 replies; 8+ messages in thread
From: Sean Wang @ 2026-05-22 20:35 UTC (permalink / raw)
To: Ryder Lee
Cc: lorenzo.bianconi83@gmail.com, roychl666@gmail.com,
linux-wireless@vger.kernel.org, nbd@nbd.name, lorenzo@kernel.org,
Shayne Chen (陳軒丞),
linux-mediatek@lists.infradead.org, Roy-CH Luo
Hi,
On Tue, May 19, 2026 at 3:43 PM Ryder Lee <Ryder.Lee@mediatek.com> wrote:
>
> On Tue, 2026-05-19 at 14:24 +0200, Lorenzo Bianconi wrote:
> >
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >
> >
> > >
> > > On Mon, May 18, 2026 at 5:31 AM Lorenzo Bianconi
> > > <lorenzo@kernel.org> wrote:
> > > >
> > > > On May 15, Cheng Hao Luo wrote:
> > > > > > struct ieee80211_tx_info {
> > > > > > u32 flags;
> > > > > > /* 0 4 */
> > > > > > u32 band:3;
> > > > > > /* 4: 0 4 */
> > > > > > u32 status_data_idr:1;
> > > > > > /* 4: 3 4 */
> > > > > > u32 status_data:13;
> > > > > > /* 4: 4 4 */
> > > > > > u32 hw_queue:4;
> > > > > > /* 4:17 4 */
> > > > > > u32 tx_time_est:10;
> > > > > > /* 4:21 4 */
> > > > > >
> > > > > > /* XXX 1 bit hole, try to pack */
> > > > > >
> > > > > > union {
> > > > > > struct {
> > > > > > union {
> > > > > > struct {
> > > > > > struct
> > > > > > ieee80211_tx_rate rates[4]; /* 8 12 */
> > > > > > s8
> > > > > > rts_cts_rate_idx; /* 20 1 */
> > > > > > u8 use_rts:1;
> > > > > > /* 21: 0 1 */
> > > > > > u8
> > > > > > use_cts_prot:1; /* 21: 1 1 */
> > > > > > u8
> > > > > > short_preamble:1; /* 21: 2 1 */
> > > > > > u8 skip_table:1;
> > > > > > /* 21: 3 1 */
> > > > > > u8 antennas:2;
> > > > > > /* 21: 4 1 */
> > > > > > };
> > > > > > /* 8 14 */
> > > > > > long unsigned int jiffies;
> > > > > > /* 8 8 */
> > > > > > };
> > > > > > /* 8 16 */
> > > > > > struct ieee80211_vif * vif;
> > > > > > /* 24 8 */
> > > > > > struct ieee80211_key_conf * hw_key;
> > > > > > /* 32 8 */
> > > > > > u32 flags;
> > > > > > /* 40 4 */
> > > > > > codel_time_t enqueue_time;
> > > > > > /* 44 4 */
> > > > > > } control;
> > > > > > /* 8 40 */
> > > > > > struct {
> > > > > > u64 cookie;
> > > > > > /* 8 8 */
> > > > > > } ack;
> > > > > > /* 8 8 */
> > > > > > struct {
> > > > > > struct ieee80211_tx_rate rates[4];
> > > > > > /* 8 12 */
> > > > > > s32 ack_signal;
> > > > > > /* 20 4 */
> > > > > > u8 ampdu_ack_len;
> > > > > > /* 24 1 */
> > > > > > u8 ampdu_len;
> > > > > > /* 25 1 */
> > > > > > u8 antenna;
> > > > > > /* 26 1 */
> > > > > > u8 pad;
> > > > > > /* 27 1 */
> > > > > > u16 tx_time;
> > > > > > /* 28 2 */
> > > > > > u8 flags;
> > > > > > /* 30 1 */
> > > > > > u8 pad2;
> > > > > > /* 31 1 */
> > > > > > void * status_driver_data[2];
> > > > > > /* 32 16 */
> > > > > > } status;
> > > > > > /* 8 40 */
> > > > > > struct {
> > > > > > struct ieee80211_tx_rate
> > > > > > driver_rates[4]; /* 8 12 */
> > > > > > u8 pad[4];
> > > > > > /* 20 4 */
> > > > > > void * rate_driver_data[3];
> > > > > > /* 24 24 */
> > > > > > };
> > > > > > /* 8 40 */
> > > > > > void * driver_data[5];
> > > > > > /* 8 40 */
> > > > > > };
> > > > > > /* 8 40 */
> > > > > >
> > > > > > /* size: 48, cachelines: 1, members: 7 */
> > > > > > /* sum members: 44 */
> > > > > > /* sum bitfield members: 31 bits, bit holes: 1, sum
> > > > > > bit holes: 1 bits */
> > > > > > /* last cacheline: 48 bytes */
> > > > > > };
> > > > > >
> > > > > > According to pahole, the size of the control inner union is
> > > > > > actually 16 bytes
> > > > > > since the compiler adds 2 bytes of padding. Since
> > > > > > mt76_tx_status_skb_add()
> > > > > > meset to 0 just mt76_tx_cb size (that is 16 bytes) I can't
> > > > > > see how
> > > > > > control.flags is overwritten. Am I missing something?
> > > > > >
> > > > > > struct mt76_tx_cb {
> > > > > > long unsigned int jiffies;
> > > > > > /* 0 8 */
> > > > > > u16 wcid;
> > > > > > /* 8 2 */
> > > > > > u8 pktid;
> > > > > > /* 10 1 */
> > > > > > u8 flags;
> > > > > > /* 11 1 */
> > > > > >
> > > > > > /* size: 16, cachelines: 1, members: 4 */
> > > > > > /* padding: 4 */
> > > > > > /* last cacheline: 16 bytes */
> > > > > > };
> > > > >
> > > > > Hi Lorenzo,
> > > > >
> > > > > The mt76_tx_cb is placed at status.status_driver_data (offset
> > > > > 32).
> > > > > It overlaps with hw_key, flags and enqueue_time in the control
> > > > > union.
> > > > >
> > > > > static inline struct mt76_tx_cb *mt76_tx_skb_cb(struct sk_buff
> > > > > *skb)
> > > > > {
> > > > > BUILD_BUG_ON(sizeof(struct mt76_tx_cb) >
> > > > > sizeof(IEEE80211_SKB_CB(skb)->status.status_driver_data));
> > > > > return ((void *)IEEE80211_SKB_CB(skb)-
> > > > > >status.status_driver_data);
> > > > > }
> > > >
> > > > Hi Roy,
> > > >
> > > > I still do not understand since mt76_tx_status_skb_add() sets to
> > > > 0 just sizeof
> > > > of mt76_tx_cb, that according to pahole is 16 bytes, so it can't
> > > > overwrite
> > > > hw_key pointer (whose offset respect to the beginning of the
> > > > control struct is
> > > > 24, 32 - 8).
> > > >
> > > > Regards,
> > > > Lorenzo
> > > >
> > > > >
> > > > > Regards,
> > > > > Roy Luo
> > >
> > > Hi Lorenzo,
> > >
> > > The mt76_tx_status_skb_add() memset zero the 16 bytes starting from
> > > status.status_driver_data (please see the above inline function
> > > shared
> > > in my last response) whose offset with respect to the beginning of
> > > the control/status union is exactly 24 (32 - 8) instead of 0.
> > >
> > > Regards,
> > > Roy Luo
> >
> > Hi Roy,
> >
> > I can see the issue now, I was confusing status.status_driver_data
> > with
> > driver_data. You are right, we have an issue here. However, copying
> > all the
> > ieee80211_tx_info struct seems a bit overkill, what do you think?
> > Moreover, we have the same issue for various chipsets (e.g. mt7925
> > and
> > mt7915). I guess we should try to find a global solution for the
> > problem.
> >
> > Regards,
> > Lorenzo
>
> What about adding an helper for cb operation?
>
> +void
> +mt76_tx_status_skb_cb_add(struct mt76_dev *dev, struct sk_buff *skb,
> + struct mt76_wcid *wcid, int pid)
> +{
> + struct mt76_tx_cb *cb = mt76_tx_skb_cb(skb);
> +
> + memset(cb, 0, sizeof(*cb));
> +
> + spin_lock_bh(&dev->status_lock);
> + cb->wcid = wcid->idx;
> + cb->pktid = pid;
> + spin_unlock_bh(&dev->status_lock);
> +}
> +EXPORT_SYMBOL_GPL(mt76_tx_status_skb_cb_add);
>
> And add this for each chipset.
>
> index 061ab66..d0b67a2 100644
> --- a/mt7996/mac.c
> +++ b/mt7996/mac.c
> @@ -1108,6 +1108,7 @@ int mt7996_tx_prepare_skb(struct mt76_dev *mdev,
> void *txwi_ptr,
> if (!is_8023 || pid >= MT_PACKET_ID_FIRST)
> mt7996_mac_write_txwi(dev, txwi_ptr, tx_info->skb,
> wcid, key,
> pid, qid, 0);
> + mt76_tx_status_skb_cb_add(dev, tx_info->skb, wcid, pid);
>
>
I would prefer to split the cb init logic out of
mt76_tx_status_skb_add() and add a dedicated helper:
/*
* Must be called only after all TXWI/TXP code has consumed
* IEEE80211_SKB_CB(skb)->control, since this switches skb->cb to the
* mt76 tx status tracking overlay.
*/
void
mt76_tx_status_skb_init(struct mt76_dev *dev, struct mt76_wcid *wcid,
struct sk_buff *skb, int pktid)
{
struct mt76_tx_cb *cb = mt76_tx_skb_cb(skb);
memset(cb, 0, sizeof(*cb));
if (wcid)
cb->wcid = wcid->idx;
cb->pktid = pktid;
}
EXPORT_SYMBOL_GPL(mt76_tx_status_skb_init);
Then
pid = mt76_tx_status_skb_add(...);
... write TXWI/TXP while info->control is still valid ...
mt76_tx_status_skb_init(...); to fix all drivers under mt76/
Reasons:
1. The helper does not need its own spinlock.
mt76_tx_status_skb_add() already protects the idr/list update with
status_lock; this helper only initializes the skb-local cb overlay.
2. wcid can be NULL on some paths, so the helper should guard against that.
3. mt76_tx_status_skb_init() should be called after
mt76_tx_status_skb_add() and after all TXWI/TXP write helpers in each
mt76 driver. The same skb cb aliasing issue can potentially affect all
drivers that consume IEEE80211_SKB_CB(skb)->control after calling
mt76_tx_status_skb_add().
4. The comment is useful because the ordering requirement is easy
to miss: mt76_tx_status_skb_init() clears the skb cb area used by
IEEE80211_SKB_CB(skb)->control, so it must run only after TXWI/TXP
setup has finished reading that control data.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-05-22 20:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-11 18:32 [PATCH] wifi: mt76: mt7996: avoid memset overwriting tx_info->control.flags Ryder Lee
2026-05-15 16:29 ` Lorenzo Bianconi
2026-05-15 18:04 ` Cheng Hao Luo
2026-05-18 12:31 ` Lorenzo Bianconi
2026-05-18 18:23 ` Roy Luo
2026-05-19 12:24 ` Lorenzo Bianconi
2026-05-19 20:42 ` Ryder Lee
2026-05-22 20:35 ` Sean Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox