| 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: <20200414220855.xudjjpfn5ejyoj5r@soft-dev3.microsemi.net>
Date: Wed, 15 Apr 2020 00:08:55 +0200
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Vladimir Oltean <olteanv@...il.com>
CC: <davem@...emloft.net>, <alexandre.belloni@...tlin.com>,
<antoine.tenart@...tlin.com>, <andrew@...n.ch>,
<f.fainelli@...il.com>, <vivien.didelot@...il.com>,
<joergen.andreasen@...rochip.com>, <allan.nielsen@...rochip.com>,
<claudiu.manoil@....com>, <netdev@...r.kernel.org>,
<UNGLinuxDriver@...rochip.com>, <alexandru.marginean@....com>,
<xiaoliang.yang_1@....com>, <yangbo.lu@....com>, <po.liu@....com>,
<jiri@...lanox.com>, <idosch@...sch.org>, <kuba@...nel.org>
Subject: Re: [PATCH net] net: mscc: ocelot: fix untagged packet drops when
enslaving to vlan aware bridge
The 04/14/2020 22:36, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> From: Vladimir Oltean <vladimir.oltean@....com>
>
> To rehash a previous explanation given in commit 1c44ce560b4d ("net:
> mscc: ocelot: fix vlan_filtering when enslaving to bridge before link is
> up"), the switch driver operates the in a mode where a single VLAN can
> be transmitted as untagged on a particular egress port. That is the
> "native VLAN on trunk port" use case.
>
> The configuration for this native VLAN is driven in 2 ways:
> - Set the egress port rewriter to strip the VLAN tag for the native
> VID (as it is egress-untagged, after all).
> - Configure the ingress port to drop untagged and priority-tagged
> traffic, if there is no native VLAN. The intention of this setting is
> that a trunk port with no native VLAN should not accept untagged
> traffic.
>
> Since both of the above configurations for the native VLAN should only
> be done if VLAN awareness is requested, they are actually done from the
> ocelot_port_vlan_filtering function, after the basic procedure of
> toggling the VLAN awareness flag of the port.
>
> But there's a problem with that simplistic approach: we are trying to
> juggle with 2 independent variables from a single function:
> - Native VLAN of the port - its value is held in port->vid.
> - VLAN awareness state of the port - currently there are some issues
> here, more on that later*.
> The actual problem can be seen when enslaving the switch ports to a VLAN
> filtering bridge:
> 0. The driver configures a pvid of zero for each port, when in
> standalone mode. While the bridge configures a default_pvid of 1 for
> each port that gets added as a slave to it.
> 1. The bridge calls ocelot_port_vlan_filtering with vlan_aware=true.
> The VLAN-filtering-dependent portion of the native VLAN
> configuration is done, considering that the native VLAN is 0.
> 2. The bridge calls ocelot_vlan_add with vid=1, pvid=true,
> untagged=true. The native VLAN changes to 1 (change which gets
> propagated to hardware).
> 3. ??? - nobody calls ocelot_port_vlan_filtering again, to reapply the
> VLAN-filtering-dependent portion of the native VLAN configuration,
> for the new native VLAN of 1. One can notice that after toggling "ip
> link set dev br0 type bridge vlan_filtering 0 && ip link set dev br0
> type bridge vlan_filtering 1", the new native VLAN finally makes it
> through and untagged traffic finally starts flowing again. But
> obviously that shouldn't be needed.
>
> So it is clear that 2 independent variables need to both re-trigger the
> native VLAN configuration. So we introduce the second variable as
> ocelot_port->vlan_aware.
>
> *Actually both the DSA Felix driver and the Ocelot driver already had
> each its own variable:
> - Ocelot: ocelot_port_private->vlan_aware
> - Felix: dsa_port->vlan_filtering
> but the common Ocelot library needs to work with a single, common,
> variable, so there is some refactoring done to move the vlan_aware
> property from the private structure into the common ocelot_port
> structure.
>
> Fixes: 97bb69e1e36e ("net: mscc: ocelot: break apart ocelot_vlan_port_apply")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> To get full VLAN functionality for the Felix DSA driver, we do also need
> someting along the lines of Russell King's patch:
> https://patchwork.ozlabs.org/project/netdev/patch/E1jEB0y-0006iF-5g@rmk-PC.armlinux.org.uk/
> however that is not material for -net.
>
> drivers/net/dsa/ocelot/felix.c | 5 +-
> drivers/net/ethernet/mscc/ocelot.c | 84 +++++++++++++++---------------
> drivers/net/ethernet/mscc/ocelot.h | 2 -
> include/soc/mscc/ocelot.h | 4 +-
> 4 files changed, 47 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index 55bf780b7b0e..53f335d83b37 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -77,11 +77,8 @@ static int felix_fdb_add(struct dsa_switch *ds, int port,
> const unsigned char *addr, u16 vid)
> {
> struct ocelot *ocelot = ds->priv;
> - bool vlan_aware;
>
> - vlan_aware = dsa_port_is_vlan_filtering(dsa_to_port(ds, port));
> -
> - return ocelot_fdb_add(ocelot, port, addr, vid, vlan_aware);
> + return ocelot_fdb_add(ocelot, port, addr, vid);
> }
>
> static int felix_fdb_del(struct dsa_switch *ds, int port,
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index f9e9d205b551..a6de5f1bd9b1 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -183,44 +183,47 @@ static void ocelot_vlan_mode(struct ocelot *ocelot, int port,
> ocelot_write(ocelot, val, ANA_VLANMASK);
> }
>
> -void ocelot_port_vlan_filtering(struct ocelot *ocelot, int port,
> - bool vlan_aware)
> +static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
> + u16 vid)
> {
> struct ocelot_port *ocelot_port = ocelot->ports[port];
> - u32 val;
> + u32 val = 0;
>
> - if (vlan_aware)
> - val = ANA_PORT_VLAN_CFG_VLAN_AWARE_ENA |
> - ANA_PORT_VLAN_CFG_VLAN_POP_CNT(1);
> - else
> - val = 0;
> - ocelot_rmw_gix(ocelot, val,
> - ANA_PORT_VLAN_CFG_VLAN_AWARE_ENA |
> - ANA_PORT_VLAN_CFG_VLAN_POP_CNT_M,
> - ANA_PORT_VLAN_CFG, port);
> + if (ocelot_port->vid != vid) {
> + /* Always permit deleting the native VLAN (vid = 0) */
> + if (ocelot_port->vid && vid) {
> + dev_err(ocelot->dev,
> + "Port already has a native VLAN: %d\n",
> + ocelot_port->vid);
> + return -EBUSY;
> + }
> + ocelot_port->vid = vid;
> + }
> +
> + ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(vid),
> + REW_PORT_VLAN_CFG_PORT_VID_M,
> + REW_PORT_VLAN_CFG, port);
>
> - if (vlan_aware && !ocelot_port->vid)
> + if (ocelot_port->vlan_aware && !ocelot_port->vid)
> /* If port is vlan-aware and tagged, drop untagged and priority
> * tagged frames.
> */
> val = ANA_PORT_DROP_CFG_DROP_UNTAGGED_ENA |
> ANA_PORT_DROP_CFG_DROP_PRIO_S_TAGGED_ENA |
> ANA_PORT_DROP_CFG_DROP_PRIO_C_TAGGED_ENA;
> - else
> - val = 0;
> ocelot_rmw_gix(ocelot, val,
> ANA_PORT_DROP_CFG_DROP_UNTAGGED_ENA |
> ANA_PORT_DROP_CFG_DROP_PRIO_S_TAGGED_ENA |
> ANA_PORT_DROP_CFG_DROP_PRIO_C_TAGGED_ENA,
> ANA_PORT_DROP_CFG, port);
>
> - if (vlan_aware) {
> + if (ocelot_port->vlan_aware) {
> if (ocelot_port->vid)
> /* Tag all frames except when VID == DEFAULT_VLAN */
> - val |= REW_TAG_CFG_TAG_CFG(1);
> + val = REW_TAG_CFG_TAG_CFG(1);
> else
> /* Tag all frames */
> - val |= REW_TAG_CFG_TAG_CFG(3);
> + val = REW_TAG_CFG_TAG_CFG(3);
> } else {
> /* Port tagging disabled. */
> val = REW_TAG_CFG_TAG_CFG(0);
> @@ -228,31 +231,31 @@ void ocelot_port_vlan_filtering(struct ocelot *ocelot, int port,
> ocelot_rmw_gix(ocelot, val,
> REW_TAG_CFG_TAG_CFG_M,
> REW_TAG_CFG, port);
> +
> + return 0;
> }
> -EXPORT_SYMBOL(ocelot_port_vlan_filtering);
>
> -static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
> - u16 vid)
> +void ocelot_port_vlan_filtering(struct ocelot *ocelot, int port,
> + bool vlan_aware)
> {
> struct ocelot_port *ocelot_port = ocelot->ports[port];
> + u32 val;
>
> - if (ocelot_port->vid != vid) {
> - /* Always permit deleting the native VLAN (vid = 0) */
> - if (ocelot_port->vid && vid) {
> - dev_err(ocelot->dev,
> - "Port already has a native VLAN: %d\n",
> - ocelot_port->vid);
> - return -EBUSY;
> - }
> - ocelot_port->vid = vid;
> - }
> + ocelot_port->vlan_aware = vlan_aware;
>
> - ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(vid),
> - REW_PORT_VLAN_CFG_PORT_VID_M,
> - REW_PORT_VLAN_CFG, port);
> + if (vlan_aware)
> + val = ANA_PORT_VLAN_CFG_VLAN_AWARE_ENA |
> + ANA_PORT_VLAN_CFG_VLAN_POP_CNT(1);
> + else
> + val = 0;
> + ocelot_rmw_gix(ocelot, val,
> + ANA_PORT_VLAN_CFG_VLAN_AWARE_ENA |
> + ANA_PORT_VLAN_CFG_VLAN_POP_CNT_M,
> + ANA_PORT_VLAN_CFG, port);
>
> - return 0;
> + ocelot_port_set_native_vlan(ocelot, port, ocelot_port->vid);
> }
> +EXPORT_SYMBOL(ocelot_port_vlan_filtering);
>
> /* Default vlan to clasify for untagged frames (may be zero) */
> static void ocelot_port_set_pvid(struct ocelot *ocelot, int port, u16 pvid)
> @@ -873,12 +876,12 @@ static void ocelot_get_stats64(struct net_device *dev,
> }
>
> int ocelot_fdb_add(struct ocelot *ocelot, int port,
> - const unsigned char *addr, u16 vid, bool vlan_aware)
> + const unsigned char *addr, u16 vid)
> {
> struct ocelot_port *ocelot_port = ocelot->ports[port];
>
> if (!vid) {
> - if (!vlan_aware)
> + if (!ocelot_port->vlan_aware)
> /* If the bridge is not VLAN aware and no VID was
> * provided, set it to pvid to ensure the MAC entry
> * matches incoming untagged packets
> @@ -905,7 +908,7 @@ static int ocelot_port_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
> struct ocelot *ocelot = priv->port.ocelot;
> int port = priv->chip_port;
>
> - return ocelot_fdb_add(ocelot, port, addr, vid, priv->vlan_aware);
> + return ocelot_fdb_add(ocelot, port, addr, vid);
> }
>
> int ocelot_fdb_del(struct ocelot *ocelot, int port,
> @@ -1496,8 +1499,8 @@ static int ocelot_port_attr_set(struct net_device *dev,
> ocelot_port_attr_ageing_set(ocelot, port, attr->u.ageing_time);
> break;
> case SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING:
> - priv->vlan_aware = attr->u.vlan_filtering;
> - ocelot_port_vlan_filtering(ocelot, port, priv->vlan_aware);
> + ocelot_port_vlan_filtering(ocelot, port,
> + attr->u.vlan_filtering);
> break;
> case SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED:
> ocelot_port_attr_mc_set(ocelot, port, !attr->u.mc_disabled);
> @@ -1876,7 +1879,6 @@ static int ocelot_netdevice_port_event(struct net_device *dev,
> } else {
> err = ocelot_port_bridge_leave(ocelot, port,
> info->upper_dev);
> - priv->vlan_aware = false;
> }
> }
> if (netif_is_lag_master(info->upper_dev)) {
> diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
> index e63bc8743187..7a9c748adda0 100644
> --- a/drivers/net/ethernet/mscc/ocelot.h
> +++ b/drivers/net/ethernet/mscc/ocelot.h
> @@ -57,8 +57,6 @@ struct ocelot_port_private {
> struct phy_device *phy;
> u8 chip_port;
>
> - u8 vlan_aware;
> -
> struct phy *serdes;
>
> struct ocelot_port_tc tc;
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 6f122bd6c3c7..25014c1c91b1 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -482,6 +482,8 @@ struct ocelot_port {
>
> void __iomem *regs;
>
> + bool vlan_aware;
> +
> /* Ingress default VLAN (pvid) */
> u16 pvid;
>
> @@ -616,7 +618,7 @@ int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
> int ocelot_fdb_dump(struct ocelot *ocelot, int port,
> dsa_fdb_dump_cb_t *cb, void *data);
> int ocelot_fdb_add(struct ocelot *ocelot, int port,
> - const unsigned char *addr, u16 vid, bool vlan_aware);
> + const unsigned char *addr, u16 vid);
> int ocelot_fdb_del(struct ocelot *ocelot, int port,
> const unsigned char *addr, u16 vid);
> int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
> --
> 2.17.1
>
Reviewed-by: Horatiu Vultur <horatiu.vultur@...rochip.com>
--
/Horatiu
Powered by blists - more mailing lists