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

[Ping-Ke Shih <pkshih@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 */

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