netdev - Re: [PATCH net] vlan: Fix to delete vid only when by_dev has hw filter capable in vlan_vids_del_by

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZMkRvQFwMdjmcCh0@shredder>
Date: Tue, 1 Aug 2023 17:07:57 +0300
From: Ido Schimmel <idosch@...sch.org>
To: Ziyang Xuan <william.xuanziyang@...wei.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, bigeasy@...utronix.de,
	wsa+renesas@...g-engineering.com, kaber@...sh.net,
	netdev@...r.kernel.org
Subject: Re: [PATCH net] vlan: Fix to delete vid only when by_dev has hw
 filter capable in vlan_vids_del_by_dev()

On Tue, Aug 01, 2023 at 05:59:43PM +0800, Ziyang Xuan wrote:
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index 0beb44f2fe1f..79cf4f033b66 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -436,8 +436,11 @@ 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 (!vlan_hw_filter_capable(by_dev, vid_info->proto))
> +			continue;
>  		vlan_vid_del(dev, vid_info->proto, vid_info->vid);

vlan_vids_add_by_dev() does not have this check which means that memory
will leak [1] if the device is enslaved after the bond already has a
VLAN upper.

I believe the correct solution is to explicitly set the STAG-related
features [3] in the bond driver in a similar fashion to how it's done
for the CTAG features. That way the bond driver will always propagate
the VLAN info to its slaves.

Please check the team driver as well. I think it's suffering from the
same problem. If so, please fix it in a separate patch.

[1]
unreferenced object 0xffff888103efbd00 (size 256):
  comm "ip", pid 351, jiffies 4294763177 (age 19.697s)
  hex dump (first 32 bytes):                                                          
    00 10 7a 11 81 88 ff ff 00 00 00 00 00 00 00 00  ..z.............
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:                                                                          
    [<ffffffff81a88c0a>] kmalloc_trace+0x2a/0xe0                     
    [<ffffffff840f349c>] vlan_vid_add+0x30c/0x790
    [<ffffffff840f4a68>] vlan_vids_add_by_dev+0x148/0x390
    [<ffffffff82eea5e8>] bond_enslave+0xaf8/0x5d50
    [<ffffffff837fbfd1>] do_set_master+0x1c1/0x220       
    [<ffffffff8380711c>] do_setlink+0xa0c/0x3fa0  
    [<ffffffff8381ee09>] __rtnl_newlink+0xc09/0x18c0
    [<ffffffff8381fb2c>] rtnl_newlink+0x6c/0xa0 
    [<ffffffff8381d4ee>] rtnetlink_rcv_msg+0x43e/0xe00
    [<ffffffff83a2c920>] netlink_rcv_skb+0x170/0x440
    [<ffffffff83a2a2cf>] netlink_unicast+0x53f/0x810  
    [<ffffffff83a2af0b>] netlink_sendmsg+0x96b/0xe90
    [<ffffffff83708e0f>] ____sys_sendmsg+0x30f/0xa70
    [<ffffffff837129fa>] ___sys_sendmsg+0x13a/0x1e0 
    [<ffffffff83712c6c>] __sys_sendmsg+0x11c/0x1f0  
    [<ffffffff842fe5c8>] do_syscall_64+0x38/0x80   
unreferenced object 0xffff888112ffab60 (size 32):
  comm "ip", pid 351, jiffies 4294763177 (age 19.697s)
  hex dump (first 32 bytes):
    a0 bd ef 03 81 88 ff ff a0 bd ef 03 81 88 ff ff  ................
    88 a8 0a 00 01 00 00 00 cc cc cc cc cc cc cc cc  ................
  backtrace:
    [<ffffffff81a88c0a>] kmalloc_trace+0x2a/0xe0
    [<ffffffff840f3599>] vlan_vid_add+0x409/0x790
    [<ffffffff840f4a68>] vlan_vids_add_by_dev+0x148/0x390
    [<ffffffff82eea5e8>] bond_enslave+0xaf8/0x5d50
    [<ffffffff837fbfd1>] do_set_master+0x1c1/0x220
    [<ffffffff8380711c>] do_setlink+0xa0c/0x3fa0
    [<ffffffff8381ee09>] __rtnl_newlink+0xc09/0x18c0
    [<ffffffff8381fb2c>] rtnl_newlink+0x6c/0xa0
    [<ffffffff8381d4ee>] rtnetlink_rcv_msg+0x43e/0xe00
    [<ffffffff83a2c920>] netlink_rcv_skb+0x170/0x440
    [<ffffffff83a2a2cf>] netlink_unicast+0x53f/0x810
    [<ffffffff83a2af0b>] netlink_sendmsg+0x96b/0xe90
    [<ffffffff83708e0f>] ____sys_sendmsg+0x30f/0xa70
    [<ffffffff837129fa>] ___sys_sendmsg+0x13a/0x1e0
    [<ffffffff83712c6c>] __sys_sendmsg+0x11c/0x1f0
    [<ffffffff842fe5c8>] do_syscall_64+0x38/0x80

[2]
#!/bin/bash

ip link add name dummy1 type dummy
ip link add bond1 type bond mode 802.3ad
ip link add link bond1 name bond1.10 type vlan id 10 protocol 802.1ad
ip link set dev dummy1 master bond1
ip link del dev dummy1

[3]
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 484c9e3e5e82..447b06ea4fc9 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5901,7 +5901,9 @@ void bond_setup(struct net_device *bond_dev)
 
        bond_dev->hw_features = BOND_VLAN_FEATURES |
                                NETIF_F_HW_VLAN_CTAG_RX |
-                               NETIF_F_HW_VLAN_CTAG_FILTER;
+                               NETIF_F_HW_VLAN_CTAG_FILTER |
+                               NETIF_F_HW_VLAN_STAG_RX |
+                               NETIF_F_HW_VLAN_STAG_FILTER;
 
        bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL;
        bond_dev->features |= bond_dev->hw_features;

> +	}
>  }
>  EXPORT_SYMBOL(vlan_vids_del_by_dev);
>  
> -- 
> 2.25.1
> 
>