linux-kernel - Re: [PATCH 05/14] mwifiex: re-register wiphy across reset

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: <20170621182706.GB92340@google.com>
Date:   Wed, 21 Jun 2017 11:27:07 -0700
From:   Brian Norris <briannorris@...omium.org>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     Kalle Valo <kvalo@...eaurora.org>,
        Ganapathi Bhat <gbhat@...vell.com>,
        Nishant Sarmukadam <nishants@...vell.com>,
        linux-kernel@...r.kernel.org,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Amitkumar Karwar <amitkarwar@...il.com>,
        linux-wireless@...r.kernel.org
Subject: Re: [PATCH 05/14] mwifiex: re-register wiphy across reset

Hi Johannes,

On Fri, Jun 09, 2017 at 11:03:38AM +0200, Johannes Berg wrote:
> On Mon, 2017-06-05 at 18:54 +0300, Kalle Valo wrote:
> 
> > > BTW, since you're taking an interest in this code now, can I
> > > trouble you with a question? Looking at mwifiex_uninit_sw() (after
> > > this patchset), you can see a loop like this:
> > > 
> > >         /* Stop data */
> > >         for (i = 0; i < adapter->priv_num; i++) {
> > >                 priv = adapter->priv[i];
> > >                 if (priv && priv->netdev) {
> > >                         mwifiex_stop_net_dev_queue(priv->netdev,
> > > adapter);
> > >                         if (netif_carrier_ok(priv->netdev))
> > >                                 netif_carrier_off(priv->netdev);
> > >                         netif_device_detach(priv->netdev);
> > >                 }
> > >         }
> > > 
> > > That seems to be the only attempt to prevent user space from
> > > talking to the device while we proceed to shut down
> > > (mwifiex_shutdown_drv()). AIUI, that's wholly insufficient, and we
> > > need to actually stop all the virtual interfaces (and possibly the
> > > wiphy as well) first. I'm looking at trying to move the
> > > mwifiex_del_virtual_intf() loop up much further in this function
> > > (but there are other bugs preventing me from doing that yet).
> > > 
> > > Does that sound like the right approach to you? I'm kinda figuring
> > > this should better mimic the mac80211 ieee80211_remove_interfaces()
> > > structure.
> > 
> > Johannes is much better person to answer this (CCed).
> 
> Wait, what? You're throwing me into pretty deep water ;-)

Regardless, thanks for the help :)

> I'm not sure what you mean by "we need to atually stop all the virtual
> interfaces ([...]) first".

Judging by your following comments, I may have been completely mistaken.
(But that's why I asked you folks!)

> There are essentially only two/three ways to reach this - data path,
> which is getting stopped here, and control path (both nl80211 and
> perhaps ndo ops like start/stop).

I think I was conflating virtual interfaces with control path (e.g.,
nl80211 scans, set freq, etc.). The idea is that control operations may
still get *started* after the above, and it's just plain impossible to
resolve the races with driver queue teardown if we're queueing up new
control ops at the same time.

But even if we kill off the wireless_dev's, I suppose there are still
control interfaces that can talk directly to the wiphy.

> Without checking the code now, it seems entirely plausible that this is
> holding some lock that would lock out the control path entirely, for
> the duration until the wiphy is actually unregistered?
> 
> Actually, you can't unregister with the relevant locks held (without
> causing deadlocks), so perhaps it's marking the wiphy as unavailable so
> that all operations fail?

One of the above two sounds along the right line. But it's something I
couldn't really figure out how to do quite right.

Dumb question: how would I mark the wiphy as unavailable? Is there
something I can do at the cfg80211 level? Or would I really have to
guard all the cfg80211 entry points into mwifiex with a flag or lock?

Also, IIUC, we need to wait for all control paths to complete (or
cancel) before we can free up the associated resources; so just marking
"unavailable" isn't enough.

Thanks,
Brian