* [PATCH net v2] net: check vlan filter feature in vlan_vids_add_by_dev() and vlan_vids_del_by_dev()
@ 2023-12-13 4:06 Liu Jian
2023-12-15 2:36 ` Jakub Kicinski
0 siblings, 1 reply; 3+ messages in thread
From: Liu Jian @ 2023-12-13 4:06 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, jiri, vladimir.oltean, andrew,
d-tatianin, justin.chen, rkannoth, idosch, jdamato, netdev
Cc: liujian56
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net v2] net: check vlan filter feature in vlan_vids_add_by_dev() and vlan_vids_del_by_dev()
2023-12-13 4:06 [PATCH net v2] net: check vlan filter feature in vlan_vids_add_by_dev() and vlan_vids_del_by_dev() Liu Jian
@ 2023-12-15 2:36 ` Jakub Kicinski
2023-12-16 7:40 ` liujian (CE)
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Kicinski @ 2023-12-15 2:36 UTC (permalink / raw)
To: Liu Jian
Cc: davem, edumazet, pabeni, jiri, vladimir.oltean, andrew,
d-tatianin, justin.chen, rkannoth, idosch, jdamato, netdev
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net v2] net: check vlan filter feature in vlan_vids_add_by_dev() and vlan_vids_del_by_dev()
2023-12-15 2:36 ` Jakub Kicinski
@ 2023-12-16 7:40 ` liujian (CE)
0 siblings, 0 replies; 3+ messages in thread
From: liujian (CE) @ 2023-12-16 7:40 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, edumazet, pabeni, jiri, vladimir.oltean, andrew,
d-tatianin, justin.chen, rkannoth, idosch, jdamato, netdev
在 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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-12-16 7:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-13 4:06 [PATCH net v2] net: check vlan filter feature in vlan_vids_add_by_dev() and vlan_vids_del_by_dev() Liu Jian
2023-12-15 2:36 ` Jakub Kicinski
2023-12-16 7:40 ` liujian (CE)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).