Re: [BUG] deadlock in nl80211_vendor_cmd

[William McVicker <willmcvicker@google.com>]

On 03/25/2022, William McVicker wrote:
> On 03/25/2022, Johannes Berg wrote:
> > On Fri, 2022-03-25 at 09:49 -0700, Jakub Kicinski wrote:
> > > On Fri, 25 Mar 2022 13:04:23 +0100 Johannes Berg wrote:
> > > > So we can avoid the potential deadlock in cfg80211 in a few ways:
> > > > 
> > > >  1) export rtnl_lock_unregistering_all() or maybe a variant after
> > > >     refactoring the two versions, to allow cfg80211 to use it, that way
> > > >     netdev_run_todo() can never have a non-empty todo list
> > > > 
> > > >  2) export __rtnl_unlock() so cfg80211 can avoid running
> > > >     netdev_run_todo() in the unlock, personally I like this less because
> > > >     it might encourage random drivers to use it
> > > > 
> > > >  3) completely rework cfg80211's locking, adding a separate mutex for
> > > >     the wiphy list so we don't need to acquire the RTNL at all here
> > > >     (unless the ops need it, but there's no issue if we don't drop it),
> > > >     something like https://p.sipsolutions.net/27d08e1f5881a793.txt
> > > > 
> > > > 
> > > > I think I'm happy with 3) now (even if it took a couple of hours), so I
> > > > think we can go with it, just need to go through all the possibilities.
> > > 
> > > I like 3) as well. FWIW a few places (e.g. mlx5, devlink, I think I've
> > > seen more) had been converting to xarray for managing the "registered"
> > > objects. It may be worth looking into if you're re-doing things, anyway.
> > > 
> > 
> > That's not a bad idea, but I think I wouldn't want to backport that, so
> > separately :) I don't think that fundamentally changes the locking
> > properties though.
> > 
> > 
> > Couple of more questions I guess: First, are we assuming that the
> > cfg80211 code *is* actually broken, even if it looks like nothing can
> > cause the situation, due to the empty todo list?
> 
> I'm able to reproduce this issue pretty easily with a Pixel 6 when I add
> support to allow vendor commands to request for the RTNL. For this case, I just
> delay unlocking the RTNL until nl80211_vendor_cmds() at which point I check the
> flags to see if I should unlock before calling doit(). That allows me to run my
> tests again and hit this issue. I imagine that I could hit this issue without
> any changes if I re-work my vendor ops to not need the RTNL.
> 
> > 
> > Given that we have rtnl_lock_unregistering() (and also
> > rtnl_lock_unregistering_all()), it looks like we *do* in fact at least
> > not want to make an assumption that no user of __rtnl_unlock() can have
> > added a todo item.
> > 
> > I mean, there's technically yet *another* thing we could do - something
> > like this:
> > 
> > [this doesn't compile, need to suitably make net_todo_list non-static]
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -95,6 +95,7 @@ void __rtnl_unlock(void)
> >  
> >         defer_kfree_skb_list = NULL;
> >  
> > +       WARN_ON(!list_empty(&net_todo_list));
> >         mutex_unlock(&rtnl_mutex);
> >  
> >         while (head) {
> > 
> > and actually that would allow us to get rid of rtnl_lock_unregistering()
> > and rtnl_lock_unregistering_all() simply because we'd actually guarantee
> > the invariant that when the RTNL is freshly locked, the list is empty
> > (by guaranteeing that it's always empty when it's unlocked, since it can
> > only be added to under RTNL).
> > 
> > With some suitable commentary, that might also be a reasonable thing?
> > __rtnl_unlock() is actually rather pretty rare, and not exported.
> > 
> > 
> > However, if you don't like that ...
> > 
> > I've been testing with this patch, to make lockdep complain:
> > 
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -9933,6 +9933,11 @@ void netdev_run_todo(void)
> >         if (!list_empty(&list))
> >                 rcu_barrier();
> >  
> > +#ifdef CONFIG_LOCKDEP
> > +       rtnl_lock();
> > +       __rtnl_unlock();
> > +#endif
> > +
> >         while (!list_empty(&list)) {
> >                 struct net_device *dev
> >                         = list_first_entry(&list, struct net_device, todo_list);
> > 
> > 
> > That causes lockdep to complain for cfg80211 even if the list *is* in
> > fact empty.
> > 
> > Would you be open to adding something like that? Perhaps if I don't just
> > do the easy rtnl_lock/unlock, but try to find the corresponding lockdep-
> > only things to do there, to cause lockdep to do things without really
> > locking? OTOH, the locking overhead of the RTNL we just unlocked is
> > probably minimal, vs. the actual work *lockdep* is doing to track all
> > this ...
> > 
> > Thanks,
> > johannes
> 
> Let me know if you'd like me to test any patches out.
> 
> Thanks,
> Will

Hi Johannes,

I found that my wlan driver is using the vendor commands to create/delete NAN
interfaces for this Android feature called Wi-Fi aware [1]. Basically, this
features allows users to discover other nearby devices and allows them to
connect directly with one another over a local network. To get my driver
working again, I first had to allow the kernel to let my driver request for the
RTNL lock for these NAN create/delete interface vendor commands. With that
I got the following code path:


Thread 1                         Thread 2
 nl80211_pre_doit():
   rtnl_lock()
   wiphy_lock()                   nl80211_pre_doit():
                                    rtnl_lock() // blocked by Thread 1
 nl80211_vendor_cmd():
   doit()
     cfg80211_unregister_netdevice()
   rtnl_unlock():
     netdev_run_todo():
       __rtnl_unlock()
                                    <got RTNL lock>
                                    wiphy_lock() // blocked by Thread 1
       rtnl_lock(); // DEADLOCK
 nl80211_post_doit():
   wiphy_unlock();


Since I'm unlocking the RTNL inside nl80211_vendor_cmd() after calling doit()
instead of waiting till post_doit(), I get into the situation you mentioned
where the net_todo_list is not empty when calling rtnl_unlock. So I decided to
drop the rtnl_unlock() in nl80211_vendor_cmd() and defer that until
nl80211_post_doit() after calling wiphy_unlock(). With this change, I haven't
been able to reproduce the deadlock. So it's possible that we aren't actually
able to hit this deadlock in nl80211_pre_doit() with the existing code since,
as you mentioned, one wouldn't be able to call unregister_netdevice() without
having the RTNL lock.

Sorry if I sent you down a rabbit hole with the first code path scenario.

Thanks,
Will

[1] https://developer.android.com/guide/topics/connectivity/wifi-aware