netdev - Re: [PATCH net-next 0/3] VLANs, DSA switches and multiple bridges

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: <20200219130707.GB245247@t480s.localdomain>
Date:   Wed, 19 Feb 2020 13:07:07 -0500
From:   Vivien Didelot <vivien.didelot@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Ido Schimmel <idosch@...sch.org>,
        "David S. Miller" <davem@...emloft.net>,
        Ivan Vecera <ivecera@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 0/3] VLANs, DSA switches and multiple bridges

Hi Russell,

On Wed, 19 Feb 2020 09:19:00 +0000, Russell King - ARM Linux admin <linux@...linux.org.uk> wrote:
> On Wed, Feb 19, 2020 at 04:47:30AM +0100, Andrew Lunn wrote:
> > On Wed, Feb 19, 2020 at 12:17:37AM +0000, Russell King - ARM Linux admin wrote:
> > > On Tue, Feb 18, 2020 at 04:00:08PM -0800, Florian Fainelli wrote:
> > > > On 2/18/20 3:45 AM, Russell King - ARM Linux admin wrote:
> > > > > Hi,
> > > > > 
> > > > > This is a repost of the previously posted RFC back in December, which
> > > > > did not get fully reviewed.  I've dropped the RFC tag this time as no
> > > > > one really found anything too problematical in the RFC posting.
> > > > > 
> > > > > I've been trying to configure DSA for VLANs and not having much success.
> > > > > The setup is quite simple:
> > > > > 
> > > > > - The main network is untagged
> > > > > - The wifi network is a vlan tagged with id $VN running over the main
> > > > >   network.
> > > > > 
> > > > > I have an Armada 388 Clearfog with a PCIe wifi card which I'm trying to
> > > > > setup to provide wifi access to the vlan $VN network, while the switch
> > > > > is also part of the main network.
> > > > 
> > > > Why not just revert 2ea7a679ca2abd251c1ec03f20508619707e1749 ("net: dsa:
> > > > Don't add vlans when vlan filtering is disabled")? If a driver wants to
> > > > veto the programming of VLANs while it has ports enslaved to a bridge
> > > > that does not have VLAN filtering, it should have enough information to
> > > > not do that operation.
> > > 
> > > I do not have the knowledge to know whether reverting that commit
> > > would be appropriate; I do not know how the non-Marvell switches will
> > > behave with such a revert - what was the reason for the commit in
> > > the first place?
> > > 
> > > The commit says:
> > > 
> > >     This fixes at least one corner case. There are still issues in other
> > >     corners, such as when vlan_filtering is later enabled.
> > > 
> > > but it doesn't say what that corner case was.  So, presumably reverting
> > > it will cause a regression of whatever that corner case was...
> > 
> > Yes, sorry, bad commit message. I'm not too sure, but it could of been
> > that the switch was adding the VLANs to its tables, even though it
> > should not because filtering is disabled. And i also think the default
> > VLAN was not defined at that point, it only gets defined when
> > vlan_filtering is enabled?
> 
> It's been too long since I researched all these details, but I seem
> to remember that in the Linux software bridge, vlan 1 is always
> present even when vlan filtering is not enabled.
> 
> Looking at br_vlan_init():
> 
>         br->default_pvid = 1;
> 
> and nbp_vlan_init() propagates that irrespective of the bridge vlan
> enable state to switchdev.  nbp_vlan_init() is called whenever any
> interface is added to a bridge (in br_add_if()).
> 
> As I believe I mentioned somewhere in the commit messages or covering
> message, for at least some of the Marvell DSA switches, it is safe to
> add VTU entries - they do not even look at the VTU when the port has
> 802.1Q disabled.  Whether that is true for all Marvell's DSA switches
> I don't know without trawling every functional spec, and I was hoping
> that you guys would know.  I guess I need to trawl the specs.

Some switches like the Marvell 88E6060 don't have a VTU, so programming the
default PVID would return -EOPNOTSUPP. Switches supporting only global VLAN
filtering cannot have a VLAN filtering aware bridge as well as a non VLAN
filtering aware bridge spanning their ports at the same time. But all this
shouldn't be a problem because drivers inform the stack whether they support
ds->vlan_filtering per-port, globally or not. We should simply reject the
operation when vlan_filtering is being enabled on unsupported hardware.

Linux bridge is the reference for the implementation of an Ethernet bridge,
if it programs VLAN entries, supported DSA hardware must do so. I'm not a
fan of having our own bridge logic in DSA, so the limitation implemented by
2ea7a679ca2a ("net: dsa: Don't add vlans when vlan filtering is disabled")
needs to go in my opinion.


Thanks,

	Vivien