[PATCH net-next 0/2] net: dsa: VLAN devices w/ filtering

[PATCH net-next 0/2] net: dsa: VLAN devices w/ filtering

[Florian Fainelli]

Hi all,

This patch series supports having VLAN devices on top of DSA/switch
ports while the switch has VLAN filtering globally turned on (as is the
case with Broadcom switches). Whether the switch does global or per-port
VLAN filtering, having VLAN entries for these VLAN devices is
beneficial.

We take care of a few possibly problematic cases:

- adding a VLAN device while there is an existing VLAN entry created by
  a VLAN aware bridge. The entire bridge's VLAN database and not just
  the specific bridge port is being checked to be safe and conserative

- adding a bridge VLAN entry when there is an existing VLAN device
  created is also not possible because that would lead to the bridge
  being able to manipulate the VLAN device's VID/attributes under its feet

- enslaving a VLAN device into a VLAN aware bridge since that duplicates
  functionality already offered by the VLAN aware bridge

Here are the different test cases that were run to exercise this:

# Create a br0 device with gphy enslaved, verify we can still obtain
# a DHCP lease
ip addr flush dev gphy
ip link add dev br0 type bridge
echo 1 > /sys/class/net/br0/bridge/vlan_filtering
ip link set dev gphy master br0
udhcpc -i br0

# Create a VID 100 interface on top of rgmii_1, verify
# we can ping 192.168.100.1 (the peer)
vconfig add rgmii_1 100
ifconfig rgmii_1.100 192.168.100.10
ping -c 2 192.168.100.1

# Create a VID 42 interface on top of br0 and let it flow tagged
# through the bridge, verify we can ping 192.168.42.1 (the peer)
vconfig add br0 42
bridge vlan add vid 42 dev gphy
bridge vlan add vid 42 dev br0 self
ifconfig br0.42 192.168.42.2
ping -c 2 192.168.42.1

# Delete and re-create rgmii_1.100 and verify things still work
# with or without VLAN filtering applied:
ip link del rgmii_1.100
vconfig add rgmii_1 100
ifconfig rgmii_1.100 192.168.100.10
ping -c 2 192.168.100.1
echo 0 > /sys/class/net/br0/bridge/vlan_filtering
ping -c 2 192.168.100.1

# Delete and attempt to create collision scenarios
ip link del rgmii_1.100
echo 1 > /sys/class/net/br0/bridge/vlan_filtering

# VLAN ID 100 is already claimed by rgmii_1.100
vconfig add rgmii_1 100
brctl addif br0 rgmii_1
# Adding VLAN 100 to rgmii_1 fails since rgmii_1.100 exists
bridge vlan add vid 100 dev rgmii_1

vconfig rem rgmii_1.100
# Adding VLAN 100 to rgmii_1 works since rgmii_1.100 does not exist
bridge vlan add vid 100 dev rgmii_1
# But this fails since we already have a VID with the bridge
vconfig add rgmii_1 100

# Delete and re-create the interface and try to make it enslaved
bridge vlan del vid 100 dev rgmii_1
vconfig add rgmii_1 100
# This fails since the bridge is VLAN aware
brctl addif br0 rgmii_1.100

Florian Fainelli (2):
  net: dsa: Deny enslaving VLAN devices into VLAN aware bridge
  net: dsa: Add ndo_vlan_rx_{add,kill}_vid implementation

 net/dsa/port.c   |  12 ++++--
 net/dsa/slave.c  | 110 +++++++++++++++++++++++++++++++++++++++++++++--
 net/dsa/switch.c |  42 ++++++++++++++++++
 3 files changed, 157 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH net-next 1/2] net: dsa: Deny enslaving VLAN devices into VLAN aware bridge

[Florian Fainelli]

VLAN devices on top of a DSA network device which is already part of a
bridge and with said bridge being VLAN aware should not be allowed to be
enslaved into that bridge. For one, this duplicates functionality
offered by the VLAN aware bridge which supports tagged and untagged VLAN
frames processing and it would make things needlessly complex to e.g.:
propagate FDB/MDB accordingly.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/slave.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 2e5e7c04821b..97519ffa445e 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1416,16 +1416,49 @@ static int dsa_slave_changeupper(struct net_device *dev,
 	return err;
 }
 
+static int dsa_slave_upper_vlan_check(struct net_device *dev,
+				      struct netdev_notifier_changeupper_info *
+				      info)
+{
+	struct netlink_ext_ack *ext_ack;
+	struct net_device *slave;
+	struct dsa_port *dp;
+
+	ext_ack = netdev_notifier_info_to_extack(&info->info);
+
+	if (!is_vlan_dev(dev))
+		return NOTIFY_DONE;
+
+	slave = vlan_dev_real_dev(dev);
+	if (!dsa_slave_dev_check(slave))
+		return NOTIFY_DONE;
+
+	dp = dsa_slave_to_port(slave);
+	if (!dp->bridge_dev)
+		return NOTIFY_DONE;
+
+	/* Deny enslaving a VLAN device into a VLAN-aware bridge */
+	if (br_vlan_enabled(dp->bridge_dev) &&
+	    netif_is_bridge_master(info->upper_dev) && info->linking) {
+		NL_SET_ERR_MSG_MOD(ext_ack,
+				   "Cannot enslave VLAN device into VLAN aware bridge");
+		return notifier_from_errno(-EINVAL);
+	}
+
+	return NOTIFY_DONE;
+}
+
 static int dsa_slave_netdevice_event(struct notifier_block *nb,
 				     unsigned long event, void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
-	if (!dsa_slave_dev_check(dev))
-		return NOTIFY_DONE;
+	if (event == NETDEV_CHANGEUPPER) {
+		if (!dsa_slave_dev_check(dev))
+			return dsa_slave_upper_vlan_check(dev, ptr);
 
-	if (event == NETDEV_CHANGEUPPER)
 		return dsa_slave_changeupper(dev, ptr);
+	}
 
 	return NOTIFY_DONE;
 }
-- 
2.17.1

[PATCH net-next 2/2] net: dsa: Add ndo_vlan_rx_{add,kill}_vid implementation

[Florian Fainelli]

In order to properly support VLAN filtering being enabled/disabled on a
bridge, while having other ports being non bridge port members, we need
to support the ndo_vlan_rx_{add,kill}_vid callbacks in order to make
sure the non-bridge ports can continue receiving VLAN tags, even when
the switch is globally configured to do ingress/egress VID checking.

Since we can call dsa_port_vlan_{add,del} with a bridge_dev pointer
NULL, we now need to check that in these two functions.

We specifically deal with two possibly problematic cases:

- creating a bridge VLAN entry while there is an existing VLAN device
  claiming that same VID

- creating a VLAN device while there is an existing bridge VLAN entry
  with that VID

Those are both resolved with returning -EBUSY back to user-space.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/port.c   | 12 ++++++--
 net/dsa/slave.c  | 71 +++++++++++++++++++++++++++++++++++++++++++++++-
 net/dsa/switch.c | 42 ++++++++++++++++++++++++++++
 3 files changed, 121 insertions(+), 4 deletions(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 2d7e01b23572..e3ac9be7a9a0 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -252,7 +252,10 @@ int dsa_port_vlan_add(struct dsa_port *dp,
 		.vlan = vlan,
 	};
 
-	if (br_vlan_enabled(dp->bridge_dev))
+	/* Can be called from dsa_slave_port_obj_add() or
+	 * dsa_slave_vlan_rx_add_vid()
+	 */
+	if (!dp->bridge_dev || br_vlan_enabled(dp->bridge_dev))
 		return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_ADD, &info);
 
 	return 0;
@@ -267,10 +270,13 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 		.vlan = vlan,
 	};
 
-	if (netif_is_bridge_master(vlan->obj.orig_dev))
+	if (vlan->obj.orig_dev && netif_is_bridge_master(vlan->obj.orig_dev))
 		return -EOPNOTSUPP;
 
-	if (br_vlan_enabled(dp->bridge_dev))
+	/* Can be called from dsa_slave_port_obj_del() or
+	 * dsa_slave_vlan_rx_kill_vid()
+	 */
+	if (!dp->bridge_dev || br_vlan_enabled(dp->bridge_dev))
 		return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
 
 	return 0;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 97519ffa445e..e54f3a7a3457 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -990,6 +990,72 @@ static int dsa_slave_get_ts_info(struct net_device *dev,
 	return ds->ops->get_ts_info(ds, p->dp->index, ts);
 }
 
+static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
+				     u16 vid)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct switchdev_obj_port_vlan vlan = {
+		.vid_begin = vid,
+		.vid_end = vid,
+		/* This API only allows programming tagged, non-PVID VIDs */
+		.flags = 0,
+	};
+	struct bridge_vlan_info info;
+	int ret;
+
+	/* 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;
+	}
+
+	ret = dsa_port_vlan_add(dp, &vlan, NULL);
+	if (ret == -EOPNOTSUPP)
+		ret = 0;
+
+	return ret;
+}
+
+static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
+				      u16 vid)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct switchdev_obj_port_vlan vlan = {
+		.vid_begin = vid,
+		.vid_end = vid,
+		/* This API only allows programming tagged, non-PVID VIDs */
+		.flags = 0,
+	};
+	struct bridge_vlan_info info;
+	int ret;
+
+	/* 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;
+	}
+
+	ret = dsa_port_vlan_del(dp, &vlan);
+	if (ret == -EOPNOTSUPP)
+		ret = 0;
+
+	return ret;
+}
+
 static const struct ethtool_ops dsa_slave_ethtool_ops = {
 	.get_drvinfo		= dsa_slave_get_drvinfo,
 	.get_regs_len		= dsa_slave_get_regs_len,
@@ -1055,6 +1121,8 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
 	.ndo_setup_tc		= dsa_slave_setup_tc,
 	.ndo_get_stats64	= dsa_slave_get_stats64,
 	.ndo_get_port_parent_id	= dsa_slave_get_port_parent_id,
+	.ndo_vlan_rx_add_vid	= dsa_slave_vlan_rx_add_vid,
+	.ndo_vlan_rx_kill_vid	= dsa_slave_vlan_rx_kill_vid,
 };
 
 static const struct switchdev_ops dsa_slave_switchdev_ops = {
@@ -1315,7 +1383,8 @@ int dsa_slave_create(struct dsa_port *port)
 	if (slave_dev == NULL)
 		return -ENOMEM;
 
-	slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
+	slave_dev->features = master->vlan_features | NETIF_F_HW_TC |
+				NETIF_F_HW_VLAN_CTAG_FILTER;
 	slave_dev->hw_features |= NETIF_F_HW_TC;
 	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
 	eth_hw_addr_inherit(slave_dev, master);
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 142b294d3446..e1fae969aa73 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -12,6 +12,7 @@
 
 #include <linux/netdevice.h>
 #include <linux/notifier.h>
+#include <linux/if_vlan.h>
 #include <net/switchdev.h>
 
 #include "dsa_priv.h"
@@ -168,6 +169,43 @@ static int dsa_switch_mdb_del(struct dsa_switch *ds,
 	return 0;
 }
 
+static int dsa_port_vlan_device_check(struct net_device *vlan_dev,
+				      int vlan_dev_vid,
+				      void *arg)
+{
+	struct switchdev_obj_port_vlan *vlan = arg;
+	u16 vid;
+
+	for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid) {
+		if (vid == vlan_dev_vid)
+			return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int dsa_port_vlan_check(struct dsa_switch *ds, int port,
+			       const struct switchdev_obj_port_vlan *vlan)
+{
+	const struct dsa_port *dp = dsa_to_port(ds, port);
+	int err = 0;
+
+	/* Device is not bridged, let it proceed with the VLAN device
+	 * creation.
+	 */
+	if (!dp->bridge_dev)
+		return err;
+
+	/* dsa_slave_vlan_rx_{add,kill}_vid() cannot use the prepare pharse and
+	 * already checks whether there is an overlapping bridge VLAN entry
+	 * with the same VID, so here we only need to check that if we are
+	 * adding a bridge VLAN entry there is not an overlapping VLAN device
+	 * claiming that VID.
+	 */
+	return vlan_for_each(dp->slave, dsa_port_vlan_device_check,
+			     (void *)vlan);
+}
+
 static int
 dsa_switch_vlan_prepare_bitmap(struct dsa_switch *ds,
 			       const struct switchdev_obj_port_vlan *vlan,
@@ -179,6 +217,10 @@ dsa_switch_vlan_prepare_bitmap(struct dsa_switch *ds,
 		return -EOPNOTSUPP;
 
 	for_each_set_bit(port, bitmap, ds->num_ports) {
+		err = dsa_port_vlan_check(ds, port, vlan);
+		if (err)
+			return err;
+
 		err = ds->ops->port_vlan_prepare(ds, port, vlan);
 		if (err)
 			return err;
-- 
2.17.1

Re: [PATCH net-next 0/2] net: dsa: VLAN devices w/ filtering

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 20 Feb 2019 14:35:37 -0800

> This patch series supports having VLAN devices on top of DSA/switch
> ports while the switch has VLAN filtering globally turned on (as is the
> case with Broadcom switches). Whether the switch does global or per-port
> VLAN filtering, having VLAN entries for these VLAN devices is
> beneficial.
> 
> We take care of a few possibly problematic cases:
> 
> - adding a VLAN device while there is an existing VLAN entry created by
>   a VLAN aware bridge. The entire bridge's VLAN database and not just
>   the specific bridge port is being checked to be safe and conserative
> 
> - adding a bridge VLAN entry when there is an existing VLAN device
>   created is also not possible because that would lead to the bridge
>   being able to manipulate the VLAN device's VID/attributes under its feet
> 
> - enslaving a VLAN device into a VLAN aware bridge since that duplicates
>   functionality already offered by the VLAN aware bridge
> 
> Here are the different test cases that were run to exercise this:
 ...

Series applied, thanks Florian.