linux-kernel - Re: [PATCH v3 7/8] wifi: wfx: allow to send frames during ROC

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] [day] [month] [year] [list]

Message-ID: <2721026.q0ZmV6gNhb@nb0018864>
Date: Wed, 27 Nov 2024 10:18:26 +0100
From: Jérôme Pouiller <jerome.pouiller@...abs.com>
To: "kvalo@...nel.org" <kvalo@...nel.org>,
        "Sverdlin, Alexander" <alexander.sverdlin@...mens.com>
Cc: "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 7/8] wifi: wfx: allow to send frames during ROC

On Tuesday 26 November 2024 16:54:12 CET Sverdlin, Alexander wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> Thanks for the quick reply Jerome,
> 
> On Tue, 2024-11-26 at 15:45 +0100, Jérôme Pouiller wrote:
> > > > +             for (i = 0; i < num_queues; i++) {
> > > > +                     skb = skb_dequeue(&queues[i]->offchan);
> > >                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >
> > > Nevertheless, the lockdep splat comes from here, because
> > > wfx_tx_queues_init() has never been called for wvif->id == 2.
> > >
> > > What was your original plan for this to happen?
> > > Do we need an explicit analogue of wfx_add_interface() for vif->id 2 somewhere?
> > >
> > > I still have not come with a reproducer yet, but you definitely have more
> > > insight into this code, maybe this will ring some bells on your side...
> > >
> > > PS. It's v6.11, even though it's exactly the same splan as in
> > > "staging: wfx: fix potential use before init", but the patch in indeed inside.
> >
> > Yes, probably a very similar issue to "staging: wfx: fix potential use
> > before init". I don't believe the issue is related to wvif->id == 2.
> >
> > You have only produced this issue once, that's it?
> >
> > I wonder why this does not happen with queues[i]->normal and
> > queues[i]->cab. Is it because queues[i]->offchan is the first to be
> > checked? Or mutex_is_locked(&wdev->scan_lock) has an impact in the
> > process?
> >
> > In wfx_add_interface(), the list of wvif is protected by conf_lock.
> > However, wfx_tx_queues_get_skb() is not protected by conf_lock. We
> > initialize struct wvif before to add it to the wvif list and we
> > consider it is sufficient. However, after reading memory-barriers.txt
> > again, it's probably a wrong assumption.
> 
> I've actually disassembled the stack trace exactly to offchan processing.
> I have no idea why kernel sends offchan on a non-configured idle interface,
> I still need to come up with a reproducer.
> 
> But as soon as there is an offchan in the sorted list of "queues" (coming
> originally from VIF 0:
> 
> void wfx_tx(struct ieee80211_hw *hw, struct ieee80211_tx_control *control, struct sk_buff *skb)
> {
> ...
>         if (tx_info->control.vif)
>                 wvif = (struct wfx_vif *)tx_info->control.vif->drv_priv;
>         else
>                 wvif = wvif_iterate(wdev, NULL);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Puts any offchan into offchan not of VIF 2, but of the only VIF 0...

Note skb_dequeue(&queues[i]->offchan) is called whatever there is a frame
in the offchan queue. In fact, wfx_tx_queues_get_skb() can be called even
if all the tx queues are empty (and this happen when the wake up event
comes from the device).

So the reproducer involves wfx_add_interface() and a not-yet-identified
event (that could be an IRQ and a Tx frame) that wake up the bh workqueue.

> I think you are right, this could only be offchan queue of VIF 0.
> But then it's just a race of TX workqueue against
> wfx_remove_interface()/wfx_add_interface() pair (which I see regularly).

We have the same conclusion.

> We probably need RCU in the TX path and NetLink lock in the VIF add/remove
> path similar to other network drivers...
> I can try to come up with a patch for this...

I wonder if there is a way to iterate over the vif using the cfg80211/mac80211
API rather than maintaining a list of vif in the driver.

[...]

-- 
Jérôme Pouiller