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

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

Ping-Ke Shih wrote:
> 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)

fix typo
            (cb is use-after-free)


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