| Message ID | 20230901035301.3473463-1-shaozhengchao@huawei.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] wifi: mac80211: fix WARNING in ieee80211_link_info_change_notify() | expand |
Zhengchao Shao <shaozhengchao@huawei.com> writes: > Syz reports the following WARNING: > wlan0: Failed check-sdata-in-driver check, flags: 0x0 > WARNING: CPU: 3 PID: 5384 at net/mac80211/main.c:287 > ieee80211_link_info_change_notify+0x1c2/0x230 > Modules linked in: > RIP: 0010:ieee80211_link_info_change_notify+0x1c2/0x230 > Call Trace: > <TASK> > ieee80211_set_mcast_rate+0x3e/0x50 > nl80211_set_mcast_rate+0x316/0x650 > genl_family_rcv_msg_doit+0x20b/0x300 > genl_rcv_msg+0x39f/0x6a0 > netlink_rcv_skb+0x13b/0x3b0 > genl_rcv+0x24/0x40 > netlink_unicast+0x4a2/0x740 > netlink_sendmsg+0x83e/0xce0 > sock_sendmsg+0xc5/0x100 > ____sys_sendmsg+0x583/0x690 > ___sys_sendmsg+0xe8/0x160 > __sys_sendmsg+0xbf/0x160 > do_syscall_64+0x35/0x80 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > </TASK> > > The execution process is as follows: > Thread A: > ieee80211_open() > ieee80211_do_open() > drv_add_interface() //set IEEE80211_SDATA_IN_DRIVER flag > ... > rtnl_newlink > do_setlink > dev_change_flags > ... > __dev_close_many > ieee80211_stop() > ieee80211_do_stop() > drv_remove_interface() //clear flag > ... > nl80211_set_mcast_rate() > ieee80211_set_mcast_rate() > ieee80211_link_info_change_notify() > check_sdata_in_driver() //WARNING because flag is cleared > > When the wlan device stops, the IEEE80211_SDATA_IN_ DRIVER flag is cleared. > And then after the set mcast rate command is executed, WARNING is generated > because the flag bit has been already cleared. > > Fixes: 591e73ee3f73 ("wifi: mac80211: properly skip link info driver update") > Reported-by: syzbot+bce2ca140cc00578ed07@syzkaller.appspotmail.com > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> Wireless patches (ie. anything which starts with "wifi:") go to wireless and wireless-next trees, not to net tree.
On 2023/9/1 12:38, Kalle Valo wrote: > Zhengchao Shao <shaozhengchao@huawei.com> writes: > >> Syz reports the following WARNING: >> wlan0: Failed check-sdata-in-driver check, flags: 0x0 >> WARNING: CPU: 3 PID: 5384 at net/mac80211/main.c:287 >> ieee80211_link_info_change_notify+0x1c2/0x230 >> Modules linked in: >> RIP: 0010:ieee80211_link_info_change_notify+0x1c2/0x230 >> Call Trace: >> <TASK> >> ieee80211_set_mcast_rate+0x3e/0x50 >> nl80211_set_mcast_rate+0x316/0x650 >> genl_family_rcv_msg_doit+0x20b/0x300 >> genl_rcv_msg+0x39f/0x6a0 >> netlink_rcv_skb+0x13b/0x3b0 >> genl_rcv+0x24/0x40 >> netlink_unicast+0x4a2/0x740 >> netlink_sendmsg+0x83e/0xce0 >> sock_sendmsg+0xc5/0x100 >> ____sys_sendmsg+0x583/0x690 >> ___sys_sendmsg+0xe8/0x160 >> __sys_sendmsg+0xbf/0x160 >> do_syscall_64+0x35/0x80 >> entry_SYSCALL_64_after_hwframe+0x46/0xb0 >> </TASK> >> >> The execution process is as follows: >> Thread A: >> ieee80211_open() >> ieee80211_do_open() >> drv_add_interface() //set IEEE80211_SDATA_IN_DRIVER flag >> ... >> rtnl_newlink >> do_setlink >> dev_change_flags >> ... >> __dev_close_many >> ieee80211_stop() >> ieee80211_do_stop() >> drv_remove_interface() //clear flag >> ... >> nl80211_set_mcast_rate() >> ieee80211_set_mcast_rate() >> ieee80211_link_info_change_notify() >> check_sdata_in_driver() //WARNING because flag is cleared >> >> When the wlan device stops, the IEEE80211_SDATA_IN_ DRIVER flag is cleared. >> And then after the set mcast rate command is executed, WARNING is generated >> because the flag bit has been already cleared. >> >> Fixes: 591e73ee3f73 ("wifi: mac80211: properly skip link info driver update") >> Reported-by: syzbot+bce2ca140cc00578ed07@syzkaller.appspotmail.com >> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > > Wireless patches (ie. anything which starts with "wifi:") go to wireless > and wireless-next trees, not to net tree. > Thank you for your reminder. I'll pay attention next time. Zhengchao Shao
On Fri, 2023-09-01 at 11:53 +0800, Zhengchao Shao wrote: > > diff --git a/net/mac80211/main.c b/net/mac80211/main.c > index 24315d7b3126..f79e2343dddd 100644 > --- a/net/mac80211/main.c > +++ b/net/mac80211/main.c > @@ -285,6 +285,9 @@ void ieee80211_link_info_change_notify(struct ieee80211_sub_if_data *sdata, > if (!changed || sdata->vif.type == NL80211_IFTYPE_AP_VLAN) > return; > > + if (!ieee80211_sdata_running(sdata)) > + return; > + > if (!check_sdata_in_driver(sdata)) > return; > I don't think this is right. Do you see anything else checking that it's running right before checking it's in the driver? :) Why can we even get into this call at all? I think the problem is already in cfg80211 allowing this. johannes
On 2023/9/1 14:32, Johannes Berg wrote: > On Fri, 2023-09-01 at 11:53 +0800, Zhengchao Shao wrote: >> >> diff --git a/net/mac80211/main.c b/net/mac80211/main.c >> index 24315d7b3126..f79e2343dddd 100644 >> --- a/net/mac80211/main.c >> +++ b/net/mac80211/main.c >> @@ -285,6 +285,9 @@ void ieee80211_link_info_change_notify(struct ieee80211_sub_if_data *sdata, >> if (!changed || sdata->vif.type == NL80211_IFTYPE_AP_VLAN) >> return; >> >> + if (!ieee80211_sdata_running(sdata)) >> + return; >> + >> if (!check_sdata_in_driver(sdata)) >> return; >> > > I don't think this is right. Do you see anything else checking that it's > running right before checking it's in the driver? :) > > Why can we even get into this call at all? I think the problem is > already in cfg80211 allowing this. > > johannes > Hi johannes: Do you mean it shouldn't be allowed to set mcast rate when dev is stopped, as in the following code? --- a/net/wireless/rdev-ops.h +++ b/net/wireless/rdev-ops.h @@ -1229,7 +1229,7 @@ rdev_set_mcast_rate(struct cfg80211_registered_device *rdev, int ret = -ENOTSUPP; trace_rdev_set_mcast_rate(&rdev->wiphy, dev, mcast_rate); - if (rdev->ops->set_mcast_rate) + if (rdev->ops->set_mcast_rate && netif_running(dev)) ret = rdev->ops->set_mcast_rate(&rdev->wiphy, dev, mcast_rate); trace_rdev_return_int(&rdev->wiphy, ret); return ret; Thank you Zhengchao Shao
Hi, > Do you mean it shouldn't be allowed to set mcast rate when dev > is stopped, > Probably? > as in the following code? > > --- a/net/wireless/rdev-ops.h > +++ b/net/wireless/rdev-ops.h > @@ -1229,7 +1229,7 @@ rdev_set_mcast_rate(struct > cfg80211_registered_device *rdev, > int ret = -ENOTSUPP; > > trace_rdev_set_mcast_rate(&rdev->wiphy, dev, mcast_rate); > - if (rdev->ops->set_mcast_rate) > + if (rdev->ops->set_mcast_rate && netif_running(dev)) > ret = rdev->ops->set_mcast_rate(&rdev->wiphy, dev, > Certainly not. Please don't do random patches without looking at the subsystem as a whole. If you don't want to take the time to understand how things work in wireless, then better don't send patches at all. johannes
On 2023/9/1 17:19, Johannes Berg wrote: > Hi, > > >> Do you mean it shouldn't be allowed to set mcast rate when dev >> is stopped, >> > > Probably? > >> as in the following code? >> >> --- a/net/wireless/rdev-ops.h >> +++ b/net/wireless/rdev-ops.h >> @@ -1229,7 +1229,7 @@ rdev_set_mcast_rate(struct >> cfg80211_registered_device *rdev, >> int ret = -ENOTSUPP; >> >> trace_rdev_set_mcast_rate(&rdev->wiphy, dev, mcast_rate); >> - if (rdev->ops->set_mcast_rate) >> + if (rdev->ops->set_mcast_rate && netif_running(dev)) >> ret = rdev->ops->set_mcast_rate(&rdev->wiphy, dev, >> > > Certainly not. Please don't do random patches without looking at the > subsystem as a whole. If you don't want to take the time to understand > how things work in wireless, then better don't send patches at all. > > johannes Hi johannes: It's a little difficult for me to solve this warning. This warning has been going on for more than one year. Could you help solve it? Thank you. links: https://groups.google.com/g/syzkaller-bugs/c/FofxpVlkONg/m/v296EFNnAAAJ Zhengchao Shao
diff --git a/net/mac80211/main.c b/net/mac80211/main.c index 24315d7b3126..f79e2343dddd 100644 --- a/net/mac80211/main.c +++ b/net/mac80211/main.c @@ -285,6 +285,9 @@ void ieee80211_link_info_change_notify(struct ieee80211_sub_if_data *sdata, if (!changed || sdata->vif.type == NL80211_IFTYPE_AP_VLAN) return; + if (!ieee80211_sdata_running(sdata)) + return; + if (!check_sdata_in_driver(sdata)) return;
Syz reports the following WARNING: wlan0: Failed check-sdata-in-driver check, flags: 0x0 WARNING: CPU: 3 PID: 5384 at net/mac80211/main.c:287 ieee80211_link_info_change_notify+0x1c2/0x230 Modules linked in: RIP: 0010:ieee80211_link_info_change_notify+0x1c2/0x230 Call Trace: <TASK> ieee80211_set_mcast_rate+0x3e/0x50 nl80211_set_mcast_rate+0x316/0x650 genl_family_rcv_msg_doit+0x20b/0x300 genl_rcv_msg+0x39f/0x6a0 netlink_rcv_skb+0x13b/0x3b0 genl_rcv+0x24/0x40 netlink_unicast+0x4a2/0x740 netlink_sendmsg+0x83e/0xce0 sock_sendmsg+0xc5/0x100 ____sys_sendmsg+0x583/0x690 ___sys_sendmsg+0xe8/0x160 __sys_sendmsg+0xbf/0x160 do_syscall_64+0x35/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 </TASK> The execution process is as follows: Thread A: ieee80211_open() ieee80211_do_open() drv_add_interface() //set IEEE80211_SDATA_IN_DRIVER flag ... rtnl_newlink do_setlink dev_change_flags ... __dev_close_many ieee80211_stop() ieee80211_do_stop() drv_remove_interface() //clear flag ... nl80211_set_mcast_rate() ieee80211_set_mcast_rate() ieee80211_link_info_change_notify() check_sdata_in_driver() //WARNING because flag is cleared When the wlan device stops, the IEEE80211_SDATA_IN_ DRIVER flag is cleared. And then after the set mcast rate command is executed, WARNING is generated because the flag bit has been already cleared. Fixes: 591e73ee3f73 ("wifi: mac80211: properly skip link info driver update") Reported-by: syzbot+bce2ca140cc00578ed07@syzkaller.appspotmail.com Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- v2: modify commit info --- net/mac80211/main.c | 3 +++ 1 file changed, 3 insertions(+)