Now that nullfunc skbs are recycled in a separate work item in the driver, the following race during initialization and processing of those skbs might lead to noticeable bugs:
Waiting thread Completing thread
rtw89_core_send_nullfunc() rtw89_core_tx_write_link() ... rtw89_pci_txwd_submit() skb_data->wait = NULL /* add skb to the queue */ skb_queue_tail(&txwd->queue, skb) rtw89_pci_napi_poll() ... rtw89_pci_release_txwd_skb() /* get skb from the queue */ skb_unlink(skb, &txwd->queue) rtw89_pci_tx_status() rtw89_core_tx_wait_complete() /* use incorrect skb_data->wait */ rtw89_core_tx_kick_off_and_wait() /* assign skb_data->wait but too late */
The value of skb_data->wait indicates whether skb is passed on to the core ieee80211 stack or released by the driver itself. So assure that by the time skb is added to txwd queue and becomes visible to the completing side, it has already allocated tx_wait-related data (in case it's needed).
Found by Linux Verification Center (linuxtesting.org).
Fixes: 1ae5ca615285 ("wifi: rtw89: add function to wait for completion of TX skbs") Cc: stable@vger.kernel.org Signed-off-by: Fedor Pchelkin pchelkin@ispras.ru ---
v4: - use wiphy_dereference (Zong-Zhe) - move wait->skb assignment place
drivers/net/wireless/realtek/rtw89/core.c | 35 ++++++++++++++--------- drivers/net/wireless/realtek/rtw89/pci.c | 2 -- 2 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c index 438930b65631..1efe4bb09262 100644 --- a/drivers/net/wireless/realtek/rtw89/core.c +++ b/drivers/net/wireless/realtek/rtw89/core.c @@ -1094,22 +1094,13 @@ int rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev, struct sk_buff *sk int qsel, unsigned int timeout) { struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb); - struct rtw89_tx_wait_info *wait; + struct rtw89_tx_wait_info *wait = wiphy_dereference(rtwdev->hw->wiphy, + skb_data->wait); unsigned long time_left; int ret = 0;
lockdep_assert_wiphy(rtwdev->hw->wiphy);
- wait = kzalloc(sizeof(*wait), GFP_KERNEL); - if (!wait) { - rtw89_core_tx_kick_off(rtwdev, qsel); - return 0; - } - - init_completion(&wait->completion); - wait->skb = skb; - rcu_assign_pointer(skb_data->wait, wait); - rtw89_core_tx_kick_off(rtwdev, qsel); time_left = wait_for_completion_timeout(&wait->completion, msecs_to_jiffies(timeout)); @@ -1171,10 +1162,12 @@ int rtw89_h2c_tx(struct rtw89_dev *rtwdev, static int rtw89_core_tx_write_link(struct rtw89_dev *rtwdev, struct rtw89_vif_link *rtwvif_link, struct rtw89_sta_link *rtwsta_link, - struct sk_buff *skb, int *qsel, bool sw_mld) + struct sk_buff *skb, int *qsel, bool sw_mld, + struct rtw89_tx_wait_info *wait) { struct ieee80211_sta *sta = rtwsta_link_to_sta_safe(rtwsta_link); struct ieee80211_vif *vif = rtwvif_link_to_vif(rtwvif_link); + struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb); struct rtw89_vif *rtwvif = rtwvif_link->rtwvif; struct rtw89_core_tx_request tx_req = {}; int ret; @@ -1191,6 +1184,8 @@ static int rtw89_core_tx_write_link(struct rtw89_dev *rtwdev, rtw89_core_tx_update_desc_info(rtwdev, &tx_req); rtw89_core_tx_wake(rtwdev, &tx_req);
+ rcu_assign_pointer(skb_data->wait, wait); + ret = rtw89_hci_tx_write(rtwdev, &tx_req); if (ret) { rtw89_err(rtwdev, "failed to transmit skb to HCI\n"); @@ -1227,7 +1222,8 @@ int rtw89_core_tx_write(struct rtw89_dev *rtwdev, struct ieee80211_vif *vif, } }
- return rtw89_core_tx_write_link(rtwdev, rtwvif_link, rtwsta_link, skb, qsel, false); + return rtw89_core_tx_write_link(rtwdev, rtwvif_link, rtwsta_link, skb, qsel, false, + NULL); }
static __le32 rtw89_build_txwd_body0(struct rtw89_tx_desc_info *desc_info) @@ -3425,6 +3421,7 @@ int rtw89_core_send_nullfunc(struct rtw89_dev *rtwdev, struct rtw89_vif_link *rt struct ieee80211_vif *vif = rtwvif_link_to_vif(rtwvif_link); int link_id = ieee80211_vif_is_mld(vif) ? rtwvif_link->link_id : -1; struct rtw89_sta_link *rtwsta_link; + struct rtw89_tx_wait_info *wait; struct ieee80211_sta *sta; struct ieee80211_hdr *hdr; struct rtw89_sta *rtwsta; @@ -3434,6 +3431,12 @@ int rtw89_core_send_nullfunc(struct rtw89_dev *rtwdev, struct rtw89_vif_link *rt if (vif->type != NL80211_IFTYPE_STATION || !vif->cfg.assoc) return 0;
+ wait = kzalloc(sizeof(*wait), GFP_KERNEL); + if (!wait) + return -ENOMEM; + + init_completion(&wait->completion); + rcu_read_lock(); sta = ieee80211_find_sta(vif, vif->cfg.ap_addr); if (!sta) { @@ -3448,6 +3451,8 @@ int rtw89_core_send_nullfunc(struct rtw89_dev *rtwdev, struct rtw89_vif_link *rt goto out; }
+ wait->skb = skb; + hdr = (struct ieee80211_hdr *)skb->data; if (ps) hdr->frame_control |= cpu_to_le16(IEEE80211_FCTL_PM); @@ -3458,7 +3463,8 @@ int rtw89_core_send_nullfunc(struct rtw89_dev *rtwdev, struct rtw89_vif_link *rt goto out; }
- ret = rtw89_core_tx_write_link(rtwdev, rtwvif_link, rtwsta_link, skb, &qsel, true); + ret = rtw89_core_tx_write_link(rtwdev, rtwvif_link, rtwsta_link, skb, &qsel, true, + wait); if (ret) { rtw89_warn(rtwdev, "nullfunc transmit failed: %d\n", ret); dev_kfree_skb_any(skb); @@ -3471,6 +3477,7 @@ int rtw89_core_send_nullfunc(struct rtw89_dev *rtwdev, struct rtw89_vif_link *rt timeout); out: rcu_read_unlock(); + kfree(wait);
return ret; } diff --git a/drivers/net/wireless/realtek/rtw89/pci.c b/drivers/net/wireless/realtek/rtw89/pci.c index 4e3034b44f56..cb9682f306a6 100644 --- a/drivers/net/wireless/realtek/rtw89/pci.c +++ b/drivers/net/wireless/realtek/rtw89/pci.c @@ -1372,7 +1372,6 @@ static int rtw89_pci_txwd_submit(struct rtw89_dev *rtwdev, struct pci_dev *pdev = rtwpci->pdev; struct sk_buff *skb = tx_req->skb; struct rtw89_pci_tx_data *tx_data = RTW89_PCI_TX_SKB_CB(skb); - struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb); bool en_wd_info = desc_info->en_wd_info; u32 txwd_len; u32 txwp_len; @@ -1388,7 +1387,6 @@ static int rtw89_pci_txwd_submit(struct rtw89_dev *rtwdev, }
tx_data->dma = dma; - rcu_assign_pointer(skb_data->wait, NULL);
txwp_len = sizeof(*txwp_info); txwd_len = chip->txwd_body_size;
Fedor Pchelkin pchelkin@ispras.ru wrote:
Now that nullfunc skbs are recycled in a separate work item in the driver, the following race during initialization and processing of those skbs might lead to noticeable bugs:
Waiting thread Completing threadrtw89_core_send_nullfunc() rtw89_core_tx_write_link() ... rtw89_pci_txwd_submit() skb_data->wait = NULL /* add skb to the queue */ skb_queue_tail(&txwd->queue, skb) rtw89_pci_napi_poll() ... rtw89_pci_release_txwd_skb() /* get skb from the queue */ skb_unlink(skb, &txwd->queue) rtw89_pci_tx_status() rtw89_core_tx_wait_complete() /* use incorrect skb_data->wait */ rtw89_core_tx_kick_off_and_wait() /* assign skb_data->wait but too late */
How will we receive tx completion before TX kick off? (see the original code below)
[...]
diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c index 438930b65631..1efe4bb09262 100644 --- a/drivers/net/wireless/realtek/rtw89/core.c +++ b/drivers/net/wireless/realtek/rtw89/core.c @@ -1094,22 +1094,13 @@ int rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev, struct sk_buff *sk int qsel, unsigned int timeout) { struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
struct rtw89_tx_wait_info *wait;
struct rtw89_tx_wait_info *wait = wiphy_dereference(rtwdev->hw->wiphy,skb_data->wait);
Can't we just pass 'wait' by function argument?
unsigned long time_left; int ret = 0; lockdep_assert_wiphy(rtwdev->hw->wiphy);
wait = kzalloc(sizeof(*wait), GFP_KERNEL);if (!wait) {rtw89_core_tx_kick_off(rtwdev, qsel);return 0;}init_completion(&wait->completion);wait->skb = skb;rcu_assign_pointer(skb_data->wait, wait);
Here, original code prepares completion before TX kick off. How it could be a problem? Do I miss something?
rtw89_core_tx_kick_off(rtwdev, qsel); time_left = wait_for_completion_timeout(&wait->completion, msecs_to_jiffies(timeout));
On Thu, 18. Sep 05:47, Ping-Ke Shih wrote:
Fedor Pchelkin pchelkin@ispras.ru wrote:
@@ -1094,22 +1094,13 @@ int rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev, struct sk_buff *sk int qsel, unsigned int timeout) { struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
struct rtw89_tx_wait_info *wait;
struct rtw89_tx_wait_info *wait = wiphy_dereference(rtwdev->hw->wiphy,skb_data->wait);Can't we just pass 'wait' by function argument?
Yep.
unsigned long time_left; int ret = 0; lockdep_assert_wiphy(rtwdev->hw->wiphy);
wait = kzalloc(sizeof(*wait), GFP_KERNEL);if (!wait) {rtw89_core_tx_kick_off(rtwdev, qsel);return 0;}init_completion(&wait->completion);wait->skb = skb;rcu_assign_pointer(skb_data->wait, wait);Here, original code prepares completion before TX kick off. How it could be a problem? Do I miss something?
That's a good question and it made me rethink the cause of the race scenario. I didn't initially take TX kick off into consideration when it actually matters.
The thing is: there might have been another thread initiating TX kick off for the same queue in parallel. But no such thread exists because a taken wiphy lock generally protects from such situations. rtw89_core_txq_schedule() worker looks like a good candidate but it doesn't operate on the needed management queues.
So I may conclude this patch doesn't fix any real bug though I'd prefer to keep it (with description rewritten of course) because it helps to avoid potential issues in future.
Fedor Pchelkin pchelkin@ispras.ru wrote:
On Thu, 18. Sep 05:47, Ping-Ke Shih wrote:
Fedor Pchelkin pchelkin@ispras.ru wrote:
@@ -1094,22 +1094,13 @@ int rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev, struct sk_buff
*sk
int qsel, unsigned int timeout){ struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
struct rtw89_tx_wait_info *wait;
struct rtw89_tx_wait_info *wait = wiphy_dereference(rtwdev->hw->wiphy,skb_data->wait);Can't we just pass 'wait' by function argument?
Yep.
unsigned long time_left; int ret = 0; lockdep_assert_wiphy(rtwdev->hw->wiphy);
wait = kzalloc(sizeof(*wait), GFP_KERNEL);if (!wait) {rtw89_core_tx_kick_off(rtwdev, qsel);return 0;}init_completion(&wait->completion);wait->skb = skb;rcu_assign_pointer(skb_data->wait, wait);Here, original code prepares completion before TX kick off. How it could be a problem? Do I miss something?
That's a good question and it made me rethink the cause of the race scenario. I didn't initially take TX kick off into consideration when it actually matters.
Do it mean that you pictured the racing scenario in commit message by code review instead of a real case you met?
The thing is: there might have been another thread initiating TX kick off for the same queue in parallel. But no such thread exists because a taken wiphy lock generally protects from such situations. rtw89_core_txq_schedule() worker looks like a good candidate but it doesn't operate on the needed management queues.
Last night I also thought if another thread works in parallel. Maybe rtw89_ops_tx() could be?
So I may conclude this patch doesn't fix any real bug though I'd prefer to keep it (with description rewritten of course) because it helps to avoid potential issues in future.
Agree.
Ping-Ke Shih pkshih@realtek.com wrote:
Fedor Pchelkin pchelkin@ispras.ru wrote:
On Thu, 18. Sep 05:47, Ping-Ke Shih wrote:
Fedor Pchelkin pchelkin@ispras.ru wrote:
@@ -1094,22 +1094,13 @@ int rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev, struct sk_buff
*sk
int qsel, unsigned int timeout){ struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
struct rtw89_tx_wait_info *wait;
struct rtw89_tx_wait_info *wait = wiphy_dereference(rtwdev->hw->wiphy,skb_data->wait);Can't we just pass 'wait' by function argument?
Yep.
unsigned long time_left; int ret = 0; lockdep_assert_wiphy(rtwdev->hw->wiphy);
wait = kzalloc(sizeof(*wait), GFP_KERNEL);if (!wait) {rtw89_core_tx_kick_off(rtwdev, qsel);return 0;}init_completion(&wait->completion);wait->skb = skb;rcu_assign_pointer(skb_data->wait, wait);Here, original code prepares completion before TX kick off. How it could be a problem? Do I miss something?
That's a good question and it made me rethink the cause of the race scenario. I didn't initially take TX kick off into consideration when it actually matters.
Do it mean that you pictured the racing scenario in commit message by code review instead of a real case you met?
The thing is: there might have been another thread initiating TX kick off for the same queue in parallel. But no such thread exists because a taken wiphy lock generally protects from such situations. rtw89_core_txq_schedule() worker looks like a good candidate but it doesn't operate on the needed management queues.
Last night I also thought if another thread works in parallel. Maybe rtw89_ops_tx() could be?
So I may conclude this patch doesn't fix any real bug though I'd prefer to keep it (with description rewritten of course) because it helps to avoid potential issues in future.
Agree.
Forgot to say. Could you mention this racing scenario was found by core review and your perspective in commit message?
On Fri, 19. Sep 00:50, Ping-Ke Shih wrote:
Ping-Ke Shih pkshih@realtek.com wrote:
Fedor Pchelkin pchelkin@ispras.ru wrote:
That's a good question and it made me rethink the cause of the race scenario. I didn't initially take TX kick off into consideration when it actually matters.
Do it mean that you pictured the racing scenario in commit message by code review instead of a real case you met?
Yes, the underlying issue for this patch was found by code review only. Somehow the negative consequences of the potential race became an "obvious" thing after preparing the first commit, and ignorance of TX kick off influence made the changelog confusing..
The thing is: there might have been another thread initiating TX kick off for the same queue in parallel. But no such thread exists because a taken wiphy lock generally protects from such situations. rtw89_core_txq_schedule() worker looks like a good candidate but it doesn't operate on the needed management queues.
Last night I also thought if another thread works in parallel. Maybe rtw89_ops_tx() could be?
Well, probably it could. I thought rtw89_ops_tx() is wiphy locked, too, but apparently it's not always the case.
Not that it's a relatively easy-to-hit race I'm going to try to reproduce though :)
So I may conclude this patch doesn't fix any real bug though I'd prefer to keep it (with description rewritten of course) because it helps to avoid potential issues in future.
Agree.
Forgot to say. Could you mention this racing scenario was found by core review and your perspective in commit message?
Sure.
linux-stable-mirror@lists.linaro.org
-
Fedor Pchelkin -
Ping-Ke Shih