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

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

Ping-Ke Shih <pkshih@realtek.com> wrote:
> Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> > On Thu, 18. Sep 05:47, Ping-Ke Shih wrote:
> > > Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> > > > @@ -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?
> >
> > Yep.
> >
> > >
> > > >         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?
> >
> > That's a good question and it made me rethink the cause of the race
> > scenario.  I didn't initially take TX kick off into consideration when
> > it actually matters.
> 
> Do it mean that you pictured the racing scenario in commit message by
> code review instead of a real case you met?
> 
> >
> > The thing is: there might have been another thread initiating TX kick off
> > for the same queue in parallel.  But no such thread exists because a taken
> > wiphy lock generally protects from such situations. rtw89_core_txq_schedule()
> > worker looks like a good candidate but it doesn't operate on the needed
> > management queues.
> 
> Last night I also thought if another thread works in parallel.
> Maybe rtw89_ops_tx() could be?
> 
> >
> > So I may conclude this patch doesn't fix any real bug though I'd prefer to
> > keep it (with description rewritten of course) because it helps to avoid
> > potential issues in future.
> 
> Agree.
> 

Forgot to say. Could you mention this racing scenario was found by core
review and your perspective in commit message?