[PATCH] staging: rtl8723bs: fix monitor netdev register/unregister

[PATCH] staging: rtl8723bs: fix monitor netdev register/unregister

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Due to the locking changes and callbacks happening inside
cfg80211, we need to use cfg80211 versions of the register
and unregister functions if called within cfg80211 methods,
otherwise deadlocks occur.

Fixes: a05829a7222e ("cfg80211: avoid holding the RTNL when calling the driver")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index ff164a8c8679..0619a7510e83 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -2579,7 +2579,7 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
 	mon_wdev->iftype = NL80211_IFTYPE_MONITOR;
 	mon_ndev->ieee80211_ptr = mon_wdev;
 
-	ret = register_netdevice(mon_ndev);
+	ret = cfg80211_register_netdevice(mon_ndev);
 	if (ret) {
 		goto out;
 	}
@@ -2661,7 +2661,7 @@ static int cfg80211_rtw_del_virtual_intf(struct wiphy *wiphy,
 	adapter = rtw_netdev_priv(ndev);
 	pwdev_priv = adapter_wdev_data(adapter);
 
-	unregister_netdevice(ndev);
+	cfg80211_unregister_netdevice(ndev);
 
 	if (ndev == pwdev_priv->pmon_ndev) {
 		pwdev_priv->pmon_ndev = NULL;
-- 
2.30.2

Re: [PATCH] staging: rtl8723bs: fix monitor netdev register/unregister

[Greg KH]

On Mon, Apr 26, 2021 at 09:28:02PM +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> Due to the locking changes and callbacks happening inside
> cfg80211, we need to use cfg80211 versions of the register
> and unregister functions if called within cfg80211 methods,
> otherwise deadlocks occur.
> 
> Fixes: a05829a7222e ("cfg80211: avoid holding the RTNL when calling the driver")
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> index ff164a8c8679..0619a7510e83 100644
> --- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> @@ -2579,7 +2579,7 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
>  	mon_wdev->iftype = NL80211_IFTYPE_MONITOR;
>  	mon_ndev->ieee80211_ptr = mon_wdev;
>  
> -	ret = register_netdevice(mon_ndev);
> +	ret = cfg80211_register_netdevice(mon_ndev);

Is this now a requirement for all wireless drivers?

If so, do other drivers/staging/ drivers need to also be fixed up?

I'm guessing this will be going through the wireless tree, so:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

thanks,

greg k-h

Re: [PATCH] staging: rtl8723bs: fix monitor netdev register/unregister

[Johannes Berg]

On Tue, 2021-04-27 at 08:25 +0200, Greg KH wrote:
> 
> > +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > @@ -2579,7 +2579,7 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
> >  	mon_wdev->iftype = NL80211_IFTYPE_MONITOR;
> >  	mon_ndev->ieee80211_ptr = mon_wdev;
> > 
> > -	ret = register_netdevice(mon_ndev);
> > +	ret = cfg80211_register_netdevice(mon_ndev);
> 
> Is this now a requirement for all wireless drivers?

Yes and no. It must only be called from within a "please add an
interface" method. Otherwise, register_netdevice() must still be called.

> If so, do other drivers/staging/ drivers need to also be fixed up?

Not as far as I can tell, this is the only wireless staging driver that
even calls register_netdevice(). Not sure why I missed this, I had
audited all of those calls across the tree. But looking a second time
always shows more I guess, sorry about that.

There's another call to register_netdevice() here but I don't think
that's affected, however, it's obviously utterly broken in the first
place:

        if (!rtnl_is_locked())
                unregister_netdev(cur_pnetdev);
        else
                unregister_netdevice(cur_pnetdev);

*sigh*.

> I'm guessing this will be going through the wireless tree, so:
> 
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

I don't care much, since unfortunately it's already too late and the
breakage is released in 5.12. I'll pick it up through my tree since I
broke it (and probably should add a Cc stable tag.)

johannes

Re: [PATCH] staging: rtl8723bs: fix monitor netdev register/unregister

[Greg KH]

On Tue, Apr 27, 2021 at 10:53:58AM +0200, Johannes Berg wrote:
> On Tue, 2021-04-27 at 08:25 +0200, Greg KH wrote:
> > 
> > > +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > > @@ -2579,7 +2579,7 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
> > >  	mon_wdev->iftype = NL80211_IFTYPE_MONITOR;
> > >  	mon_ndev->ieee80211_ptr = mon_wdev;
> > > 
> > > -	ret = register_netdevice(mon_ndev);
> > > +	ret = cfg80211_register_netdevice(mon_ndev);
> > 
> > Is this now a requirement for all wireless drivers?
> 
> Yes and no. It must only be called from within a "please add an
> interface" method. Otherwise, register_netdevice() must still be called.
> 
> > If so, do other drivers/staging/ drivers need to also be fixed up?
> 
> Not as far as I can tell, this is the only wireless staging driver that
> even calls register_netdevice(). Not sure why I missed this, I had
> audited all of those calls across the tree. But looking a second time
> always shows more I guess, sorry about that.
> 
> There's another call to register_netdevice() here but I don't think
> that's affected, however, it's obviously utterly broken in the first
> place:
> 
>         if (!rtnl_is_locked())
>                 unregister_netdev(cur_pnetdev);
>         else
>                 unregister_netdevice(cur_pnetdev);
> 
> *sigh*.

Sorry, these staging wireless drivers are really getting annoying.
Maybe I need to turn an intern onto them to just get them fixed up and
out of here to be a 'real' driver.

> > I'm guessing this will be going through the wireless tree, so:
> > 
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> I don't care much, since unfortunately it's already too late and the
> breakage is released in 5.12. I'll pick it up through my tree since I
> broke it (and probably should add a Cc stable tag.)

Great, thanks for taking it that way and doing this fix, much
appreciated.

greg k-h

Re: [PATCH] staging: rtl8723bs: fix monitor netdev register/unregister

[Johannes Berg]

On Tue, 2021-04-27 at 11:03 +0200, Greg KH wrote:
> > There's another call to register_netdevice() here but I don't think
> > that's affected, however, it's obviously utterly broken in the first
> > place:
> > 
> >         if (!rtnl_is_locked())
> >                 unregister_netdev(cur_pnetdev);
> >         else
> >                 unregister_netdevice(cur_pnetdev);
> > 
> > *sigh*.
> 
> Sorry, these staging wireless drivers are really getting annoying.
> Maybe I need to turn an intern onto them to just get them fixed up and
> out of here to be a 'real' driver.

:)

FWIW, in this case it looks like it's actually not even incorrect,
because it's guaranteed to already hold the RTNL *itself*. Just all that
code can be removed.

johannes