Skip to contentSkip to conversation

mt76: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

PATCH In review Shuah Khan 3 participants 15 messages Feb 12, 2021 – Feb 17, 2021 1 message not in the archive

Subsystems (6)
Networking Network drivers Arm MediaTek Qualcomm Atheros ATH9K wireless driver +2 more subsystems
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Shuah Khan

author
#1 lore raw

PATCH 1/2 ath9k: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and the resulting pointer is only valid under RCU lock as well.

Fix ath_tx_process_buffer() to hold RCU read lock before it calls ieee80211_find_sta_by_ifaddr() and release it when the resulting pointer is no longer needed.

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Signed-off-by Shuah Khan <[email protected]>

---
- Note: This patch is compile tested. I don't have access to hardware.

 drivers/net/wireless/ath/ath9k/xmit.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/net/wireless/ath/ath9k/xmit.c +5 -0
index e60d4737fc6e..1d36aae3f7b6 100644
@@ -701,6 +701,9 @@ static void ath_tx_process_buffer(struct ath_softc *sc, struct ath_txq *txq,
 					     ts->ts_rateindex);
 
 	hdr = (struct ieee80211_hdr *) bf->bf_mpdu->data;
+
+	rcu_read_lock();
+
 	sta = ieee80211_find_sta_by_ifaddr(hw, hdr->addr1, hdr->addr2);
 	if (sta) {
 		struct ath_node *an = (struct ath_node *)sta->drv_priv;

@@ -725,6 +728,8 @@ static void ath_tx_process_buffer(struct ath_softc *sc, struct ath_txq *txq,
 
 	if (!flush)
 		ath_txq_schedule(sc, txq);
+
+	rcu_read_unlock();
 }
 
 static bool ath_lookup_legacy(struct ath_buf *bf)
Signature · 1 line
2.27.0
author
#2 lore raw

PATCH 1/1 mt76: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and the resulting pointer is only valid under RCU lock as well.

Fix mt76_check_sta() to hold RCU read lock before it calls ieee80211_find_sta_by_ifaddr() and release it when the resulting pointer is no longer needed.

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Signed-off-by Shuah Khan <[email protected]>

---
- Note: This patch is compile tested. I don't have access to hardware.

 drivers/net/wireless/mediatek/mt76/mac80211.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
drivers/net/wireless/mediatek/mt76/mac80211.c +11 -5
index a840396f2c74..3c732da2a53f 100644
@@ -867,6 +867,9 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 	bool ps;
 
 	hw = mt76_phy_hw(dev, status->ext_phy);
+
+	rcu_read_lock();
+
 	if (ieee80211_is_pspoll(hdr->frame_control) && !wcid) {
 		sta = ieee80211_find_sta_by_ifaddr(hw, hdr->addr2, NULL);
 		if (sta)

@@ -876,7 +879,7 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 	mt76_airtime_check(dev, skb);
 
 	if (!wcid || !wcid->sta)
-		return;
+		goto exit;
 
 	sta = container_of((void *)wcid, struct ieee80211_sta, drv_priv);
 

@@ -886,17 +889,17 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 	wcid->inactive_count = 0;
 
 	if (!test_bit(MT_WCID_FLAG_CHECK_PS, &wcid->flags))
-		return;
+		goto exit;
 
 	if (ieee80211_is_pspoll(hdr->frame_control)) {
 		ieee80211_sta_pspoll(sta);
-		return;
+		goto exit;
 	}
 
 	if (ieee80211_has_morefrags(hdr->frame_control) ||
 	    !(ieee80211_is_mgmt(hdr->frame_control) ||
 	      ieee80211_is_data(hdr->frame_control)))
-		return;
+		goto exit;
 
 	ps = ieee80211_has_pm(hdr->frame_control);
 

@@ -905,7 +908,7 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 		ieee80211_sta_uapsd_trigger(sta, status->tid);
 
 	if (!!test_bit(MT_WCID_FLAG_PS, &wcid->flags) == ps)
-		return;
+		goto exit;
 
 	if (ps)
 		set_bit(MT_WCID_FLAG_PS, &wcid->flags);

@@ -914,6 +917,9 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 
 	dev->drv->sta_ps(dev, sta, ps);
 	ieee80211_sta_ps_transition(sta, ps);
+
+exit:
+	rcu_read_unlock();
 }
 
 void mt76_rx_complete(struct mt76_dev *dev, struct sk_buff_head *frames,
Signature · 7 lines
2.27.0


_______________________________________________
Linux-mediatek mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-mediatek
maintainer
#3 lore raw
Quoted 7 lines · Shuah Khan

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and the resulting pointer is only valid under RCU lock as well.

Fix mt76_check_sta() to hold RCU read lock before it calls ieee80211_find_sta_by_ifaddr() and release it when the resulting pointer is no longer needed.

Quoted 6 lines

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Signed-off-by Shuah Khan <[email protected]>

If I'm not mistaken, this patch is unnecessary. mt76_check_sta is only called from mt76_rx_poll_complete, which itself is only called under RCU lock.

- Felix

_______________________________________________
Linux-mediatek mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-mediatek

author
#4 lore raw
Quoted 14 lines · Felix Fietkau
Quoted 12 lines · Shuah Khan

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and the resulting pointer is only valid under RCU lock as well.

Fix mt76_check_sta() to hold RCU read lock before it calls ieee80211_find_sta_by_ifaddr() and release it when the resulting pointer is no longer needed.

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Quoted 5 lines
Quoted 1 line
Signed-off-by Shuah Khan <[email protected]>

If I'm not mistaken, this patch is unnecessary. mt76_check_sta is only called from mt76_rx_poll_complete, which itself is only called under RCU lock.

Yes. You are right. I checked the caller of this routine and didn't go further up. :)

thanks,
-- Shuah

_______________________________________________
Linux-mediatek mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-mediatek

author
#5 lore raw

PATCH 1/1 rtw88: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

In reply to Shuah Khan

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and the resulting pointer is only valid under RCU lock as well.

Fix rtw_rx_addr_match_iter() to hold RCU read lock before it calls ieee80211_find_sta_by_ifaddr() and release it when the resulting pointer is no longer needed.

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Signed-off-by Shuah Khan <[email protected]>

---
- Note: This patch is compile tested. I don't have access to hardware.

 drivers/net/wireless/realtek/rtw88/rx.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/net/wireless/realtek/rtw88/rx.c +6 -0
index 7087e385a9b3..4ab3d3e2bfab 100644
@@ -111,6 +111,9 @@ static void rtw_rx_addr_match_iter(void *data, u8 *mac,
 		return;
 
 	rtw_rx_phy_stat(rtwdev, pkt_stat, hdr);
+
+	rcu_read_lock();
+
 	sta = ieee80211_find_sta_by_ifaddr(rtwdev->hw, hdr->addr2,
 					   vif->addr);
 	if (!sta)

@@ -118,6 +121,9 @@ static void rtw_rx_addr_match_iter(void *data, u8 *mac,
 
 	si = (struct rtw_sta_info *)sta->drv_priv;
 	ewma_rssi_add(&si->avg_rssi, pkt_stat->rssi);
+
+exit:
+	rcu_read_unlock();
 }
 
 static void rtw_rx_addr_match(struct rtw_dev *rtwdev,
Signature · 1 line
2.27.0
maintainer
#6 lore raw

Re: [PATCH] rtw88: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

Quoted 7 lines · Shuah Khan

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and the resulting pointer is only valid under RCU lock as well.

Fix rtw_rx_addr_match_iter() to hold RCU read lock before it calls ieee80211_find_sta_by_ifaddr() and release it when the resulting pointer is no longer needed.

Quoted 6 lines

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Signed-off-by Shuah Khan <[email protected]>

This one also seems unnecessary. rtw_rx_addr_match_iter is called by ieee80211_iterate_active_interfaces_atomic, which acquires the RCU read lock before calling it.

- Felix

maintainer
#7 lore raw

Re: [PATCH 1/2] ath9k: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

In reply to Shuah Khan
Quoted 7 lines · Shuah Khan

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and the resulting pointer is only valid under RCU lock as well.

Fix ath_tx_process_buffer() to hold RCU read lock before it calls ieee80211_find_sta_by_ifaddr() and release it when the resulting pointer is no longer needed.

Quoted 6 lines

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Signed-off-by Shuah Khan <[email protected]>

This patch seems unnecessary as well. All callers of ath_tx_process_buffer seem to hold the RCU read lock already.

- Felix

author
#8 lore raw

PATCH 2/2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference

In reply to a message not in the archive

ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr() return pointer (sta) outside null check. Fix it by moving the code block under the null check.

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Signed-off-by Shuah Khan <[email protected]>

---
- Note: This patch is compile tested. I don't have access to hardware.

 drivers/net/wireless/ath/ath9k/xmit.c | 28 +++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)
drivers/net/wireless/ath/ath9k/xmit.c +16 -12
index 1d36aae3f7b6..735858144e3a 100644
@@ -711,20 +711,24 @@ static void ath_tx_process_buffer(struct ath_softc *sc, struct ath_txq *txq,
 		ath_tx_count_airtime(sc, sta, bf, ts, tid->tidno);
 		if (ts->ts_status & (ATH9K_TXERR_FILT | ATH9K_TXERR_XRETRY))
 			tid->clear_ps_filter = true;
-	}
 
-	if (!bf_isampdu(bf)) {
-		if (!flush) {
-			info = IEEE80211_SKB_CB(bf->bf_mpdu);
-			memcpy(info->control.rates, bf->rates,
-			       sizeof(info->control.rates));
-			ath_tx_rc_status(sc, bf, ts, 1, txok ? 0 : 1, txok);
-			ath_dynack_sample_tx_ts(sc->sc_ah, bf->bf_mpdu, ts,
-						sta);
+		if (!bf_isampdu(bf)) {
+			if (!flush) {
+				info = IEEE80211_SKB_CB(bf->bf_mpdu);
+				memcpy(info->control.rates, bf->rates,
+				       sizeof(info->control.rates));
+				ath_tx_rc_status(sc, bf, ts, 1,
+						 txok ? 0 : 1, txok);
+				ath_dynack_sample_tx_ts(sc->sc_ah,
+							bf->bf_mpdu, ts, sta);
+			}
+			ath_tx_complete_buf(sc, bf, txq, bf_head, sta,
+					    ts, txok);
+		} else {
+			ath_tx_complete_aggr(sc, txq, bf, bf_head, sta,
+					     tid, ts, txok);
 		}
-		ath_tx_complete_buf(sc, bf, txq, bf_head, sta, ts, txok);
-	} else
-		ath_tx_complete_aggr(sc, txq, bf, bf_head, sta, tid, ts, txok);
+	}
 
 	if (!flush)
 		ath_txq_schedule(sc, txq);
Signature · 1 line
2.27.0
#9 lore raw

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

Quoted 11 lines · Shuah Khan

ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr() return pointer (sta) outside null check. Fix it by moving the code block under the null check.

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Signed-off-by Shuah Khan <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

Patch applied to ath-next branch of ath.git, thanks.

a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/patch/43ed9abb9e8d7112f3cc168c2f8c489e253635ba.1613090339.git.skhan@linuxfoundation.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
maintainer
#10 lore raw

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

Quoted 11 lines · Kalle Valo
Quoted 9 lines · Shuah Khan

ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr() return pointer (sta) outside null check. Fix it by moving the code block under the null check.

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Quoted 6 lines
Quoted 2 lines
Signed-off-by Shuah Khan <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

Patch applied to ath-next branch of ath.git, thanks.

a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference

I just took another look at this patch, and it is completely bogus. Not only does the stated reason not make any sense (sta is simply passed to other functions, not dereferenced without checks), but this also introduces a horrible memory leak by skipping buffer completion if sta is NULL.
Please drop it, the code is fine as-is.

- Felix

author
#11 lore raw

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

Quoted 18 lines · Felix Fietkau
Quoted 16 lines · Kalle Valo
Quoted 11 lines · Shuah Khan

ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr() return pointer (sta) outside null check. Fix it by moving the code block under the null check.

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Signed-off-by Shuah Khan <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

Patch applied to ath-next branch of ath.git, thanks.

Quoted 8 lines
Quoted 1 line

a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference

I just took another look at this patch, and it is completely bogus. Not only does the stated reason not make any sense (sta is simply passed to other functions, not dereferenced without checks), but this also introduces a horrible memory leak by skipping buffer completion if sta is NULL.
Please drop it, the code is fine as-is.

A comment describing what you said here might be a good addition to this comment block though.

thanks,
-- Shuah

#12 lore raw

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

Quoted 27 lines · Shuah Khan
Quoted 18 lines · Felix Fietkau
Quoted 16 lines · Kalle Valo
Quoted 11 lines · Shuah Khan

ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr() return pointer (sta) outside null check. Fix it by moving the code block under the null check.

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Signed-off-by Shuah Khan <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

Patch applied to ath-next branch of ath.git, thanks.

Quoted 7 lines
Quoted 1 line

a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference

I just took another look at this patch, and it is completely bogus. Not only does the stated reason not make any sense (sta is simply passed to other functions, not dereferenced without checks), but this also introduces a horrible memory leak by skipping buffer completion if sta is NULL.
Please drop it, the code is fine as-is.

Quoted 2 lines

A comment describing what you said here might be a good addition to this comment block though.

Shuah, can you send a followup patch which reverts your change and adds the comment? I try to avoid rebasing my trees.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
author
#13 lore raw

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

Quoted 29 lines · Kalle Valo
Quoted 27 lines · Shuah Khan
Quoted 25 lines · Felix Fietkau
Quoted 11 lines · Kalle Valo
Quoted 9 lines · Shuah Khan

ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr() return pointer (sta) outside null check. Fix it by moving the code block under the null check.

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Quoted 6 lines
Quoted 2 lines
Signed-off-by Shuah Khan <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

Patch applied to ath-next branch of ath.git, thanks.

a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference

I just took another look at this patch, and it is completely bogus. Not only does the stated reason not make any sense (sta is simply passed to other functions, not dereferenced without checks), but this also introduces a horrible memory leak by skipping buffer completion if sta is NULL.
Please drop it, the code is fine as-is.

Quoted 6 lines
Quoted 2 lines

A comment describing what you said here might be a good addition to this comment block though.

Shuah, can you send a followup patch which reverts your change and adds the comment? I try to avoid rebasing my trees.

I can do that.

thanks,
-- Shuah

author
#14 lore raw

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

Quoted 22 lines · Shuah Khan
Quoted 21 lines · Kalle Valo
Quoted 19 lines · Shuah Khan
Quoted 18 lines · Felix Fietkau
Quoted 16 lines · Kalle Valo
Quoted 11 lines · Shuah Khan

ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr() return pointer (sta) outside null check. Fix it by moving the code block under the null check.

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Signed-off-by Shuah Khan <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

Patch applied to ath-next branch of ath.git, thanks.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 1 line

a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference

I just took another look at this patch, and it is completely bogus. Not only does the stated reason not make any sense (sta is simply passed to other functions, not dereferenced without checks), but this also introduces a horrible memory leak by skipping buffer completion if sta is NULL.
Please drop it, the code is fine as-is.

Felix,

I looked at the code path again and found the following path that can become a potential dereference downstream. My concern is about potential dereference downstream.

First path: ath_tx_complete_buf()

1. ath_tx_process_buffer() passes sta to ath_tx_complete_buf()
2. ath_tx_complete_buf() doesn't check or dereference sta Passes it on to ath_tx_complete()
3. ath_tx_complete() doesn't check or dereference sta, but assigns it to tx_info->status.status_driver_data[0] tx_info->status.status_driver_data[0] = sta;

ath_tx_complete_buf() should be fixed to check sta perhaps?

This assignment without checking could lead to dereference at some point in the future.

Second path: ath_tx_complete_aggr()

1. ath_tx_process_buffer() passes sta to ath_tx_complete_aggr()
2. No problems in this path as ath_tx_complete_aggr() checks sta before use.

I can send the revert as it moves more code than necessary under the null check. As you pointed out, it could lead to memory leak. Not knowing this code well, I can't really tell where. However, my original concern is valid for ath_tx_complete_buf() path.

Sending revert as requested.

thanks,
-- Shuah

maintainer
#15 lore raw

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

Quoted 50 lines · Shuah Khan
Quoted 22 lines · Shuah Khan
Quoted 22 lines · Kalle Valo
Quoted 19 lines · Shuah Khan
Quoted 18 lines · Felix Fietkau
Quoted 16 lines · Kalle Valo
Quoted 11 lines · Shuah Khan

ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr() return pointer (sta) outside null check. Fix it by moving the code block under the null check.

This problem was found while reviewing code to debug RCU warn from ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit of other callers of ieee80211_find_sta_by_ifaddr() that don't hold RCU read lock.

Signed-off-by Shuah Khan <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

Patch applied to ath-next branch of ath.git, thanks.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 1 line

a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference

I just took another look at this patch, and it is completely bogus. Not only does the stated reason not make any sense (sta is simply passed to other functions, not dereferenced without checks), but this also introduces a horrible memory leak by skipping buffer completion if sta is NULL.
Please drop it, the code is fine as-is.

Felix,

I looked at the code path again and found the following path that can become a potential dereference downstream. My concern is about potential dereference downstream.

First path: ath_tx_complete_buf()

1. ath_tx_process_buffer() passes sta to ath_tx_complete_buf()
2. ath_tx_complete_buf() doesn't check or dereference sta Passes it on to ath_tx_complete()
3. ath_tx_complete() doesn't check or dereference sta, but assigns it to tx_info->status.status_driver_data[0] tx_info->status.status_driver_data[0] = sta;

Quoted 4 lines

ath_tx_complete_buf() should be fixed to check sta perhaps?

This assignment without checking could lead to dereference at some point in the future.

The assignment is fine, no check needed here. If there was any invalid dereference here, we would see reports of crashes with NULL pointer dereference. Sending packets with sta==NULL is quite common, especially in AP mode.

Quoted 10 lines

Second path: ath_tx_complete_aggr()

1. ath_tx_process_buffer() passes sta to ath_tx_complete_aggr()
2. No problems in this path as ath_tx_complete_aggr() checks sta before use.

I can send the revert as it moves more code than necessary under the null check. As you pointed out, it could lead to memory leak. Not knowing this code well, I can't really tell where. However, my original concern is valid for ath_tx_complete_buf

I still don’t see anything to be concerned about. I don’t even think passing a pointer that could be NULL to another function even deserves a comment in the code. Stuff like that is used in many other places as well.

- Felix