Re: [PATCH] net: wireless: convert WARN_ON() to pr_warn() in cfg80211_sme_connect

[Du Cheng <ducheng2@gmail.com>]

Le Wed, Apr 07, 2021 at 07:54:22AM +0200, Greg Kroah-Hartman a écrit :
> On Wed, Apr 07, 2021 at 10:19:03AM +0800, Du Cheng wrote:
> > A WARN_ON(wdev->conn) would trigger in cfg80211_sme_connect(), if
> > multiple send_msg() system calls are made from the userland, which
> > should be anticipated and handled by the wireless driver. Convert this
> > WARN() to pr_warn to prevent a kernel panic if kernel is configured to
> > "panic on warn".
> > 
> > Bug reported by syzbot.
> > 
> > Reported-by: syzbot+5f9392825de654244975@syzkaller.appspotmail.com
> > Signed-off-by: Du Cheng <ducheng2@gmail.com>
> > ---
> > link to syzkaller:
> > https://syzkaller.appspot.com/bug?extid=5f9392825de654244975
> > this patch has passed syzbot test.
> >  net/wireless/sme.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/wireless/sme.c b/net/wireless/sme.c
> > index 07756ca5e3b5..87a65a4c40ae 100644
> > --- a/net/wireless/sme.c
> > +++ b/net/wireless/sme.c
> > @@ -529,8 +529,10 @@ static int cfg80211_sme_connect(struct wireless_dev *wdev,
> >  		cfg80211_sme_free(wdev);
> >  	}
> >  
> > -	if (WARN_ON(wdev->conn))
> > +	if (wdev->conn) {
> > +		pr_warn("%s: wdev->conn != NULL, sme connect in progress", __func__);
> 

Hi Greg,

Thanks for the reply.

> You have a real device, please always use 'dev_warn() for stuff like
> this, or the netdev equivalent.  Also no need for __func__ for stuff
> like this, that's just clutter.

If the warning is indeed useful here, I will change the line to dev_warn(),
however I am not sure if it is a good idea to even generate warning output as
the kernel is well able to handle this special case from the userland.
> 
> Also, what can a user do if they get this information?  What does it
> mean to them?  Try making the text more informative.
> 
> thanks,

I have spent some time to understand the netlink subsystem as a IPC mechanism.
What I have now is a reliable sequence of steps to reproduce the crash, by
condensing the syzkaller C reproducer:
[link to reproducer: https://syzkaller.appspot.com/text?tag=ReproC&x=1414c997900000]

* hwsim80211_create_device (sendmsg: HWSIM_CMD_NEW_RADIO)
* nl80211_set_interface (sendmsg: NL80211_CMD_SET_INTERFACE)
* set_interface_state (ioctl: SIOCSIFFLAGS)
* nl80211_join_ibss (sendmsg: NL80211_CMD_JOIN_IBSS)
* sendmsg: NL80211_CMD_SET_INTERFACE
* 1st sendmsg: NL80211_CMD_CONNECT
* 2nd sendmsg: NL80211_CMD_CONNECT <- this triggers the WARN_ON(wdev->conn)
* (if kernel not panic yet) more sendmsg: NL80211_CMD_CONNECT ...

If the code skips WARN_ON() and instead returns -EINPROGRESS, user application
will receive error from the following recv(sock, ...). User application can hence
choose to handle this error accordingly while kernel soldiers on without panicking.

If dev_warn() is added, for every subsequent NL80211_CMD_CONNECT, the console is
flooded with the printout.

Maybe it is ok to silently return -EINPROGRESS for the 2nd NL80211_CMD_CONNECT
and beyond.
> 
> greg k-h