netdev - Re: [RFC PATCH 4/9] net: dsa: convert denying bridge VLAN with existing 8021q upper to PRECHANGEUPPER

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]

Date:   Sat, 19 Sep 2020 19:37:05 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org,
        davem@...emloft.net
Cc:     andrew@...n.ch, vivien.didelot@...il.com, idosch@...sch.org,
        jiri@...nulli.us, kurt.kanzenbach@...utronix.de, kuba@...nel.org
Subject: Re: [RFC PATCH 4/9] net: dsa: convert denying bridge VLAN with
 existing 8021q upper to PRECHANGEUPPER



On 9/19/2020 6:47 PM, Vladimir Oltean wrote:
> This is checking for the following order of operations, and makes sure
> to deny that configuration:
> 
> ip link add link swp2 name swp2.100 type vlan id 100
> ip link add br0 type bridge vlan_filtering 1
> ip link set swp2 master br0
> bridge vlan add dev swp2 vid 100
> 
> Instead of using vlan_for_each(), which looks at the VLAN filters
> installed with vlan_vid_add(), just track the 8021q uppers. This has the
> advantage of freeing up the vlan_vid_add() call for actual VLAN
> filtering.
> 
> There is another change in this patch. The check is moved in slave.c,
> from switch.c. I don't think it makes sense to have this 8021q upper
> check for each switch port that gets notified of that VLAN addition
> (these include DSA links and CPU ports, we know those can't have 8021q
> uppers because they don't have a net_device registered for them), so
> just do it in slave.c, for that one slave interface.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
>   net/dsa/slave.c  | 33 +++++++++++++++++++++++++++++++++
>   net/dsa/switch.c | 41 -----------------------------------------
>   2 files changed, 33 insertions(+), 41 deletions(-)
> 
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 1940c2458f0f..b88a31a79e2f 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -303,6 +303,28 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
>   	return ret;
>   }
>   
> +/* Must be called under rcu_read_lock() */
> +static int
> +dsa_slave_vlan_check_for_8021q_uppers(struct net_device *slave,
> +				      const struct switchdev_obj_port_vlan *vlan)
> +{
> +	struct net_device *upper_dev;
> +	struct list_head *iter;
> +
> +	netdev_for_each_upper_dev_rcu(slave, upper_dev, iter) {
> +		u16 vid;
> +
> +		if (!is_vlan_dev(upper_dev))
> +			continue;
> +
> +		vid = vlan_dev_vlan_id(upper_dev);
> +		if (vlan->vid_begin <= vid && vlan->vid_end >= vid)
> +			return -EBUSY;

I would find:
		if (vid >= vlan->vid_begin && vid <= vlan->vid_end)

more natural but this works too.

Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
-- 
Florian