| 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
| ||
|
Message-ID: <e681971641d53b830a7aeafb9e88a3ac059915c5.camel@microchip.com>
Date: Wed, 6 Jul 2022 10:51:34 +0000
From: <Arun.Ramadoss@...rochip.com>
To: <netdev@...r.kernel.org>, <vladimir.oltean@....com>
CC: <claudiu.manoil@....com>, <alexandre.belloni@...tlin.com>,
<UNGLinuxDriver@...rochip.com>, <andrew@...n.ch>,
<vivien.didelot@...il.com>, <petrm@...dia.com>,
<idosch@...dia.com>, <linux@...pel-privat.de>,
<f.fainelli@...il.com>, <hauke@...ke-m.de>,
<xiaoliang.yang_1@....com>, <kuba@...nel.org>, <pabeni@...hat.com>,
<edumazet@...gle.com>, <martin.blumenstingl@...glemail.com>,
<Woojung.Huh@...rochip.com>, <davem@...emloft.net>
Subject: Re: [RFC PATCH net-next 3/3] net: dsa: never skip VLAN configuration
Hi Vladimir,
On Tue, 2022-07-05 at 20:31 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> 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.
When ksz9477_port_vlan_filtering is set to false, then it clears the
802.1Q vlan enable bit in the switch. So the packets are not classified
based on vlan tag. Only if the vlan bit is set, packets are classified
based on pvid.
>
> 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.
I can test it and report the observation. But I haven't run the
selftests before. I looked in the scripts today, but couldn't find out
how to compile it as part of kernel and program it to the target &
test. I infer that, this scripts should be run on target (I have
SAMA5D3 + KSZ9477) with 4 switch ports. Can you guide me on testing
this.
>
> 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