linux-kernel - Re: [PATCH rtw v4 2/4] wifi: rtw89: fix tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250918173522-07abe99566c12fa46a096fc5-pchelkin@ispras>
Date: Thu, 18 Sep 2025 18:19:34 +0300
From: Fedor Pchelkin <pchelkin@...ras.ru>
To: Ping-Ke Shih <pkshih@...ltek.com>
Cc: Zong-Zhe Yang <kevin_yang@...ltek.com>, 
	Bitterblue Smith <rtl8821cerfe2@...il.com>, Bernie Huang <phhuang@...ltek.com>, 
	"linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"lvc-project@...uxtesting.org" <lvc-project@...uxtesting.org>, "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH rtw v4 2/4] wifi: rtw89: fix tx_wait initialization race

On Thu, 18. Sep 05:47, Ping-Ke Shih wrote:
> Fedor Pchelkin <pchelkin@...ras.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.

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.

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.