RE: [PATCH rtw v3 2/5] wifi: rtw89: fix tx_wait initialization race

[Zong-Zhe Yang <kevin_yang@realtek.com>]

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 */
> 
> 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>
> ---
>  drivers/net/wireless/realtek/rtw89/core.c | 30 ++++++++++++++---------
> drivers/net/wireless/realtek/rtw89/pci.c  |  2 --
>  2 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> b/drivers/net/wireless/realtek/rtw89/core.c
> index 5c964283cd67..57f20559dfde 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1094,20 +1094,12 @@ 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 = skb_data->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);
> -       rcu_assign_pointer(skb_data->wait, wait);
>         wait->skb = skb;

Also, move "wait->skb = skb" to where wait is initialized.

> 
>         rtw89_core_tx_kick_off(rtwdev, qsel); @@ -1172,10 +1164,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;
> @@ -1192,6 +1186,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"); @@ -1228,7 +1224,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) @@ -3426,6
> +3423,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;
> @@ -3435,6 +3433,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) {
> @@ -3459,7 +3463,8 @@ int rtw89_core_send_nullfunc(struct rtw89_dev *rtwdev, struct
> rtw89_vif_link *rt
>                 goto out;
>         }
> 

Fill wait->skb as mentioned above.

> -       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);
> @@ -3472,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;
> --
> 2.51.0
>