[PATCH net 0/2] Fix PTP received on wrong port with bridged SJA1105 DSA

[PATCH net 0/2] Fix PTP received on wrong port with bridged SJA1105 DSA

[Vladimir Oltean]

Since the changes were made to tag_8021q to support imprecise RX for
bridged ports, the tag_sja1105 driver still prefers the source port
information deduced from the VLAN headers for link-local traffic, even
though the switch can theoretically do better and report the precise
source port.

The problem is that the tagger doesn't know when to trust one source of
information over another, because the INCL_SRCPT option (to "tag" link
local frames) is sometimes enabled and sometimes it isn't.

The first patch makes the switch provide the hardware tag for link local
traffic under all circumstances, and the second patch makes the tagger
always use that hardware tag as primary source of information for link
local packets.

Vladimir Oltean (2):
  net: dsa: sja1105: always enable the INCL_SRCPT option
  net: dsa: tag_sja1105: always prefer source port information from
    INCL_SRCPT

 drivers/net/dsa/sja1105/sja1105_main.c |  9 ++-----
 net/dsa/tag_sja1105.c                  | 35 ++++++++++++++++++--------
 2 files changed, 27 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH net 1/2] net: dsa: sja1105: always enable the INCL_SRCPT option

[Vladimir Oltean]

Link-local traffic on bridged SJA1105 ports is sometimes tagged by the
hardware with source port information (when the port is under a VLAN
aware bridge).

The tag_8021q source port identification has become more loose
("imprecise") and will report a plausible rather than exact bridge port,
when under a bridge (be it VLAN-aware or VLAN-unaware). But link-local
traffic always needs to know the precise source port.

Modify the driver logic (and therefore: the tagging protocol itself) to
always include the source port information with link-local packets,
regardless of whether the port is standalone, under a VLAN-aware or
VLAN-unaware bridge. This makes it possible for the tagging driver to
give priority to that information over the tag_8021q VLAN header.

The big drawback with INCL_SRCPT is that it makes it impossible to
distinguish between an original MAC DA of 01:80:C2:XX:YY:ZZ and
01:80:C2:AA:BB:ZZ, because the tagger just patches MAC DA bytes 3 and 4
with zeroes. Only if PTP RX timestamping is enabled, the switch will
generate a META follow-up frame containing the RX timestamp and the
original bytes 3 and 4 of the MAC DA. Those will be used to patch up the
original packet. Nonetheless, in the absence of PTP RX timestamping, we
have to live with this limitation, since it is more important to have
the more precise source port information for link-local traffic.

Fixes: d7f9787a763f ("net: dsa: tag_8021q: add support for imprecise RX based on the VBID")
Fixes: 91495f21fcec ("net: dsa: tag_8021q: replace the SVL bridging with VLAN-unaware IVL bridging")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index a55a6436fc05..dd154b2b9680 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -866,11 +866,11 @@ static int sja1105_init_general_params(struct sja1105_private *priv)
 		.hostprio = 7,
 		.mac_fltres1 = SJA1105_LINKLOCAL_FILTER_A,
 		.mac_flt1    = SJA1105_LINKLOCAL_FILTER_A_MASK,
-		.incl_srcpt1 = false,
+		.incl_srcpt1 = true,
 		.send_meta1  = false,
 		.mac_fltres0 = SJA1105_LINKLOCAL_FILTER_B,
 		.mac_flt0    = SJA1105_LINKLOCAL_FILTER_B_MASK,
-		.incl_srcpt0 = false,
+		.incl_srcpt0 = true,
 		.send_meta0  = false,
 		/* Default to an invalid value */
 		.mirr_port = priv->ds->num_ports,
@@ -2405,11 +2405,6 @@ int sja1105_vlan_filtering(struct dsa_switch *ds, int port, bool enabled,
 	general_params->tpid = tpid;
 	/* EtherType used to identify outer tagged (S-tag) VLAN traffic */
 	general_params->tpid2 = tpid2;
-	/* When VLAN filtering is on, we need to at least be able to
-	 * decode management traffic through the "backup plan".
-	 */
-	general_params->incl_srcpt1 = enabled;
-	general_params->incl_srcpt0 = enabled;
 
 	for (port = 0; port < ds->num_ports; port++) {
 		if (dsa_is_unused_port(ds, port))
-- 
2.34.1

[PATCH net 2/2] net: dsa: tag_sja1105: always prefer source port information from INCL_SRCPT

[Vladimir Oltean]

Currently the sja1105 tagging protocol prefers using the source port
information from the VLAN header if that is available, falling back to
the INCL_SRCPT option if it isn't. The VLAN header is available for all
frames except for META frames initiated by the switch (containing RX
timestamps), and thus, the "if (is_link_local)" branch is practically
dead.

The tag_8021q source port identification has become more loose
("imprecise") and will report a plausible rather than exact bridge port,
when under a bridge (be it VLAN-aware or VLAN-unaware). But link-local
traffic always needs to know the precise source port. With incorrect
source port reporting, for example PTP traffic over 2 bridged ports will
all be seen on sockets opened on the first such port, which is incorrect.

Now that the tagging protocol has been changed to make link-local frames
always contain source port information, we can reverse the order of the
checks so that we always give precedence to that information (which is
always precise) in lieu of the tag_8021q VID which is only precise for a
standalone port.

Fixes: d7f9787a763f ("net: dsa: tag_8021q: add support for imprecise RX based on the VBID")
Fixes: 91495f21fcec ("net: dsa: tag_8021q: replace the SVL bridging with VLAN-unaware IVL bridging")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/tag_sja1105.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index a5f3b73da417..0e62eab8f251 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -545,10 +545,7 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
 	is_link_local = sja1105_is_link_local(skb);
 	is_meta = sja1105_is_meta_frame(skb);
 
-	if (sja1105_skb_has_tag_8021q(skb)) {
-		/* Normal traffic path. */
-		sja1105_vlan_rcv(skb, &source_port, &switch_id, &vbid, &vid);
-	} else if (is_link_local) {
+	if (is_link_local) {
 		/* Management traffic path. Switch embeds the switch ID and
 		 * port ID into bytes of the destination MAC, courtesy of
 		 * the incl_srcpt options.
@@ -562,16 +559,34 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
 		sja1105_meta_unpack(skb, &meta);
 		source_port = meta.source_port;
 		switch_id = meta.switch_id;
-	} else {
-		return NULL;
 	}
 
-	if (vbid >= 1)
+	/* Normal data plane traffic and link-local frames are tagged with
+	 * a tag_8021q VLAN which we have to strip
+	 */
+	if (sja1105_skb_has_tag_8021q(skb)) {
+		int tmp_source_port = -1, tmp_switch_id = -1;
+
+		sja1105_vlan_rcv(skb, &tmp_source_port, &tmp_switch_id, &vbid,
+				 &vid);
+		/* Preserve the source information from the INCL_SRCPT option,
+		 * if available. This allows us to not overwrite a valid source
+		 * port and switch ID with zeroes when receiving link-local
+		 * frames from a VLAN-unaware bridged port (non-zero vbid) or a
+		 * VLAN-aware bridged port (non-zero vid).
+		 */
+		if (source_port == -1)
+			source_port = tmp_source_port;
+		if (switch_id == -1)
+			switch_id = tmp_switch_id;
+	}
+
+	if (source_port != -1 && switch_id != -1)
+		skb->dev = dsa_master_find_slave(netdev, switch_id, source_port);
+	else if (vbid >= 1)
 		skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
-	else 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);
+		skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
 	if (!skb->dev) {
 		netdev_warn(netdev, "Couldn't decode source port\n");
 		return NULL;
-- 
2.34.1

Re: [PATCH net 2/2] net: dsa: tag_sja1105: always prefer source port information from INCL_SRCPT

[Simon Horman]

On Mon, Jun 26, 2023 at 06:51:12PM +0300, Vladimir Oltean wrote:
> Currently the sja1105 tagging protocol prefers using the source port
> information from the VLAN header if that is available, falling back to
> the INCL_SRCPT option if it isn't. The VLAN header is available for all
> frames except for META frames initiated by the switch (containing RX
> timestamps), and thus, the "if (is_link_local)" branch is practically
> dead.
> 
> The tag_8021q source port identification has become more loose
> ("imprecise") and will report a plausible rather than exact bridge port,
> when under a bridge (be it VLAN-aware or VLAN-unaware). But link-local
> traffic always needs to know the precise source port. With incorrect
> source port reporting, for example PTP traffic over 2 bridged ports will
> all be seen on sockets opened on the first such port, which is incorrect.
> 
> Now that the tagging protocol has been changed to make link-local frames
> always contain source port information, we can reverse the order of the
> checks so that we always give precedence to that information (which is
> always precise) in lieu of the tag_8021q VID which is only precise for a
> standalone port.
> 
> Fixes: d7f9787a763f ("net: dsa: tag_8021q: add support for imprecise RX based on the VBID")
> Fixes: 91495f21fcec ("net: dsa: tag_8021q: replace the SVL bridging with VLAN-unaware IVL bridging")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/dsa/tag_sja1105.c | 35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
> index a5f3b73da417..0e62eab8f251 100644
> --- a/net/dsa/tag_sja1105.c
> +++ b/net/dsa/tag_sja1105.c
> @@ -545,10 +545,7 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
>  	is_link_local = sja1105_is_link_local(skb);
>  	is_meta = sja1105_is_meta_frame(skb);
>  
> -	if (sja1105_skb_has_tag_8021q(skb)) {
> -		/* Normal traffic path. */
> -		sja1105_vlan_rcv(skb, &source_port, &switch_id, &vbid, &vid);
> -	} else if (is_link_local) {
> +	if (is_link_local) {
>  		/* Management traffic path. Switch embeds the switch ID and
>  		 * port ID into bytes of the destination MAC, courtesy of
>  		 * the incl_srcpt options.
> @@ -562,16 +559,34 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
>  		sja1105_meta_unpack(skb, &meta);
>  		source_port = meta.source_port;
>  		switch_id = meta.switch_id;
> -	} else {
> -		return NULL;
>  	}
>  
> -	if (vbid >= 1)
> +	/* Normal data plane traffic and link-local frames are tagged with
> +	 * a tag_8021q VLAN which we have to strip
> +	 */
> +	if (sja1105_skb_has_tag_8021q(skb)) {
> +		int tmp_source_port = -1, tmp_switch_id = -1;
> +
> +		sja1105_vlan_rcv(skb, &tmp_source_port, &tmp_switch_id, &vbid,
> +				 &vid);
> +		/* Preserve the source information from the INCL_SRCPT option,
> +		 * if available. This allows us to not overwrite a valid source
> +		 * port and switch ID with zeroes when receiving link-local
> +		 * frames from a VLAN-unaware bridged port (non-zero vbid) or a
> +		 * VLAN-aware bridged port (non-zero vid).
> +		 */
> +		if (source_port == -1)
> +			source_port = tmp_source_port;
> +		if (switch_id == -1)
> +			switch_id = tmp_switch_id;
> +	}
> +
> +	if (source_port != -1 && switch_id != -1)
> +		skb->dev = dsa_master_find_slave(netdev, switch_id, source_port);
> +	else if (vbid >= 1)
>  		skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
> -	else 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);
> +		skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);

Hi Vladimir,

A similar comment to that made for [1], though the code is somewhat
different to that case: are you sure vid is initialised here?
GCC 12 and Smatch seem unsure about it.

[1] Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q
    https://lore.kernel.org/all/ZJg2M+Qvg3Fv73CH@corigine.com/

>  	if (!skb->dev) {
>  		netdev_warn(netdev, "Couldn't decode source port\n");
>  		return NULL;
> -- 
> 2.34.1
> 
> 

Re: [PATCH net 2/2] net: dsa: tag_sja1105: always prefer source port information from INCL_SRCPT

[Vladimir Oltean]

Hi Simon,

On Mon, Jun 26, 2023 at 08:11:53PM +0200, Simon Horman wrote:
> Hi Vladimir,
> 
> A similar comment to that made for [1], though the code is somewhat
> different to that case: are you sure vid is initialised here?
> GCC 12 and Smatch seem unsure about it.
> 
> [1] Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q
>     https://lore.kernel.org/all/ZJg2M+Qvg3Fv73CH@corigine.com/

"vid" can be uninitialized if the tagger is fed a junk packet (a
non-link-local, non-meta packet that also has no tag_8021q header).

The immediate answer that comes to mind is: it depends on how the driver
configures the hardware to send packets to the CPU (and it will never
configure the switch in that way).

But, between the sja1105 driver configuring the switch in a certain way
and the tag_sja1105 driver seeing the results of that, there's also the
DSA master driver (can be any net_device) which can alter the packet in
a nonsensical way, like remove the VLAN header for some reason.

Considering the fact that the DSA master can have tc rules on its
ingress path which do just that, it would probably be wise to be
defensive about this. So I can probably add:

	if (sja1105_skb_has_tag_8021q(skb)) {
		... // existing call to sja1105_vlan_rcv() here
	} else if (source_port == -1 && switch_id == -1) {
		/* Packets with no source information have no chance of
		 * getting accepted, drop them straight away.
		 */
		return NULL;
	}

This "else if" block should ensure that when "vid" is uninitialized,
either "source_port" and "switch_id", or "vbid", always have valid values.

Re: [PATCH net 2/2] net: dsa: tag_sja1105: always prefer source port information from INCL_SRCPT

[Simon Horman]

On Tue, Jun 27, 2023 at 01:18:28AM +0300, Vladimir Oltean wrote:
> Hi Simon,
> 
> On Mon, Jun 26, 2023 at 08:11:53PM +0200, Simon Horman wrote:
> > Hi Vladimir,
> > 
> > A similar comment to that made for [1], though the code is somewhat
> > different to that case: are you sure vid is initialised here?
> > GCC 12 and Smatch seem unsure about it.
> > 
> > [1] Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q
> >     https://lore.kernel.org/all/ZJg2M+Qvg3Fv73CH@corigine.com/
> 
> "vid" can be uninitialized if the tagger is fed a junk packet (a
> non-link-local, non-meta packet that also has no tag_8021q header).
> 
> The immediate answer that comes to mind is: it depends on how the driver
> configures the hardware to send packets to the CPU (and it will never
> configure the switch in that way).
> 
> But, between the sja1105 driver configuring the switch in a certain way
> and the tag_sja1105 driver seeing the results of that, there's also the
> DSA master driver (can be any net_device) which can alter the packet in
> a nonsensical way, like remove the VLAN header for some reason.
> 
> Considering the fact that the DSA master can have tc rules on its
> ingress path which do just that, it would probably be wise to be
> defensive about this. So I can probably add:
> 
> 	if (sja1105_skb_has_tag_8021q(skb)) {
> 		... // existing call to sja1105_vlan_rcv() here
> 	} else if (source_port == -1 && switch_id == -1) {
> 		/* Packets with no source information have no chance of
> 		 * getting accepted, drop them straight away.
> 		 */
> 		return NULL;
> 	}
> 
> This "else if" block should ensure that when "vid" is uninitialized,
> either "source_port" and "switch_id", or "vbid", always have valid values.

This is kind of complex :)

Can I clarify that either:

1. Both source_port and switch_id are -1; or
2. Neither source_port nor switch_id are -1

If so, I agree with your proposal.

Re: [PATCH net 2/2] net: dsa: tag_sja1105: always prefer source port information from INCL_SRCPT

[Vladimir Oltean]

On Tue, Jun 27, 2023 at 01:15:03PM +0200, Simon Horman wrote:
> On Tue, Jun 27, 2023 at 01:18:28AM +0300, Vladimir Oltean wrote:
> > Hi Simon,
> > 
> > On Mon, Jun 26, 2023 at 08:11:53PM +0200, Simon Horman wrote:
> > > Hi Vladimir,
> > > 
> > > A similar comment to that made for [1], though the code is somewhat
> > > different to that case: are you sure vid is initialised here?
> > > GCC 12 and Smatch seem unsure about it.
> > > 
> > > [1] Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q
> > >     https://lore.kernel.org/all/ZJg2M+Qvg3Fv73CH@corigine.com/
> > 
> > "vid" can be uninitialized if the tagger is fed a junk packet (a
> > non-link-local, non-meta packet that also has no tag_8021q header).
> > 
> > The immediate answer that comes to mind is: it depends on how the driver
> > configures the hardware to send packets to the CPU (and it will never
> > configure the switch in that way).
> > 
> > But, between the sja1105 driver configuring the switch in a certain way
> > and the tag_sja1105 driver seeing the results of that, there's also the
> > DSA master driver (can be any net_device) which can alter the packet in
> > a nonsensical way, like remove the VLAN header for some reason.
> > 
> > Considering the fact that the DSA master can have tc rules on its
> > ingress path which do just that, it would probably be wise to be
> > defensive about this. So I can probably add:
> > 
> > 	if (sja1105_skb_has_tag_8021q(skb)) {
> > 		... // existing call to sja1105_vlan_rcv() here
> > 	} else if (source_port == -1 && switch_id == -1) {
> > 		/* Packets with no source information have no chance of
> > 		 * getting accepted, drop them straight away.
> > 		 */
> > 		return NULL;
> > 	}
> > 
> > This "else if" block should ensure that when "vid" is uninitialized,
> > either "source_port" and "switch_id", or "vbid", always have valid values.
> 
> This is kind of complex :)
> 
> Can I clarify that either:
> 
> 1. Both source_port and switch_id are -1; or
> 2. Neither source_port nor switch_id are -1
> 
> If so, I agree with your proposal.

They are integers assigned from the same code blocks in all cases,
starting with -1 and later being assigned rvalues either from u64 fields
limited to 0-255 (meta->source_port, meta->switch_id) or from unsigned
char fields (hdr->h_dest[3], hdr->h_dest[4]), or from
dsa_8021q_rx_source_port() and dsa_8021q_rx_switch_id() which return
limited-size positive integers due to their implementation.

Re: [PATCH net 2/2] net: dsa: tag_sja1105: always prefer source port information from INCL_SRCPT

[Simon Horman]

On Tue, Jun 27, 2023 at 02:41:48PM +0300, Vladimir Oltean wrote:
> On Tue, Jun 27, 2023 at 01:15:03PM +0200, Simon Horman wrote:
> > On Tue, Jun 27, 2023 at 01:18:28AM +0300, Vladimir Oltean wrote:
> > > Hi Simon,
> > > 
> > > On Mon, Jun 26, 2023 at 08:11:53PM +0200, Simon Horman wrote:
> > > > Hi Vladimir,
> > > > 
> > > > A similar comment to that made for [1], though the code is somewhat
> > > > different to that case: are you sure vid is initialised here?
> > > > GCC 12 and Smatch seem unsure about it.
> > > > 
> > > > [1] Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q
> > > >     https://lore.kernel.org/all/ZJg2M+Qvg3Fv73CH@corigine.com/
> > > 
> > > "vid" can be uninitialized if the tagger is fed a junk packet (a
> > > non-link-local, non-meta packet that also has no tag_8021q header).
> > > 
> > > The immediate answer that comes to mind is: it depends on how the driver
> > > configures the hardware to send packets to the CPU (and it will never
> > > configure the switch in that way).
> > > 
> > > But, between the sja1105 driver configuring the switch in a certain way
> > > and the tag_sja1105 driver seeing the results of that, there's also the
> > > DSA master driver (can be any net_device) which can alter the packet in
> > > a nonsensical way, like remove the VLAN header for some reason.
> > > 
> > > Considering the fact that the DSA master can have tc rules on its
> > > ingress path which do just that, it would probably be wise to be
> > > defensive about this. So I can probably add:
> > > 
> > > 	if (sja1105_skb_has_tag_8021q(skb)) {
> > > 		... // existing call to sja1105_vlan_rcv() here
> > > 	} else if (source_port == -1 && switch_id == -1) {
> > > 		/* Packets with no source information have no chance of
> > > 		 * getting accepted, drop them straight away.
> > > 		 */
> > > 		return NULL;
> > > 	}
> > > 
> > > This "else if" block should ensure that when "vid" is uninitialized,
> > > either "source_port" and "switch_id", or "vbid", always have valid values.
> > 
> > This is kind of complex :)
> > 
> > Can I clarify that either:
> > 
> > 1. Both source_port and switch_id are -1; or
> > 2. Neither source_port nor switch_id are -1
> > 
> > If so, I agree with your proposal.
> 
> They are integers assigned from the same code blocks in all cases,
> starting with -1 and later being assigned rvalues either from u64 fields
> limited to 0-255 (meta->source_port, meta->switch_id) or from unsigned
> char fields (hdr->h_dest[3], hdr->h_dest[4]), or from
> dsa_8021q_rx_source_port() and dsa_8021q_rx_switch_id() which return
> limited-size positive integers due to their implementation.

Thanks, in that case I think we are good.