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:   Tue,  5 Jul 2022 20:31:14 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     netdev@...r.kernel.org
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        UNGLinuxDriver@...rochip.com, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Petr Machata <petrm@...dia.com>,
        Ido Schimmel <idosch@...dia.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Arun Ramadoss <arun.ramadoss@...rochip.com>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Subject: [RFC PATCH net-next 3/3] net: dsa: never skip VLAN configuration

Stop protecting DSA drivers from switchdev VLAN notifications emitted
while the bridge has vlan_filtering 0, by deleting the deprecated bool
ds->configure_vlan_while_not_filtering opt-in. Now all DSA drivers see
all notifications and should save the bridge PVID until they need to
commit it to hardware.

The 2 remaining unconverted drivers are the gswip and the Microchip KSZ
family. They are both probably broken and would need changing as far as
I can see:

- For lantiq_gswip, after the initial call path
  -> gswip_port_bridge_join
     -> gswip_vlan_add_unaware
        -> gswip_switch_w(priv, 0, GSWIP_PCE_DEFPVID(port));
  nobody seems to prevent a future call path
  -> gswip_port_vlan_add
     -> gswip_vlan_add_aware
        -> gswip_switch_w(priv, idx, GSWIP_PCE_DEFPVID(port));

- For ksz9477, REG_PORT_DEFAULT_VID seems to be only modified by the
  bridge VLAN add/del handlers, so there is no logic to update it on
  VLAN awareness change. I don't know exactly how the switch behaves
  after ksz9477_port_vlan_filtering() is called with "false".
  If packets are classified to the REG_PORT_DEFAULT_VID, then it is
  broken. Similar thing can be said for KSZ8.

In any case, see commit 8b6836d82470 ("net: dsa: mv88e6xxx: keep the
pvid at 0 when VLAN-unaware") for an example of how to deal with the
problem, and test pvid_change() in
tools/testing/selftests/drivers/net/dsa/bridge_vlan_unaware.sh for how
to check whether the driver behaves correctly. I don't have the hardware
to test any changes.

Cc: Arun Ramadoss <arun.ramadoss@...rochip.com>
Cc: Hauke Mehrtens <hauke@...ke-m.de>
Cc: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 drivers/net/dsa/lantiq_gswip.c         |  2 --
 drivers/net/dsa/microchip/ksz_common.c |  2 --
 include/net/dsa.h                      |  7 -------
 net/dsa/dsa2.c                         |  2 --
 net/dsa/dsa_priv.h                     |  1 -
 net/dsa/port.c                         | 14 --------------
 net/dsa/slave.c                        | 22 +---------------------
 7 files changed, 1 insertion(+), 49 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index e531b93f3cb2..86acf8a4e53c 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -893,8 +893,6 @@ static int gswip_setup(struct dsa_switch *ds)
 
 	gswip_port_enable(ds, cpu_port, NULL);
 
-	ds->configure_vlan_while_not_filtering = false;
-
 	return 0;
 }
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 28d7cb2ce98f..561a515c7cb8 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -927,8 +927,6 @@ static int ksz_setup(struct dsa_switch *ds)
 
 	ksz_init_mib_timer(dev);
 
-	ds->configure_vlan_while_not_filtering = false;
-
 	if (dev->dev_ops->setup) {
 		ret = dev->dev_ops->setup(ds);
 		if (ret)
diff --git a/include/net/dsa.h b/include/net/dsa.h
index b902b31bebce..2ed1c2db4037 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -400,13 +400,6 @@ struct dsa_switch {
 	/* Keep VLAN filtering enabled on ports not offloading any upper */
 	u32			needs_standalone_vlan_filtering:1;
 
-	/* Pass .port_vlan_add and .port_vlan_del to drivers even for bridges
-	 * that have vlan_filtering=0. All drivers should ideally set this (and
-	 * then the option would get removed), but it is unknown whether this
-	 * would break things or not.
-	 */
-	u32			configure_vlan_while_not_filtering:1;
-
 	/* If the switch driver always programs the CPU port as egress tagged
 	 * despite the VLAN configuration indicating otherwise, then setting
 	 * @untag_bridge_pvid will force the DSA receive path to pop the
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index cac48a741f27..47a2e60b6080 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -890,8 +890,6 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 	if (err)
 		goto unregister_devlink_ports;
 
-	ds->configure_vlan_while_not_filtering = true;
-
 	err = ds->ops->setup(ds);
 	if (err < 0)
 		goto unregister_notifier;
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index d9722e49864b..63191db45d02 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -224,7 +224,6 @@ void dsa_port_pre_lag_leave(struct dsa_port *dp, struct net_device *lag_dev);
 void dsa_port_lag_leave(struct dsa_port *dp, struct net_device *lag_dev);
 int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 			    struct netlink_ext_ack *extack);
-bool dsa_port_skip_vlan_configuration(struct dsa_port *dp);
 int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock);
 int dsa_port_mst_enable(struct dsa_port *dp, bool on,
 			struct netlink_ext_ack *extack);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 3738f2d40a0b..79853f673b65 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -834,20 +834,6 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 	return err;
 }
 
-/* This enforces legacy behavior for switch drivers which assume they can't
- * receive VLAN configuration when enslaved to a bridge with vlan_filtering=0
- */
-bool dsa_port_skip_vlan_configuration(struct dsa_port *dp)
-{
-	struct net_device *br = dsa_port_bridge_dev_get(dp);
-	struct dsa_switch *ds = dp->ds;
-
-	if (!br)
-		return false;
-
-	return !ds->configure_vlan_while_not_filtering && !br_vlan_enabled(br);
-}
-
 int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock)
 {
 	unsigned long ageing_jiffies = clock_t_to_jiffies(ageing_clock);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index ad6a6663feeb..d1284f8684d8 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -533,11 +533,6 @@ static int dsa_slave_vlan_add(struct net_device *dev,
 	struct switchdev_obj_port_vlan *vlan;
 	int err;
 
-	if (dsa_port_skip_vlan_configuration(dp)) {
-		NL_SET_ERR_MSG_MOD(extack, "skipping configuration of VLAN");
-		return 0;
-	}
-
 	vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
 
 	/* Deny adding a bridge VLAN when there is already an 802.1Q upper with
@@ -571,11 +566,6 @@ static int dsa_slave_host_vlan_add(struct net_device *dev,
 	if (!dp->bridge)
 		return -EOPNOTSUPP;
 
-	if (dsa_port_skip_vlan_configuration(dp)) {
-		NL_SET_ERR_MSG_MOD(extack, "skipping configuration of VLAN");
-		return 0;
-	}
-
 	vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj);
 
 	/* Even though drivers often handle CPU membership in special ways,
@@ -642,9 +632,6 @@ static int dsa_slave_vlan_del(struct net_device *dev,
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	struct switchdev_obj_port_vlan *vlan;
 
-	if (dsa_port_skip_vlan_configuration(dp))
-		return 0;
-
 	vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
 
 	return dsa_port_vlan_del(dp, vlan);
@@ -660,9 +647,6 @@ static int dsa_slave_host_vlan_del(struct net_device *dev,
 	if (!dp->bridge)
 		return -EOPNOTSUPP;
 
-	if (dsa_port_skip_vlan_configuration(dp))
-		return 0;
-
 	vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
 
 	return dsa_port_host_vlan_del(dp, vlan);
@@ -1655,11 +1639,7 @@ static int dsa_slave_clear_vlan(struct net_device *vdev, int vid, void *arg)
  *         - no VLAN (any 8021q upper is a software VLAN)
  *
  * - If under a vlan_filtering=0 bridge which it offload:
- *     - if ds->configure_vlan_while_not_filtering = true (default):
- *         - the bridge VLANs. These VLANs are committed to hardware but inactive.
- *     - else (deprecated):
- *         - no VLAN. The bridge VLANs are not restored when VLAN awareness is
- *           enabled, so this behavior is broken and discouraged.
+ *     - the bridge VLANs. These VLANs are committed to hardware but inactive.
  *
  * - If under a vlan_filtering=1 bridge which it offload:
  *     - the bridge VLANs
-- 
2.25.1

Powered by blists - more mailing lists