netdev - Re: [RFC net] net: dsa: Centralize validation of VLAN configuration

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] [day] [month] [year] [list]

Date:   Mon, 15 Mar 2021 11:29:43 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Tobias Waldekranz <tobias@...dekranz.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>, davem@...emloft.net,
        kuba@...nel.org, andrew@...n.ch, vivien.didelot@...il.com,
        netdev@...r.kernel.org
Subject: Re: [RFC net] net: dsa: Centralize validation of VLAN configuration

On Sun, Mar 14, 2021 at 10:40:55PM +0100, Tobias Waldekranz wrote:
> On Wed, Mar 10, 2021 at 00:01, Vladimir Oltean <olteanv@...il.com> wrote:
> > +test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port()
> > +{
> > +	ip link add br0 type bridge vlan_filtering 1
> > +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> > +	ip link set ${eth1} master br0
> > +	ip link set ${eth0}.100 master br0
> > +	check_fail $? "Expected to fail but didn't"
> 
> Should it though?
> 
>    br0
>    / \
> .100  \
>   |    \
> eth0   eth1
> 
> eth0 is in standalone mode here. So if the kernel allows it, who are we
> to argue?

Without my "call_switchdev_notifiers(SWITCHDEV_BRPORT_OFFLOADED)" patch,
We have the same old problem with bridging with non-offloaded uppers and
the bridge not knowing they aren't offloaded, don't we? The bridge port
will have a wrong offloading mark.

I think in principle the configuration could be supported with software
bridging, and then the dsa_prevent_bridging_8021q_upper restriction can
be lifted, but I imagine we need to add logic for a DSA port offloading
and unoffloading an existing bridge port depending on its upper configuration.

For example, would we support this configuration?

       br0
       /  \
      /    \      br1
     /  eth1.100 /  \
    /       |   /    \
   /        |  /      \
  eth0     eth1      eth2

eth1 would not be "standalone" except from the perspective of br0, but
due to offloading br1, we would need to turn on address learning and
such. So we should probably either enforce that eth1 is standalone when
at least one non-LAG upper is bridged, or deny bridging the non-LAG
uppers. Without a known use case for such configurations, I would rather
deny them for the time being.

> > +	ip link del br0
> > +
> > +	log_test "VLAN upper joins VLAN-aware bridge which contains another physical port"
> > +}
> > +
> > +test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port_initially_unaware()
> > +{
> > +	ip link add br0 type bridge vlan_filtering 0
> > +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> > +	ip link set ${eth1} master br0
> > +	ip link set ${eth0}.100 master br0
> > +	ip link set br0 type bridge vlan_filtering 1
> > +	check_fail $? "Expected to fail but didn't"
> 
> Same thing here.
> 
> > +	ip link del ${eth0}.100 > /dev/null 2>&1 || :
> > +	ip link del br0
> > +
> > +	log_test "VLAN upper joins VLAN-aware bridge which contains another physical port, but bridge is initially unaware"
> > +}
> > +
> > +test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge()
> > +{
> > +	ip link add br0 type bridge vlan_filtering 1
> > +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> > +	ip link set ${eth0}.100 master br0
> > +	ip link set ${eth1} master br0
> > +	check_fail $? "Expected to fail but didn't"
> 
> And here.
> 
> > +	ip link del ${eth0}.100 > /dev/null 2>&1 || :
> > +	ip link del br0
> > +
> > +	log_test "Bridge join when VLAN upper of another port is already in VLAN-aware bridge"
> > +}
> > +
> > +test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge_initially_unaware()
> > +{
> > +	ip link add br0 type bridge vlan_filtering 0
> > +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> > +	ip link set ${eth0}.100 master br0
> > +	ip link set ${eth0} master br0
> 
> I think you meant for this to be eth1, correct?

Yes, this is a copy-paste mistake.