[PATCH net-next] net: dsa: sja1105: disable rxvlan offload for the DSA master

* [PATCH net-next] net: dsa: sja1105: disable rxvlan offload for the DSA master
@ 2020-05-12 23:49 Vladimir Oltean
  2020-05-13  1:03 ` David Miller
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Vladimir Oltean @ 2020-05-12 23:49 UTC (permalink / raw)
  To: andrew, f.fainelli, vivien.didelot
  Cc: davem, kuba, eric.dumazet, jiri, idosch, rmk+kernel, netdev,
	linux-kernel

From: Vladimir Oltean <vladimir.oltean@nxp.com>

On sja1105 operating in best_effort_vlan_filtering mode (when the TPID
of the DSA tags is 0x8100), it can be seen that __netif_receive_skb_core
calls __vlan_hwaccel_clear_tag right before passing the skb to the DSA
packet_type handler.

This means that the tagger does not see the VLAN tag in the skb, nor in
the skb meta data.

The patch that started zeroing the skb VLAN tag is:

  commit d4b812dea4a236f729526facf97df1a9d18e191c
  Author: Eric Dumazet <edumazet@xxxxxxxxxx>
  Date:   Thu Jul 18 07:19:26 2013 -0700

      vlan: mask vlan prio bits

      In commit 48cc32d38a52d0b68f91a171a8d00531edc6a46e
      ("vlan: don't deliver frames for unknown vlans to protocols")
      Florian made sure we set pkt_type to PACKET_OTHERHOST
      if the vlan id is set and we could find a vlan device for this
      particular id.

      But we also have a problem if prio bits are set.

      Steinar reported an issue on a router receiving IPv6 frames with a
      vlan tag of 4000 (id 0, prio 2), and tunneled into a sit device,
      because skb->vlan_tci is set.

      Forwarded frame is completely corrupted : We can see (8100:4000)
      being inserted in the middle of IPv6 source address :

      16:48:00.780413 IP6 2001:16d8:8100:4000:ee1c:0:9d9:bc87 >
      9f94:4d95:2001:67c:29f4::: ICMP6, unknown icmp6 type (0), length 64
             0x0000:  0000 0029 8000 c7c3 7103 0001 a0ae e651
             0x0010:  0000 0000 ccce 0b00 0000 0000 1011 1213
             0x0020:  1415 1617 1819 1a1b 1c1d 1e1f 2021 2223
             0x0030:  2425 2627 2829 2a2b 2c2d 2e2f 3031 3233

      It seems we are not really ready to properly cope with this right now.

      We can probably do better in future kernels :
      vlan_get_ingress_priority() should be a netdev property instead of
      a per vlan_dev one.

      For stable kernels, lets clear vlan_tci to fix the bugs.

      Reported-by: Steinar H. Gunderson <sesse@xxxxxxxxxx>
      Signed-off-by: Eric Dumazet <edumazet@xxxxxxxxxx>
      Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>

The patch doesn't say why "we are not really ready to properly cope with
this right now", and hence why the best solution is to remove the VLAN
tag from skb's that don't have a local VLAN sub-interface interested in
them. And I have no idea either.

But the above patch has a loophole: if the VLAN tag is not
hw-accelerated, it isn't removed from the skb if there is no VLAN
sub-interface interested in it (our case). So we are hooking into the
.ndo_fix_features callback of the DSA master and clearing the rxvlan
offload feature, so the DSA tagger will always see the VLAN as part of
the skb data. This is symmetrical with the ETH_P_DSA_8021Q case and does
not need special treatment in the tagger.

If there was an API by which the dsa tag_8021q module would declare its
interest in servicing VLANs 1024-3071, such that the packets wouldn't be
classified as PACKET_OTHERHOST, and if that API wasn't as tightly
integrated with the 8021q module as vlan_find_dev/vlan_group_set_device
are, I would be interested in using it, but so far I couldn't find it.
With this patch, even though the frames still are PACKET_OTHERHOST, at
least the VLAN tag reaches far enough that the DSA packet_type handler
sees and consumes it.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c |  2 ++
 include/net/dsa.h                      |  3 +++
 net/dsa/master.c                       | 13 +++++++++++++
 3 files changed, 18 insertions(+)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 44ce7882dfb1..24757c8adfe7 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -2912,6 +2912,8 @@ static int sja1105_setup(struct dsa_switch *ds)
 
 	ds->mtu_enforcement_ingress = true;
 
+	ds->disable_master_rxvlan = true;
+
 	ds->configure_vlan_while_not_filtering = true;
 
 	rc = sja1105_setup_devlink_params(ds);
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 50389772c597..3938b20461de 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -299,6 +299,9 @@ struct dsa_switch {
 	 */
 	bool			pcs_poll;
 
+	/* Necessary for tagging protocols such as tag_8021q. */
+	bool			disable_master_rxvlan;
+
 	/* For switches that only have the MRU configurable. To ensure the
 	 * configured MTU is not exceeded, normalization of MRU on all bridged
 	 * interfaces is needed.
diff --git a/net/dsa/master.c b/net/dsa/master.c
index a621367c6e8c..12e8126bc29c 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -197,6 +197,18 @@ static int dsa_master_get_phys_port_name(struct net_device *dev,
 	return 0;
 }
 
+static netdev_features_t dsa_master_fix_features(struct net_device *dev,
+						 netdev_features_t features)
+{
+	struct dsa_port *cpu_dp = dev->dsa_ptr;
+	struct dsa_switch *ds = cpu_dp->ds;
+
+	if (ds->disable_master_rxvlan)
+		features &= ~NETIF_F_HW_VLAN_CTAG_RX;
+
+	return features;
+}
+
 static int dsa_master_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
 	struct dsa_port *cpu_dp = dev->dsa_ptr;
@@ -278,6 +290,7 @@ static int dsa_master_ndo_setup(struct net_device *dev)
 		memcpy(ops, cpu_dp->orig_ndo_ops, sizeof(*ops));
 
 	ops->ndo_get_phys_port_name = dsa_master_get_phys_port_name;
+	ops->ndo_fix_features = dsa_master_fix_features;
 	ops->ndo_do_ioctl = dsa_master_ioctl;
 
 	dev->netdev_ops  = ops;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread