[PATCH net,v2] wifi: mac80211: fix WARNING in ieee80211_link_info_change_notify()

[PATCH net,v2] wifi: mac80211: fix WARNING in ieee80211_link_info_change_notify()

[Zhengchao Shao]

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(+)

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;
 
-- 
2.34.1

Re: [PATCH net,v2] wifi: mac80211: fix WARNING in ieee80211_link_info_change_notify()

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH net,v2] wifi: mac80211: fix WARNING in ieee80211_link_info_change_notify()

[shaozhengchao]

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

Re: [PATCH net,v2] wifi: mac80211: fix WARNING in ieee80211_link_info_change_notify()

[Johannes Berg]

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 

Re: [PATCH net,v2] wifi: mac80211: fix WARNING in ieee80211_link_info_change_notify()

[shaozhengchao]

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

Re: [PATCH net,v2] wifi: mac80211: fix WARNING in ieee80211_link_info_change_notify()

[Johannes Berg]

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

Re: [PATCH net,v2] wifi: mac80211: fix WARNING in ieee80211_link_info_change_notify()

[shaozhengchao]

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