[PATCH net-next 0/3] NXP SJA1105 VLAN regressions

[PATCH net-next 0/3] NXP SJA1105 VLAN regressions

[Vladimir Oltean]

These are 3 patches to fix issues seen with some more varied testing
done after the changes in the "Traffic termination for sja1105 ports
under VLAN-aware bridge" series were made:
https://patchwork.kernel.org/project/netdevbpf/cover/20210726165536.1338471-1-vladimir.oltean@nxp.com/

Issue 1: traffic no longer works on a port after leaving a VLAN-aware bridge
Issue 2: untagged traffic not dropped if pvid is absent from a VLAN-aware port
Issue 3: PTP and STP broken on ports under a VLAN-aware bridge

Vladimir Oltean (3):
  net: dsa: sja1105: reset the port pvid when leaving a VLAN-aware
    bridge
  net: dsa: sja1105: make sure untagged packets are dropped on ingress
    ports with no pvid
  net: dsa: tag_sja1105: fix control packets on SJA1110 being received
    on an imprecise port

 drivers/net/dsa/sja1105/sja1105_main.c | 134 ++++++++++++++++---------
 net/dsa/tag_sja1105.c                  |  27 ++---
 2 files changed, 98 insertions(+), 63 deletions(-)

-- 
2.25.1

[PATCH net-next 1/3] net: dsa: sja1105: reset the port pvid when leaving a VLAN-aware bridge

[Vladimir Oltean]

Now that we no longer have the ultra-central sja1105_build_vlan_table(),
we need to be more careful about checking all corner cases manually.

For example, when a port leaves a VLAN-aware bridge, it becomes
standalone so its pvid should become a tag_8021q RX VLAN again. However,
sja1105_commit_pvid() only gets called from sja1105_bridge_vlan_add()
and from sja1105_vlan_filtering(), and no VLAN awareness change takes
place (VLAN filtering is a global setting for sja1105, so the switch
remains VLAN-aware overall).

This means that we need to put another sja1105_commit_pvid() call in
sja1105_bridge_member().

Fixes: 6dfd23d35e75 ("net: dsa: sja1105: delete vlan delta save/restore logic")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 62 ++++++++++++++------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 3047704c24d3..293c77622657 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -57,6 +57,35 @@ static bool sja1105_can_forward(struct sja1105_l2_forwarding_entry *l2_fwd,
 	return !!(l2_fwd[from].reach_port & BIT(to));
 }
 
+static int sja1105_pvid_apply(struct sja1105_private *priv, int port, u16 pvid)
+{
+	struct sja1105_mac_config_entry *mac;
+
+	mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
+
+	if (mac[port].vlanid == pvid)
+		return 0;
+
+	mac[port].vlanid = pvid;
+
+	return sja1105_dynamic_config_write(priv, BLK_IDX_MAC_CONFIG, port,
+					    &mac[port], true);
+}
+
+static int sja1105_commit_pvid(struct dsa_switch *ds, int port)
+{
+	struct dsa_port *dp = dsa_to_port(ds, port);
+	struct sja1105_private *priv = ds->priv;
+	u16 pvid;
+
+	if (dp->bridge_dev && br_vlan_enabled(dp->bridge_dev))
+		pvid = priv->bridge_pvid[port];
+	else
+		pvid = priv->tag_8021q_pvid[port];
+
+	return sja1105_pvid_apply(priv, port, pvid);
+}
+
 static int sja1105_init_mac_settings(struct sja1105_private *priv)
 {
 	struct sja1105_mac_config_entry default_mac = {
@@ -1656,6 +1685,10 @@ static int sja1105_bridge_member(struct dsa_switch *ds, int port,
 	if (rc)
 		return rc;
 
+	rc = sja1105_commit_pvid(ds, port);
+	if (rc)
+		return rc;
+
 	return sja1105_manage_flood_domains(priv);
 }
 
@@ -1955,35 +1988,6 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 	return rc;
 }
 
-static int sja1105_pvid_apply(struct sja1105_private *priv, int port, u16 pvid)
-{
-	struct sja1105_mac_config_entry *mac;
-
-	mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
-
-	if (mac[port].vlanid == pvid)
-		return 0;
-
-	mac[port].vlanid = pvid;
-
-	return sja1105_dynamic_config_write(priv, BLK_IDX_MAC_CONFIG, port,
-					   &mac[port], true);
-}
-
-static int sja1105_commit_pvid(struct dsa_switch *ds, int port)
-{
-	struct dsa_port *dp = dsa_to_port(ds, port);
-	struct sja1105_private *priv = ds->priv;
-	u16 pvid;
-
-	if (dp->bridge_dev && br_vlan_enabled(dp->bridge_dev))
-		pvid = priv->bridge_pvid[port];
-	else
-		pvid = priv->tag_8021q_pvid[port];
-
-	return sja1105_pvid_apply(priv, port, pvid);
-}
-
 static enum dsa_tag_protocol
 sja1105_get_tag_protocol(struct dsa_switch *ds, int port,
 			 enum dsa_tag_protocol mp)
-- 
2.25.1

[PATCH net-next 2/3] net: dsa: sja1105: make sure untagged packets are dropped on ingress ports with no pvid

[Vladimir Oltean]

Surprisingly, this configuration:

ip link add br0 type bridge vlan_filtering 1
ip link set swp2 master br0
bridge vlan del dev swp2 vid 1

still has the sja1105 switch sending untagged packets to the CPU (and
failing to decode them, since dsa_find_designated_bridge_port_by_vid
searches by VID 1 and rightfully finds no bridge VLAN 1 on a port).

Dumping the switch configuration, the VLANs are managed properly:
- the pvid of swp2 is 1 in the MAC Configuration Table, but
- only the CPU port is in the port membership of VLANID 1 in the VLAN
  Lookup Table

When the ingress packets are tagged with VID 1, they are properly
dropped. But when they are untagged, they are able to reach the CPU
port. Also, when the pvid in the MAC Configuration Table is changed to
e.g. 55 (an unused VLAN), the untagged packets are also dropped.

So it looks like:
- the switch bypasses ingress VLAN membership checks for untagged traffic
- the reason why the untagged traffic is dropped when I make the pvid 55
  is due to the lack of valid destination ports in VLAN 55, rather than
  an ingress membership violation
- the ingress VLAN membership cheks are only done for VLAN-tagged traffic

Interesting. It looks like there is an explicit bit to drop untagged
traffic, so we should probably be using that to preserve user expectations.

Note that only VLAN-aware ports should drop untagged packets due to no
pvid - when VLAN-unaware, the software bridge doesn't do this even if
there is no pvid on any bridge port and on the bridge itself. So the new
sja1105_drop_untagged() function cannot simply be called with "false"
from sja1105_bridge_vlan_add() and with "true" from sja1105_bridge_vlan_del.
Instead, we need to also consider the VLAN awareness state. That means
we need to hook the "drop untagged" setting in all the same places where
the "commit pvid" logic is, and it needs to factor in all the state when
flipping the "drop untagged" bit: is our current pvid in the VLAN Lookup
Table, and is the current port in that VLAN's port membership list?
VLAN-unaware ports will never drop untagged frames because these checks
always succeed by construction, and the tag_8021q VLANs cannot be changed
by the user.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 74 +++++++++++++++++++-------
 1 file changed, 56 insertions(+), 18 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 293c77622657..5ab1676a7448 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -57,6 +57,38 @@ static bool sja1105_can_forward(struct sja1105_l2_forwarding_entry *l2_fwd,
 	return !!(l2_fwd[from].reach_port & BIT(to));
 }
 
+static int sja1105_is_vlan_configured(struct sja1105_private *priv, u16 vid)
+{
+	struct sja1105_vlan_lookup_entry *vlan;
+	int count, i;
+
+	vlan = priv->static_config.tables[BLK_IDX_VLAN_LOOKUP].entries;
+	count = priv->static_config.tables[BLK_IDX_VLAN_LOOKUP].entry_count;
+
+	for (i = 0; i < count; i++)
+		if (vlan[i].vlanid == vid)
+			return i;
+
+	/* Return an invalid entry index if not found */
+	return -1;
+}
+
+static int sja1105_drop_untagged(struct dsa_switch *ds, int port, bool drop)
+{
+	struct sja1105_private *priv = ds->priv;
+	struct sja1105_mac_config_entry *mac;
+
+	mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
+
+	if (mac[port].drpuntag == drop)
+		return 0;
+
+	mac[port].drpuntag = drop;
+
+	return sja1105_dynamic_config_write(priv, BLK_IDX_MAC_CONFIG, port,
+					    &mac[port], true);
+}
+
 static int sja1105_pvid_apply(struct sja1105_private *priv, int port, u16 pvid)
 {
 	struct sja1105_mac_config_entry *mac;
@@ -76,6 +108,9 @@ static int sja1105_commit_pvid(struct dsa_switch *ds, int port)
 {
 	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct sja1105_private *priv = ds->priv;
+	struct sja1105_vlan_lookup_entry *vlan;
+	bool drop_untagged = false;
+	int match, rc;
 	u16 pvid;
 
 	if (dp->bridge_dev && br_vlan_enabled(dp->bridge_dev))
@@ -83,7 +118,18 @@ static int sja1105_commit_pvid(struct dsa_switch *ds, int port)
 	else
 		pvid = priv->tag_8021q_pvid[port];
 
-	return sja1105_pvid_apply(priv, port, pvid);
+	rc = sja1105_pvid_apply(priv, port, pvid);
+	if (rc)
+		return rc;
+
+	vlan = priv->static_config.tables[BLK_IDX_VLAN_LOOKUP].entries;
+
+	match = sja1105_is_vlan_configured(priv, pvid);
+
+	if (match < 0 || !(vlan[match].vmemb_port & BIT(port)))
+		drop_untagged = true;
+
+	return sja1105_drop_untagged(ds, port, drop_untagged);
 }
 
 static int sja1105_init_mac_settings(struct sja1105_private *priv)
@@ -1997,22 +2043,6 @@ sja1105_get_tag_protocol(struct dsa_switch *ds, int port,
 	return priv->info->tag_proto;
 }
 
-static int sja1105_is_vlan_configured(struct sja1105_private *priv, u16 vid)
-{
-	struct sja1105_vlan_lookup_entry *vlan;
-	int count, i;
-
-	vlan = priv->static_config.tables[BLK_IDX_VLAN_LOOKUP].entries;
-	count = priv->static_config.tables[BLK_IDX_VLAN_LOOKUP].entry_count;
-
-	for (i = 0; i < count; i++)
-		if (vlan[i].vlanid == vid)
-			return i;
-
-	/* Return an invalid entry index if not found */
-	return -1;
-}
-
 /* The TPID setting belongs to the General Parameters table,
  * which can only be partially reconfigured at runtime (and not the TPID).
  * So a switch reset is required.
@@ -2219,8 +2249,16 @@ static int sja1105_bridge_vlan_del(struct dsa_switch *ds, int port,
 				   const struct switchdev_obj_port_vlan *vlan)
 {
 	struct sja1105_private *priv = ds->priv;
+	int rc;
 
-	return sja1105_vlan_del(priv, port, vlan->vid);
+	rc = sja1105_vlan_del(priv, port, vlan->vid);
+	if (rc)
+		return rc;
+
+	/* In case the pvid was deleted, make sure that untagged packets will
+	 * be dropped.
+	 */
+	return sja1105_commit_pvid(ds, port);
 }
 
 static int sja1105_dsa_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid,
-- 
2.25.1

[PATCH net-next 3/3] net: dsa: tag_sja1105: fix control packets on SJA1110 being received on an imprecise port

[Vladimir Oltean]

On RX, a control packet with SJA1110 will have:
- an in-band control extension (DSA tag) composed of a header and an
  optional trailer (if it is a timestamp frame). We can (and do) deduce
  the source port and switch id from this.
- a VLAN header, which can either be the tag_8021q RX VLAN (pvid) or the
  bridge VLAN. The sja1105_vlan_rcv() function attempts to deduce the
  source port and switch id a second time from this.

The basic idea is that even though we don't need the source port
information from the tag_8021q header if it's a control packet, we do
need to strip that header before we pass it on to the network stack.

The problem is that we call sja1105_vlan_rcv for ports under VLAN-aware
bridges, and that function tells us it couldn't identify a tag_8021q
header, so we need to perform imprecise RX by VID. Well, we don't,
because we already know the source port and switch ID.

This patch drops the return value from sja1105_vlan_rcv and we just look
at the source_port and switch_id values from sja1105_rcv and sja1110_rcv
which were initialized to -1. If they are still -1 it means we need to
perform imprecise RX.

Fixes: 884be12f8566 ("net: dsa: sja1105: add support for imprecise RX")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/tag_sja1105.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index cddee4b499d8..c1f993d592ef 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -368,10 +368,11 @@ static bool sja1110_skb_has_inband_control_extension(const struct sk_buff *skb)
 	return ntohs(eth_hdr(skb)->h_proto) == ETH_P_SJA1110;
 }
 
-/* Returns true for imprecise RX and sets the @vid.
- * Returns false for precise RX and sets @source_port and @switch_id.
+/* If the VLAN in the packet is a tag_8021q one, set @source_port and
+ * @switch_id and strip the header. Otherwise set @vid and keep it in the
+ * packet.
  */
-static bool sja1105_vlan_rcv(struct sk_buff *skb, int *source_port,
+static void sja1105_vlan_rcv(struct sk_buff *skb, int *source_port,
 			     int *switch_id, u16 *vid)
 {
 	struct vlan_ethhdr *hdr = (struct vlan_ethhdr *)skb_mac_header(skb);
@@ -382,15 +383,11 @@ static bool sja1105_vlan_rcv(struct sk_buff *skb, int *source_port,
 	else
 		vlan_tci = ntohs(hdr->h_vlan_TCI);
 
-	if (vid_is_dsa_8021q_rxvlan(vlan_tci & VLAN_VID_MASK)) {
-		dsa_8021q_rcv(skb, source_port, switch_id);
-		return false;
-	}
+	if (vid_is_dsa_8021q_rxvlan(vlan_tci & VLAN_VID_MASK))
+		return dsa_8021q_rcv(skb, source_port, switch_id);
 
 	/* Try our best with imprecise RX */
 	*vid = vlan_tci & VLAN_VID_MASK;
-
-	return true;
 }
 
 static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
@@ -399,7 +396,6 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
 {
 	int source_port = -1, switch_id = -1;
 	struct sja1105_meta meta = {0};
-	bool imprecise_rx = false;
 	struct ethhdr *hdr;
 	bool is_link_local;
 	bool is_meta;
@@ -413,8 +409,7 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
 
 	if (sja1105_skb_has_tag_8021q(skb)) {
 		/* Normal traffic path. */
-		imprecise_rx = sja1105_vlan_rcv(skb, &source_port, &switch_id,
-						&vid);
+		sja1105_vlan_rcv(skb, &source_port, &switch_id, &vid);
 	} else if (is_link_local) {
 		/* Management traffic path. Switch embeds the switch ID and
 		 * port ID into bytes of the destination MAC, courtesy of
@@ -433,7 +428,7 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
 		return NULL;
 	}
 
-	if (imprecise_rx)
+	if (source_port == -1 || switch_id == -1)
 		skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
 	else
 		skb->dev = dsa_master_find_slave(netdev, switch_id, source_port);
@@ -550,7 +545,6 @@ static struct sk_buff *sja1110_rcv(struct sk_buff *skb,
 				   struct packet_type *pt)
 {
 	int source_port = -1, switch_id = -1;
-	bool imprecise_rx = false;
 	u16 vid;
 
 	skb->offload_fwd_mark = 1;
@@ -564,10 +558,9 @@ static struct sk_buff *sja1110_rcv(struct sk_buff *skb,
 
 	/* Packets with in-band control extensions might still have RX VLANs */
 	if (likely(sja1105_skb_has_tag_8021q(skb)))
-		imprecise_rx = sja1105_vlan_rcv(skb, &source_port, &switch_id,
-						&vid);
+		sja1105_vlan_rcv(skb, &source_port, &switch_id, &vid);
 
-	if (imprecise_rx)
+	if (source_port == -1 || switch_id == -1)
 		skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
 	else
 		skb->dev = dsa_master_find_slave(netdev, switch_id, source_port);
-- 
2.25.1

Re: [PATCH net-next 1/3] net: dsa: sja1105: reset the port pvid when leaving a VLAN-aware bridge

[Florian Fainelli]

On 7/28/2021 2:54 PM, Vladimir Oltean wrote:
> Now that we no longer have the ultra-central sja1105_build_vlan_table(),
> we need to be more careful about checking all corner cases manually.
> 
> For example, when a port leaves a VLAN-aware bridge, it becomes
> standalone so its pvid should become a tag_8021q RX VLAN again. However,
> sja1105_commit_pvid() only gets called from sja1105_bridge_vlan_add()
> and from sja1105_vlan_filtering(), and no VLAN awareness change takes
> place (VLAN filtering is a global setting for sja1105, so the switch
> remains VLAN-aware overall).
> 
> This means that we need to put another sja1105_commit_pvid() call in
> sja1105_bridge_member().
> 
> Fixes: 6dfd23d35e75 ("net: dsa: sja1105: delete vlan delta save/restore logic")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 2/3] net: dsa: sja1105: make sure untagged packets are dropped on ingress ports with no pvid

[Florian Fainelli]

On 7/28/2021 2:54 PM, Vladimir Oltean wrote:
> Surprisingly, this configuration:
> 
> ip link add br0 type bridge vlan_filtering 1
> ip link set swp2 master br0
> bridge vlan del dev swp2 vid 1
> 
> still has the sja1105 switch sending untagged packets to the CPU (and
> failing to decode them, since dsa_find_designated_bridge_port_by_vid
> searches by VID 1 and rightfully finds no bridge VLAN 1 on a port).
> 
> Dumping the switch configuration, the VLANs are managed properly:
> - the pvid of swp2 is 1 in the MAC Configuration Table, but
> - only the CPU port is in the port membership of VLANID 1 in the VLAN
>    Lookup Table
> 
> When the ingress packets are tagged with VID 1, they are properly
> dropped. But when they are untagged, they are able to reach the CPU
> port. Also, when the pvid in the MAC Configuration Table is changed to
> e.g. 55 (an unused VLAN), the untagged packets are also dropped.
> 
> So it looks like:
> - the switch bypasses ingress VLAN membership checks for untagged traffic
> - the reason why the untagged traffic is dropped when I make the pvid 55
>    is due to the lack of valid destination ports in VLAN 55, rather than
>    an ingress membership violation
> - the ingress VLAN membership cheks are only done for VLAN-tagged traffic
> 
> Interesting. It looks like there is an explicit bit to drop untagged
> traffic, so we should probably be using that to preserve user expectations.
> 
> Note that only VLAN-aware ports should drop untagged packets due to no
> pvid - when VLAN-unaware, the software bridge doesn't do this even if
> there is no pvid on any bridge port and on the bridge itself. So the new
> sja1105_drop_untagged() function cannot simply be called with "false"
> from sja1105_bridge_vlan_add() and with "true" from sja1105_bridge_vlan_del.
> Instead, we need to also consider the VLAN awareness state. That means
> we need to hook the "drop untagged" setting in all the same places where
> the "commit pvid" logic is, and it needs to factor in all the state when
> flipping the "drop untagged" bit: is our current pvid in the VLAN Lookup
> Table, and is the current port in that VLAN's port membership list?
> VLAN-unaware ports will never drop untagged frames because these checks
> always succeed by construction, and the tag_8021q VLANs cannot be changed
> by the user.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 0/3] NXP SJA1105 VLAN regressions

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Thu, 29 Jul 2021 00:54:26 +0300 you wrote:
> These are 3 patches to fix issues seen with some more varied testing
> done after the changes in the "Traffic termination for sja1105 ports
> under VLAN-aware bridge" series were made:
> https://patchwork.kernel.org/project/netdevbpf/cover/20210726165536.1338471-1-vladimir.oltean@nxp.com/
> 
> Issue 1: traffic no longer works on a port after leaving a VLAN-aware bridge
> Issue 2: untagged traffic not dropped if pvid is absent from a VLAN-aware port
> Issue 3: PTP and STP broken on ports under a VLAN-aware bridge
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] net: dsa: sja1105: reset the port pvid when leaving a VLAN-aware bridge
    https://git.kernel.org/netdev/net-next/c/cde8078e83e3
  - [net-next,2/3] net: dsa: sja1105: make sure untagged packets are dropped on ingress ports with no pvid
    https://git.kernel.org/netdev/net-next/c/bef0746cf4cc
  - [net-next,3/3] net: dsa: tag_sja1105: fix control packets on SJA1110 being received on an imprecise port
    https://git.kernel.org/netdev/net-next/c/04a1758348a8

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html