RE: [PATCH rtw v4 2/4] wifi: rtw89: fix tx_wait initialization race

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 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 */

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));