netdev - Re: VLAN filtering with DSA

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:   Fri, 11 Sep 2020 19:30:42 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Ido Schimmel <idosch@...dia.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>
Subject: Re: VLAN filtering with DSA

On Fri, Sep 11, 2020 at 04:20:58PM +0300, Ido Schimmel wrote:
> On Thu, Sep 10, 2020 at 11:41:04AM -0700, Florian Fainelli wrote:
> > +Ido,
> >
> > On 9/10/2020 8:07 AM, Vladimir Oltean wrote:
> > > Florian, can you please reiterate what is the problem with calling
> > > vlan_vid_add() with a VLAN that is installed by the bridge?
> > >
> > > The effect of vlan_vid_add(), to my knowledge, is that the network
> > > interface should add this VLAN to its filtering table, and not drop it.
> > > So why return -EBUSY?
>
> Can you clarify when you return -EBUSY? At least in mlxsw we return an
> error in case we have a VLAN-aware bridge taking care of some VLAN and
> then user space tries to install a VLAN upper with the same VLAN on the
> same port. See more below.
>

In the original post Message-ID: <20200910150738.mwhh2i6j2qgacqev@...uf>
I had copied this piece of code:

static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
				     u16 vid)
{
	...

	/* Check for a possible bridge VLAN entry now since there is no
	 * need to emulate the switchdev prepare + commit phase.
	 */
	if (dp->bridge_dev) {
		...
		/* br_vlan_get_info() returns -EINVAL or -ENOENT if the
		 * device, respectively the VID is not found, returning
		 * 0 means success, which is a failure for us here.
		 */
		ret = br_vlan_get_info(dp->bridge_dev, vid, &info);
		if (ret == 0)
			return -EBUSY;
	}
}

> > Most of this was based on discussions we had with Ido and him explaining to
> > me how they were doing it in mlxsw.
> >
> > AFAIR the other case which is that you already have a 802.1Q upper, and then
> > you add the switch port to the bridge is permitted and the bridge would
> > inherit the VLAN into its local database.
>
> If you have swp1 and swp1.10, you can put swp1 in a VLAN-aware bridge
> and swp1.10 in a VLAN-unaware bridge. If you add VLAN 10 as part of the
> VLAN-aware bridge on swp1, traffic tagged with this VLAN will still be
> injected into the stack via swp1.10.
>
> I'm not sure what is the use case for such a configuration and we reject
> it in mlxsw.

Maybe the problem has to do with the fact that Florian took the
.ndo_vlan_rx_add_vid() callback as a shortcut for deducing that there is
an 8021q upper interface.

Currently there are other places in the network stack that don't really
work with a network interface that has problems with an interface that
has "rx-vlan-filter: on" in ethtool -k. See this discussion with Jiri on
the use of tc-vlan:

https://www.spinics.net/lists/netdev/msg645931.html

So, even though today .ndo_vlan_rx_add_vid() is only called from 8021q,
maybe we should dispel the myth that it's specific to 8021q somehow.

Maybe DSA should start tracking its upper interfaces, after all? Not
convinced though.

Thanks,
-Vladimir