Re: VLAN filtering with DSA

From: Florian Fainelli <f.fainelli@gmail.com>
To: Vladimir Oltean <olteanv@gmail.com>, Ido Schimmel <idosch@nvidia.com>
Cc: netdev <netdev@vger.kernel.org>, Andrew Lunn <andrew@lunn.ch>,
	Vivien Didelot <vivien.didelot@gmail.com>
Subject: Re: VLAN filtering with DSA
Date: Fri, 11 Sep 2020 09:57:30 -0700	[thread overview]
Message-ID: <64168d1e-1f37-c2d6-fd67-19fc9071fc48@gmail.com> (raw)
In-Reply-To: <20200911163042.u5xegcsfpwzh6zkf@skbuf>

On 9/11/2020 9:30 AM, Vladimir Oltean wrote:
> 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@skbuf>
> 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.

Yes, that was/is definitively the assumption when that code was added, 
and as you indicate below, as of today, only 802.1Q upppers call that NDO.

> 
> 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.

If we started doing that, it may make sense to refactor the existing 
mlxsw code into something more generic that can be used almost as-is by 
switchdev driver as well as DSA, which ... are switchdev drivers to some 
extent as well.
-- 
Florian