[PATCH RFC] net: dsa: Make switches VLAN aware when enslaved into a bridge

[PATCH RFC] net: dsa: Make switches VLAN aware when enslaved into a bridge

[Florian Fainelli]

Commit 2ea7a679ca2a ("net: dsa: Don't add vlans when vlan filtering is
disabled") changed the behavior of DSA switches when the switch ports
are enslaved into the bridge and only pushed the VLAN configuration down
to the switch if the bridge is configured with VLAN filtering enabled.

This is unfortunately wrong, because what vlan_filtering configures is a
policy on the acceptance of VLAN tagged frames with an unknown VID.

vlan_filtering=0 means a frame with a VLAN tag that is not part of the
VLAN table should be allowed to ingress the switch, and vlan_fltering=1
would reject that frame.

Fixes: 2ea7a679ca2a ("net: dsa: Don't add vlans when vlan filtering is disabled")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Andrew,

I checked with Jiri and he confirmed that our interpretention of
vlan_filtering in DSA was incorrect and that it does denote whether the
switch should be doing VID ingress policy checking.

You mentioned in the commit message some problems without being too
specific about them which is why I am putting the same checks in
mv88e6xxx in order not to break your use cases. Let me know if you want
to drop that hunk entirely.

Thanks!

 drivers/net/dsa/mv88e6xxx/chip.c |  7 ++++++-
 net/dsa/port.c                   | 10 ++--------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 8da3d39e3218..df411e776911 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1684,13 +1684,14 @@ static int _mv88e6xxx_port_vlan_add(struct mv88e6xxx_chip *chip, int port,
 static void mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port,
 				    const struct switchdev_obj_port_vlan *vlan)
 {
+	struct dsa_port *dp = &ds->ports[i];
 	struct mv88e6xxx_chip *chip = ds->priv;
 	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
 	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
 	u8 member;
 	u16 vid;
 
-	if (!chip->info->max_vid)
+	if (!chip->info->max_vid || br_vlan_enabled(dp->bridge_dev))
 		return;
 
 	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
@@ -1751,12 +1752,16 @@ static int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port,
 				   const struct switchdev_obj_port_vlan *vlan)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
+	struct dsa_port *dp = &ds->ports[i];
 	u16 pvid, vid;
 	int err = 0;
 
 	if (!chip->info->max_vid)
 		return -EOPNOTSUPP;
 
+	if (br_vlan_enabled(dp->bridge_dev))
+		return 0;
+
 	mutex_lock(&chip->reg_lock);
 
 	err = mv88e6xxx_port_get_pvid(chip, port, &pvid);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index ed0595459df1..111d7cfc8982 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -255,10 +255,7 @@ int dsa_port_vlan_add(struct dsa_port *dp,
 	if (netif_is_bridge_master(vlan->obj.orig_dev))
 		return -EOPNOTSUPP;
 
-	if (br_vlan_enabled(dp->bridge_dev))
-		return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_ADD, &info);
-
-	return 0;
+	return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_ADD, &info);
 }
 
 int dsa_port_vlan_del(struct dsa_port *dp,
@@ -273,10 +270,7 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 	if (netif_is_bridge_master(vlan->obj.orig_dev))
 		return -EOPNOTSUPP;
 
-	if (br_vlan_enabled(dp->bridge_dev))
-		return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
-
-	return 0;
+	return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
 }
 
 static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp)
-- 
2.17.1

Re: [PATCH RFC] net: dsa: Make switches VLAN aware when enslaved into a bridge

[Florian Fainelli]

On 10/24/18 12:36 PM, Florian Fainelli wrote:
> Commit 2ea7a679ca2a ("net: dsa: Don't add vlans when vlan filtering is
> disabled") changed the behavior of DSA switches when the switch ports
> are enslaved into the bridge and only pushed the VLAN configuration down
> to the switch if the bridge is configured with VLAN filtering enabled.
> 
> This is unfortunately wrong, because what vlan_filtering configures is a
> policy on the acceptance of VLAN tagged frames with an unknown VID.
> 
> vlan_filtering=0 means a frame with a VLAN tag that is not part of the
> VLAN table should be allowed to ingress the switch, and vlan_fltering=1
> would reject that frame.
> 
> Fixes: 2ea7a679ca2a ("net: dsa: Don't add vlans when vlan filtering is disabled")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Andrew,
> 
> I checked with Jiri and he confirmed that our interpretention of
> vlan_filtering in DSA was incorrect and that it does denote whether the
> switch should be doing VID ingress policy checking.
> 
> You mentioned in the commit message some problems without being too
> specific about them which is why I am putting the same checks in
> mv88e6xxx in order not to break your use cases. Let me know if you want
> to drop that hunk entirely.
> 
> Thanks!
> 
>  drivers/net/dsa/mv88e6xxx/chip.c |  7 ++++++-
>  net/dsa/port.c                   | 10 ++--------
>  2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 8da3d39e3218..df411e776911 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1684,13 +1684,14 @@ static int _mv88e6xxx_port_vlan_add(struct mv88e6xxx_chip *chip, int port,
>  static void mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port,
>  				    const struct switchdev_obj_port_vlan *vlan)
>  {
> +	struct dsa_port *dp = &ds->ports[i];

This should obviously be port instead of i, but I would still appreciate
a review, thanks!
-- 
Florian

Re: [PATCH RFC] net: dsa: Make switches VLAN aware when enslaved into a bridge

[Ido Schimmel]

On Wed, Oct 24, 2018 at 12:36:57PM -0700, Florian Fainelli wrote:
> Commit 2ea7a679ca2a ("net: dsa: Don't add vlans when vlan filtering is
> disabled") changed the behavior of DSA switches when the switch ports
> are enslaved into the bridge and only pushed the VLAN configuration down
> to the switch if the bridge is configured with VLAN filtering enabled.

This is what mlxsw is doing.

> This is unfortunately wrong, because what vlan_filtering configures is a
> policy on the acceptance of VLAN tagged frames with an unknown VID.
> 
> vlan_filtering=0 means a frame with a VLAN tag that is not part of the
> VLAN table should be allowed to ingress the switch, and vlan_fltering=1
> would reject that frame.

While you correctly describe the logic, this is not how VLAN-unaware
bridges are actually used. The expectation is that packets will be
untagged when entering the bridge. Either because they are truly
untagged or because they were untagged by a VLAN netdev.

For a long time we rejected the enslavement of physical ports to
VLAN-unaware bridges and only allowed VLAN netdevs to be enslaved. In
order to support the logic you described, we would need to map all 4K
VLANs on each port to 4K different FIDs. In addition, each FDB entry
would need to be programmed 4K times, each time with a different FID.
This is because FDB lookup is performed using {MAC, FID} and not only
MAC. I can go into more details about why we cannot map different VLANs
on a port to the same FID, but I do not think it is pertinent to our
discussion.

Eventually, users started complaining about this constraint and we
relaxed it in commit 65b53bfd497b ("mlxsw: spectrum_switchdev: Allow
port enslavement to a VLAN-unaware bridge").

P.S. Corrected Petr's mail address.

Re: [PATCH RFC] net: dsa: Make switches VLAN aware when enslaved into a bridge

[Florian Fainelli]

On 10/26/18 8:10 AM, Ido Schimmel wrote:
> On Wed, Oct 24, 2018 at 12:36:57PM -0700, Florian Fainelli wrote:
>> Commit 2ea7a679ca2a ("net: dsa: Don't add vlans when vlan filtering is
>> disabled") changed the behavior of DSA switches when the switch ports
>> are enslaved into the bridge and only pushed the VLAN configuration down
>> to the switch if the bridge is configured with VLAN filtering enabled.
> 
> This is what mlxsw is doing.
> 
>> This is unfortunately wrong, because what vlan_filtering configures is a
>> policy on the acceptance of VLAN tagged frames with an unknown VID.
>>
>> vlan_filtering=0 means a frame with a VLAN tag that is not part of the
>> VLAN table should be allowed to ingress the switch, and vlan_fltering=1
>> would reject that frame.
> 
> While you correctly describe the logic, this is not how VLAN-unaware
> bridges are actually used. The expectation is that packets will be
> untagged when entering the bridge. Either because they are truly
> untagged or because they were untagged by a VLAN netdev.
> 
> For a long time we rejected the enslavement of physical ports to
> VLAN-unaware bridges and only allowed VLAN netdevs to be enslaved. In
> order to support the logic you described, we would need to map all 4K
> VLANs on each port to 4K different FIDs. In addition, each FDB entry
> would need to be programmed 4K times, each time with a different FID.
> This is because FDB lookup is performed using {MAC, FID} and not only
> MAC. I can go into more details about why we cannot map different VLANs
> on a port to the same FID, but I do not think it is pertinent to our
> discussion.
> 
> Eventually, users started complaining about this constraint and we
> relaxed it in commit 65b53bfd497b ("mlxsw: spectrum_switchdev: Allow
> port enslavement to a VLAN-unaware bridge").

Thanks for providing more context, I suppose we will keep the current
logic then, if nothing else it aligns us with mlxsw.
-- 
Florian