RE: [PATCH rtw-next v3] wifi: rtw88: usb: Download the reserved page synchronously

[Ping-Ke Shih <pkshih@realtek.com>]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> @@ -499,16 +506,21 @@ static void rtw_usb_tx_queue_purge(struct rtw_usb *rtwusb)
> 
>  static void rtw_usb_write_port_complete(struct urb *urb)
>  {
> -       struct sk_buff *skb = urb->context;
> +       struct rtw_usb_write_data_cb *cb = urb->context;
> 

I think here is a racing with timeout case of wait_for_completion_timeout().

                           CPU0                      CPU1
           (rtw_usb_write_port_complete)         (rtw_usb_write_data)

           cb = urb->context
                                                wait_for_completion_timeout() timeout
                                                kfree(cb);
           cb->status = urb->status;
          (cb is free-after-use)

rtw89 handles this kind of problem by RCU. You can reference
rtw89_wait_for_cond_eval() to know how it does.

> -       dev_kfree_skb_any(skb);
> +       cb->status = urb->status;
> +       complete(&cb->done);
> +       dev_kfree_skb_any(cb->skb);
> +       if (!cb->sync)
> +               kfree(cb);
>  }
> 
>  static int rtw_usb_write_data(struct rtw_dev *rtwdev,
>                               struct rtw_tx_pkt_info *pkt_info,
> -                             u8 *buf)
> +                             u8 *buf, bool sync)
>  {
>         const struct rtw_chip_info *chip = rtwdev->chip;
> +       struct rtw_usb_write_data_cb *cb;
>         struct sk_buff *skb;
>         unsigned int size;
>         u8 qsel;
> @@ -528,11 +540,29 @@ static int rtw_usb_write_data(struct rtw_dev *rtwdev,
>         rtw_tx_fill_tx_desc(rtwdev, pkt_info, skb);
>         rtw_tx_fill_txdesc_checksum(rtwdev, pkt_info, skb->data);
> 
> +       cb = kmalloc_obj(*cb, GFP_KERNEL);
> +       if (!cb) {
> +               dev_kfree_skb_any(skb);
> +               return -ENOMEM;
> +       }
> +
> +       init_completion(&cb->done);
> +       cb->status = -EINPROGRESS;
> +       cb->skb = skb;
> +       cb->sync = sync;
> +
>         ret = rtw_usb_write_port(rtwdev, qsel, skb,
> -                                rtw_usb_write_port_complete, skb);
> +                                rtw_usb_write_port_complete, cb);
>         if (unlikely(ret)) {
>                 rtw_err(rtwdev, "failed to do USB write, ret=%d\n", ret);
>                 dev_kfree_skb_any(skb);
> +               kfree(cb);
> +       } else if (sync) {
> +               if (!wait_for_completion_timeout(&cb->done, secs_to_jiffies(5)))
> +                       ret = -ETIMEDOUT;
> +               else
> +                       ret = cb->status;
> +               kfree(cb);
>         }
> 
>         return ret;