[PATCH net v2] net: check vlan filter feature in vlan_vids_add_by_dev() and vlan_vids_del_by_dev()

[PATCH net v2] net: check vlan filter feature in vlan_vids_add_by_dev() and vlan_vids_del_by_dev()

[Liu Jian]

I got the bleow warning trace:

WARNING: CPU: 4 PID: 4056 at net/core/dev.c:11066 unregister_netdevice_many_notify
CPU: 4 PID: 4056 Comm: ip Not tainted 6.7.0-rc4+ #15
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
RIP: 0010:unregister_netdevice_many_notify+0x9a4/0x9b0
Call Trace:
 rtnl_dellink
 rtnetlink_rcv_msg
 netlink_rcv_skb
 netlink_unicast
 netlink_sendmsg
 __sock_sendmsg
 ____sys_sendmsg
 ___sys_sendmsg
 __sys_sendmsg
 do_syscall_64
 entry_SYSCALL_64_after_hwframe

It can be repoduced via:

    ip netns add ns1
    ip netns exec ns1 ip link add bond0 type bond mode 0
    ip netns exec ns1 ip link add bond_slave_1 type veth peer veth2
    ip netns exec ns1 ip link set bond_slave_1 master bond0
[1] ip netns exec ns1 ethtool -K bond0 rx-vlan-filter off
[2] ip netns exec ns1 ip link add link bond_slave_1 name bond_slave_1.0 type vlan id 0
[3] ip netns exec ns1 ip link add link bond0 name bond0.0 type vlan id 0
[4] ip netns exec ns1 ip link set bond_slave_1 nomaster
[5] ip netns exec ns1 ip link del veth2
    ip netns del ns1

This is all caused by command [1] turning off the rx-vlan-filter function
of bond0. The reason is the same as commit 01f4fd270870 ("bonding: Fix
incorrect deletion of ETH_P_8021AD protocol vid from slaves"). Commands
[2] [3] add the same vid to slave and master respectively, causing
command [4] to empty slave->vlan_info. The following command [5] triggers
this problem.

To fix this problem, we should add VLAN_FILTER feature checks in
vlan_vids_add_by_dev() and vlan_vids_del_by_dev() to prevent incorrect
addition or deletion of vlan_vid information.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Liu Jian <liujian56@huawei.com>
---
v1->v2: Modify patch title and commit message.
	Remove superfluous operations in ethtool/features.c and ioctl.c
 net/8021q/vlan_core.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 0beb44f2fe1f..e94b509386bb 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -407,6 +407,12 @@ int vlan_vids_add_by_dev(struct net_device *dev,
 		return 0;
 
 	list_for_each_entry(vid_info, &vlan_info->vid_list, list) {
+		if (!(by_dev->features & NETIF_F_HW_VLAN_CTAG_FILTER) &&
+		    vid_info->proto == htons(ETH_P_8021Q))
+			continue;
+		if (!(by_dev->features & NETIF_F_HW_VLAN_STAG_FILTER) &&
+		    vid_info->proto == htons(ETH_P_8021AD))
+			continue;
 		err = vlan_vid_add(dev, vid_info->proto, vid_info->vid);
 		if (err)
 			goto unwind;
@@ -417,6 +423,12 @@ int vlan_vids_add_by_dev(struct net_device *dev,
 	list_for_each_entry_continue_reverse(vid_info,
 					     &vlan_info->vid_list,
 					     list) {
+		if (!(by_dev->features & NETIF_F_HW_VLAN_CTAG_FILTER) &&
+		    vid_info->proto == htons(ETH_P_8021Q))
+			continue;
+		if (!(by_dev->features & NETIF_F_HW_VLAN_STAG_FILTER) &&
+		    vid_info->proto == htons(ETH_P_8021AD))
+			continue;
 		vlan_vid_del(dev, vid_info->proto, vid_info->vid);
 	}
 
@@ -436,8 +448,15 @@ void vlan_vids_del_by_dev(struct net_device *dev,
 	if (!vlan_info)
 		return;
 
-	list_for_each_entry(vid_info, &vlan_info->vid_list, list)
+	list_for_each_entry(vid_info, &vlan_info->vid_list, list) {
+		if (!(by_dev->features & NETIF_F_HW_VLAN_CTAG_FILTER) &&
+		    vid_info->proto == htons(ETH_P_8021Q))
+			continue;
+		if (!(by_dev->features & NETIF_F_HW_VLAN_STAG_FILTER) &&
+		    vid_info->proto == htons(ETH_P_8021AD))
+			continue;
 		vlan_vid_del(dev, vid_info->proto, vid_info->vid);
+	}
 }
 EXPORT_SYMBOL(vlan_vids_del_by_dev);
 
-- 
2.34.1

Re: [PATCH net v2] net: check vlan filter feature in vlan_vids_add_by_dev() and vlan_vids_del_by_dev()

[Jakub Kicinski]

On Wed, 13 Dec 2023 12:06:41 +0800 Liu Jian wrote:
> I got the bleow warning trace:

s/bleow/below/

> WARNING: CPU: 4 PID: 4056 at net/core/dev.c:11066 unregister_netdevice_many_notify
> CPU: 4 PID: 4056 Comm: ip Not tainted 6.7.0-rc4+ #15
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
> RIP: 0010:unregister_netdevice_many_notify+0x9a4/0x9b0
> Call Trace:
>  rtnl_dellink
>  rtnetlink_rcv_msg
>  netlink_rcv_skb
>  netlink_unicast
>  netlink_sendmsg
>  __sock_sendmsg
>  ____sys_sendmsg
>  ___sys_sendmsg
>  __sys_sendmsg
>  do_syscall_64
>  entry_SYSCALL_64_after_hwframe
> 
> It can be repoduced via:
> 
>     ip netns add ns1
>     ip netns exec ns1 ip link add bond0 type bond mode 0
>     ip netns exec ns1 ip link add bond_slave_1 type veth peer veth2
>     ip netns exec ns1 ip link set bond_slave_1 master bond0
> [1] ip netns exec ns1 ethtool -K bond0 rx-vlan-filter off
> [2] ip netns exec ns1 ip link add link bond_slave_1 name bond_slave_1.0 type vlan id 0
> [3] ip netns exec ns1 ip link add link bond0 name bond0.0 type vlan id 0
> [4] ip netns exec ns1 ip link set bond_slave_1 nomaster
> [5] ip netns exec ns1 ip link del veth2
>     ip netns del ns1

Could you construct a selftest based on those commands?

> This is all caused by command [1] turning off the rx-vlan-filter function
> of bond0. The reason is the same as commit 01f4fd270870 ("bonding: Fix
> incorrect deletion of ETH_P_8021AD protocol vid from slaves"). Commands
> [2] [3] add the same vid to slave and master respectively, causing
> command [4] to empty slave->vlan_info. The following command [5] triggers
> this problem.
> 
> To fix this problem, we should add VLAN_FILTER feature checks in
> vlan_vids_add_by_dev() and vlan_vids_del_by_dev() to prevent incorrect
> addition or deletion of vlan_vid information.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

Did the STAG/CTAG features exist in 2.6? I thought I saw the commit
that added them in git at some point. Could be misremembering...

> Signed-off-by: Liu Jian <liujian56@huawei.com>
> ---
> v1->v2: Modify patch title and commit message.
> 	Remove superfluous operations in ethtool/features.c and ioctl.c
>  net/8021q/vlan_core.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index 0beb44f2fe1f..e94b509386bb 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -407,6 +407,12 @@ int vlan_vids_add_by_dev(struct net_device *dev,
>  		return 0;
>  
>  	list_for_each_entry(vid_info, &vlan_info->vid_list, list) {
> +		if (!(by_dev->features & NETIF_F_HW_VLAN_CTAG_FILTER) &&
> +		    vid_info->proto == htons(ETH_P_8021Q))
> +			continue;
> +		if (!(by_dev->features & NETIF_F_HW_VLAN_STAG_FILTER) &&
> +		    vid_info->proto == htons(ETH_P_8021AD))
> +			continue;

this code is copied 3 times, could you please factor it out to a helper
taking dev and vid_info and deciding if the walk should skip?
-- 
pw-bot: cr

Re: [PATCH net v2] net: check vlan filter feature in vlan_vids_add_by_dev() and vlan_vids_del_by_dev()

[liujian (CE)]

在 2023/12/15 10:36, Jakub Kicinski 写道:
> On Wed, 13 Dec 2023 12:06:41 +0800 Liu Jian wrote:
>> I got the bleow warning trace:
> 
> s/bleow/below/
> 
>> WARNING: CPU: 4 PID: 4056 at net/core/dev.c:11066 unregister_netdevice_many_notify
>> CPU: 4 PID: 4056 Comm: ip Not tainted 6.7.0-rc4+ #15
>> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
>> RIP: 0010:unregister_netdevice_many_notify+0x9a4/0x9b0
>> Call Trace:
>>   rtnl_dellink
>>   rtnetlink_rcv_msg
>>   netlink_rcv_skb
>>   netlink_unicast
>>   netlink_sendmsg
>>   __sock_sendmsg
>>   ____sys_sendmsg
>>   ___sys_sendmsg
>>   __sys_sendmsg
>>   do_syscall_64
>>   entry_SYSCALL_64_after_hwframe
>>
>> It can be repoduced via:
>>
>>      ip netns add ns1
>>      ip netns exec ns1 ip link add bond0 type bond mode 0
>>      ip netns exec ns1 ip link add bond_slave_1 type veth peer veth2
>>      ip netns exec ns1 ip link set bond_slave_1 master bond0
>> [1] ip netns exec ns1 ethtool -K bond0 rx-vlan-filter off
>> [2] ip netns exec ns1 ip link add link bond_slave_1 name bond_slave_1.0 type vlan id 0
>> [3] ip netns exec ns1 ip link add link bond0 name bond0.0 type vlan id 0
>> [4] ip netns exec ns1 ip link set bond_slave_1 nomaster
>> [5] ip netns exec ns1 ip link del veth2
>>      ip netns del ns1
> 
> Could you construct a selftest based on those commands?
OK.
> 
>> This is all caused by command [1] turning off the rx-vlan-filter function
>> of bond0. The reason is the same as commit 01f4fd270870 ("bonding: Fix
>> incorrect deletion of ETH_P_8021AD protocol vid from slaves"). Commands
>> [2] [3] add the same vid to slave and master respectively, causing
>> command [4] to empty slave->vlan_info. The following command [5] triggers
>> this problem.
>>
>> To fix this problem, we should add VLAN_FILTER feature checks in
>> vlan_vids_add_by_dev() and vlan_vids_del_by_dev() to prevent incorrect
>> addition or deletion of vlan_vid information.
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> 
> Did the STAG/CTAG features exist in 2.6? I thought I saw the commit
> that added them in git at some point. Could be misremembering...
I just saw the feature NETIF_F_HW_VLAN_FILTER 
(NETIF_F_HW_VLAN_CTAG_FILTER) in this tag.
Now I find that the following tag may be more suitable.
348a1443cc43 ("vlan: introduce functions to do mass addition/deletion of 
vids by another device")
> 
>> Signed-off-by: Liu Jian <liujian56@huawei.com>
>> ---
>> v1->v2: Modify patch title and commit message.
>> 	Remove superfluous operations in ethtool/features.c and ioctl.c
>>   net/8021q/vlan_core.c | 21 ++++++++++++++++++++-
>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>> index 0beb44f2fe1f..e94b509386bb 100644
>> --- a/net/8021q/vlan_core.c
>> +++ b/net/8021q/vlan_core.c
>> @@ -407,6 +407,12 @@ int vlan_vids_add_by_dev(struct net_device *dev,
>>   		return 0;
>>   
>>   	list_for_each_entry(vid_info, &vlan_info->vid_list, list) {
>> +		if (!(by_dev->features & NETIF_F_HW_VLAN_CTAG_FILTER) &&
>> +		    vid_info->proto == htons(ETH_P_8021Q))
>> +			continue;
>> +		if (!(by_dev->features & NETIF_F_HW_VLAN_STAG_FILTER) &&
>> +		    vid_info->proto == htons(ETH_P_8021AD))
>> +			continue;
> 
> this code is copied 3 times, could you please factor it out to a helper
> taking dev and vid_info and deciding if the walk should skip?

Find a suitable existing function vlan_hw_filter_capable().
Thanks for your review.