| Message ID | 20210407021903.384158-1-ducheng2@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | net: wireless: convert WARN_ON() to pr_warn() in cfg80211_sme_connect | expand |
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__); 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. Also, what can a user do if they get this information? What does it mean to them? Try making the text more informative. thanks, greg k-h
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
Hi, > 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. > Yeah, I think the right thing to do is to just drop the WARN_ON entirely. In fact, I can't really seem to figure out now why it was added there (even if I probably did that myself), nothing else seems to prevent getting to this code path multiple times directly one after another. johannes
Le Wed, Apr 07, 2021 at 05:51:05PM +0200, Johannes Berg a écrit : > Hi, > > > 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. > > > > Yeah, I think the right thing to do is to just drop the WARN_ON > entirely. In fact, I can't really seem to figure out now why it was > added there (even if I probably did that myself), nothing else seems to > prevent getting to this code path multiple times directly one after > another. > > johannes > Hi Johannes, Thanks for your input! I will send a v2 that drops the WARN_ON(). Regards, Du Cheng
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__); return -EINPROGRESS; + } wdev->conn = kzalloc(sizeof(*wdev->conn), GFP_KERNEL); if (!wdev->conn)
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(-)