[PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[Lukasz Majewski]

Provide fix to decode correctly supervisory frames when HSRv1 version of
the HSR protocol is used.

Without this patch console is polluted with:
ksz-switch spi1.0 lan1: hsr_addr_subst_dest: Unknown node

as a result of destination node's A MAC address equals to:
00:00:00:00:00:00.

cat /sys/kernel/debug/hsr/hsr0/node_table
Node Table entries for (HSR) device
MAC-Address-A,    MAC-Address-B,    time_in[A], time_in[B], Address-B
00:00:00:00:00:00 00:10:a1:94:77:30      400bf,       399c,	        0

It was caused by wrong frames decoding in the hsr_handle_sup_frame().

As the supervisor frame is encapsulated in HSRv1 frame:

SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34
SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000040: 00 00

The code had to be adjusted accordingly and the MAC-Address-A now
has the proper address (the MAC-Address-B now has all 0's).

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 net/hsr/hsr_framereg.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
index 80fc71daf7ca..85abe052e0a9 100644
--- a/net/hsr/hsr_framereg.c
+++ b/net/hsr/hsr_framereg.c
@@ -300,9 +300,24 @@ void hsr_handle_sup_frame(struct hsr_frame_info *frame)
 
 	ethhdr = (struct ethhdr *)skb_mac_header(skb);
 
-	/* And leave the HSR tag. */
+	 * And leave the HSR tag.
+	 *
+	 * The HSRv1 supervisory frame encapsulates the v0 frame
+	 * with EtherType of 0x88FB
+	 */
 	if (ethhdr->h_proto == htons(ETH_P_HSR)) {
-		pull_size = sizeof(struct ethhdr);
+		if (hsr->prot_version == HSR_V1)
+			/* In the above step the DA, SA and EtherType
+			 * (0x892F - HSRv1) bytes has been removed.
+			 *
+			 * As the HSRv1 has the HSR header added, one need
+			 * to remove path_and_LSDU_size and sequence_nr fields.
+			 *
+			 */
+			pull_size = 4;
+		else
+			pull_size = sizeof(struct hsr_tag);
+
 		skb_pull(skb, pull_size);
 		total_pull_size += pull_size;
 	}
@@ -313,6 +328,19 @@ void hsr_handle_sup_frame(struct hsr_frame_info *frame)
 	total_pull_size += pull_size;
 
 	/* get HSR sup payload */
+	if (hsr->prot_version == HSR_V1) {
+		/* In the HSRv1 supervisor frame, when
+		 * one with EtherType = 0x88FB is extracted, the Node A
+		 * MAC address is preceded with type and length elements of TLV
+		 * data field.
+		 *
+		 * It needs to be removed to get the remote peer MAC address.
+		 */
+		pull_size = sizeof(struct hsr_sup_tlv);
+		skb_pull(skb, pull_size);
+		total_pull_size += pull_size;
+	}
+
 	hsr_sp = (struct hsr_sup_payload *)skb->data;
 
 	/* Merge node_curr (registered on macaddress_B) into node_real */
-- 
2.20.1

RE: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[Tristram.Ha]

> -       /* And leave the HSR tag. */
> +        * And leave the HSR tag.
> +        *
> +        * The HSRv1 supervisory frame encapsulates the v0 frame
> +        * with EtherType of 0x88FB
> +        */
>         if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> -               pull_size = sizeof(struct ethhdr);
> +               if (hsr->prot_version == HSR_V1)
> +                       /* In the above step the DA, SA and EtherType
> +                        * (0x892F - HSRv1) bytes has been removed.
> +                        *
> +                        * As the HSRv1 has the HSR header added, one need
> +                        * to remove path_and_LSDU_size and sequence_nr fields.
> +                        *
> +                        */
> +                       pull_size = 4;
> +               else
> +                       pull_size = sizeof(struct hsr_tag);
> +
>                 skb_pull(skb, pull_size);
>                 total_pull_size += pull_size;
>         }
> @@ -313,6 +328,19 @@ void hsr_handle_sup_frame(struct hsr_frame_info *frame)
>         total_pull_size += pull_size;
> 
>         /* get HSR sup payload */
> +       if (hsr->prot_version == HSR_V1) {
> +               /* In the HSRv1 supervisor frame, when
> +                * one with EtherType = 0x88FB is extracted, the Node A
> +                * MAC address is preceded with type and length elements of TLV
> +                * data field.
> +                *
> +                * It needs to be removed to get the remote peer MAC address.
> +                */
> +               pull_size = sizeof(struct hsr_sup_tlv);
> +               skb_pull(skb, pull_size);
> +               total_pull_size += pull_size;
> +       }
> +
>         hsr_sp = (struct hsr_sup_payload *)skb->data;

I thought the fix is simply this:

	if (ethhdr->h_proto == htons(ETH_P_HSR)) {
-		pull_size = sizeof(struct ethhdr);
+		pull_size = sizeof(struct hsr_tag);
		skb_pull(skb, pull_size);
		total_pull_size += pull_size;
	}

-	pull_size = sizeof(struct hsr_tag);
+	pull_size = sizeof(struct hsr_sup_tag);

Note the sizes of hsr_tag and hsr_sup_tag are the same: 6 bytes.
The code in 5.15 before this refactored code uses those structures.
When using v0 the EtherType uses the PRP tag instead of the HSR tag so
the HSR related code is not executed.

Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[kernel test robot]

Hi Lukasz,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]
[also build test ERROR on net/main linus/master v6.5-rc7 next-20230825]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lukasz-Majewski/net-hsr-Provide-fix-for-HSRv1-supervisor-frames-decoding/20230825-233423
base:   net-next/main
patch link:    https://lore.kernel.org/r/20230825153111.228768-1-lukma%40denx.de
patch subject: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
config: powerpc64-randconfig-r022-20230826 (https://download.01.org/0day-ci/archive/20230826/202308260733.G7tU8UHx-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20230826/202308260733.G7tU8UHx-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308260733.G7tU8UHx-lkp@intel.com/

All errors (new ones prefixed by >>):

>> net/hsr/hsr_framereg.c:289:8: error: expected ';' after expression
     289 |          * And leave the HSR tag.
         |               ^
         |               ;
>> net/hsr/hsr_framereg.c:289:5: error: use of undeclared identifier 'And'
     289 |          * And leave the HSR tag.
         |            ^
>> net/hsr/hsr_framereg.c:289:9: error: use of undeclared identifier 'leave'
     289 |          * And leave the HSR tag.
         |                ^
   3 errors generated.


vim +289 net/hsr/hsr_framereg.c

   249	
   250	/* Use the Supervision frame's info about an eventual macaddress_B for merging
   251	 * nodes that has previously had their macaddress_B registered as a separate
   252	 * node.
   253	 */
   254	void hsr_handle_sup_frame(struct hsr_frame_info *frame)
   255	{
   256		struct hsr_node *node_curr = frame->node_src;
   257		struct hsr_port *port_rcv = frame->port_rcv;
   258		struct hsr_priv *hsr = port_rcv->hsr;
   259		struct hsr_sup_payload *hsr_sp;
   260		struct hsr_sup_tlv *hsr_sup_tlv;
   261		struct hsr_node *node_real;
   262		struct sk_buff *skb = NULL;
   263		struct list_head *node_db;
   264		struct ethhdr *ethhdr;
   265		int i;
   266		unsigned int pull_size = 0;
   267		unsigned int total_pull_size = 0;
   268	
   269		/* Here either frame->skb_hsr or frame->skb_prp should be
   270		 * valid as supervision frame always will have protocol
   271		 * header info.
   272		 */
   273		if (frame->skb_hsr)
   274			skb = frame->skb_hsr;
   275		else if (frame->skb_prp)
   276			skb = frame->skb_prp;
   277		else if (frame->skb_std)
   278			skb = frame->skb_std;
   279		if (!skb)
   280			return;
   281	
   282		/* Leave the ethernet header. */
   283		pull_size = sizeof(struct ethhdr);
   284		skb_pull(skb, pull_size);
   285		total_pull_size += pull_size;
   286	
   287		ethhdr = (struct ethhdr *)skb_mac_header(skb);
   288	
 > 289		 * And leave the HSR tag.
   290		 *
   291		 * The HSRv1 supervisory frame encapsulates the v0 frame
   292		 * with EtherType of 0x88FB
   293		 */
   294		if (ethhdr->h_proto == htons(ETH_P_HSR)) {
   295			if (hsr->prot_version == HSR_V1)
   296				/* In the above step the DA, SA and EtherType
   297				 * (0x892F - HSRv1) bytes has been removed.
   298				 *
   299				 * As the HSRv1 has the HSR header added, one need
   300				 * to remove path_and_LSDU_size and sequence_nr fields.
   301				 *
   302				 */
   303				pull_size = 4;
   304			else
   305				pull_size = sizeof(struct hsr_tag);
   306	
   307			skb_pull(skb, pull_size);
   308			total_pull_size += pull_size;
   309		}
   310	
   311		/* And leave the HSR sup tag. */
   312		pull_size = sizeof(struct hsr_tag);
   313		skb_pull(skb, pull_size);
   314		total_pull_size += pull_size;
   315	
   316		/* get HSR sup payload */
   317		if (hsr->prot_version == HSR_V1) {
   318			/* In the HSRv1 supervisor frame, when
   319			 * one with EtherType = 0x88FB is extracted, the Node A
   320			 * MAC address is preceded with type and length elements of TLV
   321			 * data field.
   322			 *
   323			 * It needs to be removed to get the remote peer MAC address.
   324			 */
   325			pull_size = sizeof(struct hsr_sup_tlv);
   326			skb_pull(skb, pull_size);
   327			total_pull_size += pull_size;
   328		}
   329	
   330		hsr_sp = (struct hsr_sup_payload *)skb->data;
   331	
   332		/* Merge node_curr (registered on macaddress_B) into node_real */
   333		node_db = &port_rcv->hsr->node_db;
   334		node_real = find_node_by_addr_A(node_db, hsr_sp->macaddress_A);
   335		if (!node_real)
   336			/* No frame received from AddrA of this node yet */
   337			node_real = hsr_add_node(hsr, node_db, hsr_sp->macaddress_A,
   338						 HSR_SEQNR_START - 1, true,
   339						 port_rcv->type);
   340		if (!node_real)
   341			goto done; /* No mem */
   342		if (node_real == node_curr)
   343			/* Node has already been merged */
   344			goto done;
   345	
   346		/* Leave the first HSR sup payload. */
   347		pull_size = sizeof(struct hsr_sup_payload);
   348		skb_pull(skb, pull_size);
   349		total_pull_size += pull_size;
   350	
   351		/* Get second supervision tlv */
   352		hsr_sup_tlv = (struct hsr_sup_tlv *)skb->data;
   353		/* And check if it is a redbox mac TLV */
   354		if (hsr_sup_tlv->HSR_TLV_type == PRP_TLV_REDBOX_MAC) {
   355			/* We could stop here after pushing hsr_sup_payload,
   356			 * or proceed and allow macaddress_B and for redboxes.
   357			 */
   358			/* Sanity check length */
   359			if (hsr_sup_tlv->HSR_TLV_length != 6)
   360				goto done;
   361	
   362			/* Leave the second HSR sup tlv. */
   363			pull_size = sizeof(struct hsr_sup_tlv);
   364			skb_pull(skb, pull_size);
   365			total_pull_size += pull_size;
   366	
   367			/* Get redbox mac address. */
   368			hsr_sp = (struct hsr_sup_payload *)skb->data;
   369	
   370			/* Check if redbox mac and node mac are equal. */
   371			if (!ether_addr_equal(node_real->macaddress_A, hsr_sp->macaddress_A)) {
   372				/* This is a redbox supervision frame for a VDAN! */
   373				goto done;
   374			}
   375		}
   376	
   377		ether_addr_copy(node_real->macaddress_B, ethhdr->h_source);
   378		spin_lock_bh(&node_real->seq_out_lock);
   379		for (i = 0; i < HSR_PT_PORTS; i++) {
   380			if (!node_curr->time_in_stale[i] &&
   381			    time_after(node_curr->time_in[i], node_real->time_in[i])) {
   382				node_real->time_in[i] = node_curr->time_in[i];
   383				node_real->time_in_stale[i] =
   384							node_curr->time_in_stale[i];
   385			}
   386			if (seq_nr_after(node_curr->seq_out[i], node_real->seq_out[i]))
   387				node_real->seq_out[i] = node_curr->seq_out[i];
   388		}
   389		spin_unlock_bh(&node_real->seq_out_lock);
   390		node_real->addr_B_port = port_rcv->type;
   391	
   392		spin_lock_bh(&hsr->list_lock);
   393		if (!node_curr->removed) {
   394			list_del_rcu(&node_curr->mac_list);
   395			node_curr->removed = true;
   396			kfree_rcu(node_curr, rcu_head);
   397		}
   398		spin_unlock_bh(&hsr->list_lock);
   399	
   400	done:
   401		/* Push back here */
   402		skb_push(skb, total_pull_size);
   403	}
   404	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[kernel test robot]

Hi Lukasz,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]
[also build test ERROR on net/main linus/master v6.5-rc7 next-20230825]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lukasz-Majewski/net-hsr-Provide-fix-for-HSRv1-supervisor-frames-decoding/20230825-233423
base:   net-next/main
patch link:    https://lore.kernel.org/r/20230825153111.228768-1-lukma%40denx.de
patch subject: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
config: riscv-randconfig-001-20230826 (https://download.01.org/0day-ci/archive/20230826/202308260833.erhVKBnc-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230826/202308260833.erhVKBnc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308260833.erhVKBnc-lkp@intel.com/

All errors (new ones prefixed by >>):

   net/hsr/hsr_framereg.c: In function 'hsr_handle_sup_frame':
>> net/hsr/hsr_framereg.c:289:12: error: 'And' undeclared (first use in this function)
     289 |          * And leave the HSR tag.
         |            ^~~
   net/hsr/hsr_framereg.c:289:12: note: each undeclared identifier is reported only once for each function it appears in
>> net/hsr/hsr_framereg.c:289:15: error: expected ';' before 'leave'
     289 |          * And leave the HSR tag.
         |               ^~~~~~
         |               ;


vim +/And +289 net/hsr/hsr_framereg.c

   249	
   250	/* Use the Supervision frame's info about an eventual macaddress_B for merging
   251	 * nodes that has previously had their macaddress_B registered as a separate
   252	 * node.
   253	 */
   254	void hsr_handle_sup_frame(struct hsr_frame_info *frame)
   255	{
   256		struct hsr_node *node_curr = frame->node_src;
   257		struct hsr_port *port_rcv = frame->port_rcv;
   258		struct hsr_priv *hsr = port_rcv->hsr;
   259		struct hsr_sup_payload *hsr_sp;
   260		struct hsr_sup_tlv *hsr_sup_tlv;
   261		struct hsr_node *node_real;
   262		struct sk_buff *skb = NULL;
   263		struct list_head *node_db;
   264		struct ethhdr *ethhdr;
   265		int i;
   266		unsigned int pull_size = 0;
   267		unsigned int total_pull_size = 0;
   268	
   269		/* Here either frame->skb_hsr or frame->skb_prp should be
   270		 * valid as supervision frame always will have protocol
   271		 * header info.
   272		 */
   273		if (frame->skb_hsr)
   274			skb = frame->skb_hsr;
   275		else if (frame->skb_prp)
   276			skb = frame->skb_prp;
   277		else if (frame->skb_std)
   278			skb = frame->skb_std;
   279		if (!skb)
   280			return;
   281	
   282		/* Leave the ethernet header. */
   283		pull_size = sizeof(struct ethhdr);
   284		skb_pull(skb, pull_size);
   285		total_pull_size += pull_size;
   286	
   287		ethhdr = (struct ethhdr *)skb_mac_header(skb);
   288	
 > 289		 * And leave the HSR tag.
   290		 *
   291		 * The HSRv1 supervisory frame encapsulates the v0 frame
   292		 * with EtherType of 0x88FB
   293		 */
   294		if (ethhdr->h_proto == htons(ETH_P_HSR)) {
   295			if (hsr->prot_version == HSR_V1)
   296				/* In the above step the DA, SA and EtherType
   297				 * (0x892F - HSRv1) bytes has been removed.
   298				 *
   299				 * As the HSRv1 has the HSR header added, one need
   300				 * to remove path_and_LSDU_size and sequence_nr fields.
   301				 *
   302				 */
   303				pull_size = 4;
   304			else
   305				pull_size = sizeof(struct hsr_tag);
   306	
   307			skb_pull(skb, pull_size);
   308			total_pull_size += pull_size;
   309		}
   310	
   311		/* And leave the HSR sup tag. */
   312		pull_size = sizeof(struct hsr_tag);
   313		skb_pull(skb, pull_size);
   314		total_pull_size += pull_size;
   315	
   316		/* get HSR sup payload */
   317		if (hsr->prot_version == HSR_V1) {
   318			/* In the HSRv1 supervisor frame, when
   319			 * one with EtherType = 0x88FB is extracted, the Node A
   320			 * MAC address is preceded with type and length elements of TLV
   321			 * data field.
   322			 *
   323			 * It needs to be removed to get the remote peer MAC address.
   324			 */
   325			pull_size = sizeof(struct hsr_sup_tlv);
   326			skb_pull(skb, pull_size);
   327			total_pull_size += pull_size;
   328		}
   329	
   330		hsr_sp = (struct hsr_sup_payload *)skb->data;
   331	
   332		/* Merge node_curr (registered on macaddress_B) into node_real */
   333		node_db = &port_rcv->hsr->node_db;
   334		node_real = find_node_by_addr_A(node_db, hsr_sp->macaddress_A);
   335		if (!node_real)
   336			/* No frame received from AddrA of this node yet */
   337			node_real = hsr_add_node(hsr, node_db, hsr_sp->macaddress_A,
   338						 HSR_SEQNR_START - 1, true,
   339						 port_rcv->type);
   340		if (!node_real)
   341			goto done; /* No mem */
   342		if (node_real == node_curr)
   343			/* Node has already been merged */
   344			goto done;
   345	
   346		/* Leave the first HSR sup payload. */
   347		pull_size = sizeof(struct hsr_sup_payload);
   348		skb_pull(skb, pull_size);
   349		total_pull_size += pull_size;
   350	
   351		/* Get second supervision tlv */
   352		hsr_sup_tlv = (struct hsr_sup_tlv *)skb->data;
   353		/* And check if it is a redbox mac TLV */
   354		if (hsr_sup_tlv->HSR_TLV_type == PRP_TLV_REDBOX_MAC) {
   355			/* We could stop here after pushing hsr_sup_payload,
   356			 * or proceed and allow macaddress_B and for redboxes.
   357			 */
   358			/* Sanity check length */
   359			if (hsr_sup_tlv->HSR_TLV_length != 6)
   360				goto done;
   361	
   362			/* Leave the second HSR sup tlv. */
   363			pull_size = sizeof(struct hsr_sup_tlv);
   364			skb_pull(skb, pull_size);
   365			total_pull_size += pull_size;
   366	
   367			/* Get redbox mac address. */
   368			hsr_sp = (struct hsr_sup_payload *)skb->data;
   369	
   370			/* Check if redbox mac and node mac are equal. */
   371			if (!ether_addr_equal(node_real->macaddress_A, hsr_sp->macaddress_A)) {
   372				/* This is a redbox supervision frame for a VDAN! */
   373				goto done;
   374			}
   375		}
   376	
   377		ether_addr_copy(node_real->macaddress_B, ethhdr->h_source);
   378		spin_lock_bh(&node_real->seq_out_lock);
   379		for (i = 0; i < HSR_PT_PORTS; i++) {
   380			if (!node_curr->time_in_stale[i] &&
   381			    time_after(node_curr->time_in[i], node_real->time_in[i])) {
   382				node_real->time_in[i] = node_curr->time_in[i];
   383				node_real->time_in_stale[i] =
   384							node_curr->time_in_stale[i];
   385			}
   386			if (seq_nr_after(node_curr->seq_out[i], node_real->seq_out[i]))
   387				node_real->seq_out[i] = node_curr->seq_out[i];
   388		}
   389		spin_unlock_bh(&node_real->seq_out_lock);
   390		node_real->addr_B_port = port_rcv->type;
   391	
   392		spin_lock_bh(&hsr->list_lock);
   393		if (!node_curr->removed) {
   394			list_del_rcu(&node_curr->mac_list);
   395			node_curr->removed = true;
   396			kfree_rcu(node_curr, rcu_head);
   397		}
   398		spin_unlock_bh(&hsr->list_lock);
   399	
   400	done:
   401		/* Push back here */
   402		skb_push(skb, total_pull_size);
   403	}
   404	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 5975 bytes --]

Hi Tristram,

> > -       /* And leave the HSR tag. */
> > +        * And leave the HSR tag.
> > +        *
> > +        * The HSRv1 supervisory frame encapsulates the v0 frame
> > +        * with EtherType of 0x88FB
> > +        */
> >         if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> > -               pull_size = sizeof(struct ethhdr);
> > +               if (hsr->prot_version == HSR_V1)
> > +                       /* In the above step the DA, SA and
> > EtherType
> > +                        * (0x892F - HSRv1) bytes has been removed.
> > +                        *
> > +                        * As the HSRv1 has the HSR header added,
> > one need
> > +                        * to remove path_and_LSDU_size and
> > sequence_nr fields.
> > +                        *
> > +                        */
> > +                       pull_size = 4;
> > +               else
> > +                       pull_size = sizeof(struct hsr_tag);
> > +
> >                 skb_pull(skb, pull_size);
> >                 total_pull_size += pull_size;
> >         }
> > @@ -313,6 +328,19 @@ void hsr_handle_sup_frame(struct
> > hsr_frame_info *frame) total_pull_size += pull_size;
> > 
> >         /* get HSR sup payload */
> > +       if (hsr->prot_version == HSR_V1) {
> > +               /* In the HSRv1 supervisor frame, when
> > +                * one with EtherType = 0x88FB is extracted, the
> > Node A
> > +                * MAC address is preceded with type and length
> > elements of TLV
> > +                * data field.
> > +                *
> > +                * It needs to be removed to get the remote peer
> > MAC address.
> > +                */
> > +               pull_size = sizeof(struct hsr_sup_tlv);
> > +               skb_pull(skb, pull_size);
> > +               total_pull_size += pull_size;
> > +       }
> > +
> >         hsr_sp = (struct hsr_sup_payload *)skb->data;  
> 
> I thought the fix is simply this:
> 
> 	if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> -		pull_size = sizeof(struct ethhdr);
> +		pull_size = sizeof(struct hsr_tag);
> 		skb_pull(skb, pull_size);
> 		total_pull_size += pull_size;
> 	}
> 
> -	pull_size = sizeof(struct hsr_tag);
> +	pull_size = sizeof(struct hsr_sup_tag);
> 
> Note the sizes of hsr_tag and hsr_sup_tag are the same: 6 bytes.
> The code in 5.15 before this refactored code uses those structures.
> When using v0 the EtherType uses the PRP tag instead of the HSR tag so
> the HSR related code is not executed.
> 

This would not be enough it seems. Please find below skb->data dump
when entering hsr_handle_sup_frame() [0]:

SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34
SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000040: 00 00                                          

With the newest kernel (before applying this patch) in [1] we do remove:
01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f (which is equal to
sizeof(struct ethhdr) = 6 + 6 + 2 B = 14 B)

So we do have:

							  00 34
SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000040: 00 00

And we need to remove rest of the HSR v1 tag (4 Bytes).

Then we do have:

SKB_I100000010:       88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000040: 00 00

The 0x88FB is the PRP/HSRv0 supervisory frame ETH type, so the tag
needs to be removed (6 Bytes) and then we do have TYPE (0x17) and
Length (0x06), which indicate the other HSR host IP address.

When I do apply your proposed changes we would have the DA and SA
MAC addresses removed implicitly (as the struct hsr_tag and hsr_sup_tag
are 6 bytes in size) and we end up with frame starting with HSR v1 tag -
i.e.:

SKB_I100000000:                                     89 2f 00 34
SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000040: 00 00                                          


Hence mine question - is my setup or understanding wrong (as the PRP
supervisory frame is encapsulated in HSR v1 frame)? 

I do use the same kernel on two KSZ9477-EVB boards with Port[12]
connected together to work with HSR. I also to explicitly force the HSR
driver to use v1 of HSR (by default v0 is enforced).





If you don't mind - I would also like to ask a question regarding the
node_db for HSR.

Why the output of:

# cat /sys/kernel/debug/hsr/hsr0/node_table                 
Node Table entries for (HSR) device
MAC-Address-A,    MAC-Address-B,    time_in[A], time_in[B], 
00:10:a1:94:77:30 00:00:00:00:00:00    1689193,    1689199,

Address-B port, DAN-H
 	0,        1

Has the MAC-Address-B equal to 00:00:00:00:00:00 ?

As I do have the same MAC addresses for both HSR ports (to facilitate
frame duplication in KSZ9477 IC removal) I would expect to have this MAC
address set to 00:10:a1:94:77:30 as well...

Is this expected? Or is there any other issue to fix?


Thanks in advance for your help and support :-)

Links:

[0] -
https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L281

[1] -
https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L290

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 6659 bytes --]

Hi Tristram,

> Hi Tristram,
> 
> > > -       /* And leave the HSR tag. */
> > > +        * And leave the HSR tag.
> > > +        *
> > > +        * The HSRv1 supervisory frame encapsulates the v0 frame
> > > +        * with EtherType of 0x88FB
> > > +        */
> > >         if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> > > -               pull_size = sizeof(struct ethhdr);
> > > +               if (hsr->prot_version == HSR_V1)
> > > +                       /* In the above step the DA, SA and
> > > EtherType
> > > +                        * (0x892F - HSRv1) bytes has been
> > > removed.
> > > +                        *
> > > +                        * As the HSRv1 has the HSR header added,
> > > one need
> > > +                        * to remove path_and_LSDU_size and
> > > sequence_nr fields.
> > > +                        *
> > > +                        */
> > > +                       pull_size = 4;
> > > +               else
> > > +                       pull_size = sizeof(struct hsr_tag);
> > > +
> > >                 skb_pull(skb, pull_size);
> > >                 total_pull_size += pull_size;
> > >         }
> > > @@ -313,6 +328,19 @@ void hsr_handle_sup_frame(struct
> > > hsr_frame_info *frame) total_pull_size += pull_size;
> > > 
> > >         /* get HSR sup payload */
> > > +       if (hsr->prot_version == HSR_V1) {
> > > +               /* In the HSRv1 supervisor frame, when
> > > +                * one with EtherType = 0x88FB is extracted, the
> > > Node A
> > > +                * MAC address is preceded with type and length
> > > elements of TLV
> > > +                * data field.
> > > +                *
> > > +                * It needs to be removed to get the remote peer
> > > MAC address.
> > > +                */
> > > +               pull_size = sizeof(struct hsr_sup_tlv);
> > > +               skb_pull(skb, pull_size);
> > > +               total_pull_size += pull_size;
> > > +       }
> > > +
> > >         hsr_sp = (struct hsr_sup_payload *)skb->data;    
> > 
> > I thought the fix is simply this:
> > 
> > 	if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> > -		pull_size = sizeof(struct ethhdr);
> > +		pull_size = sizeof(struct hsr_tag);
> > 		skb_pull(skb, pull_size);
> > 		total_pull_size += pull_size;
> > 	}
> > 
> > -	pull_size = sizeof(struct hsr_tag);
> > +	pull_size = sizeof(struct hsr_sup_tag);
> > 
> > Note the sizes of hsr_tag and hsr_sup_tag are the same: 6 bytes.
> > The code in 5.15 before this refactored code uses those structures.
> > When using v0 the EtherType uses the PRP tag instead of the HSR tag
> > so the HSR related code is not executed.
> >   
> 
> This would not be enough it seems. Please find below skb->data dump
> when entering hsr_handle_sup_frame() [0]:
> 
> SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34
> SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00                                          
> 
> With the newest kernel (before applying this patch) in [1] we do
> remove: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f (which is equal to
> sizeof(struct ethhdr) = 6 + 6 + 2 B = 14 B)
> 
> So we do have:
> 
> 							  00 34
> SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00
> 
> And we need to remove rest of the HSR v1 tag (4 Bytes).
> 
> Then we do have:
> 
> SKB_I100000010:       88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00
> 
> The 0x88FB is the PRP/HSRv0 supervisory frame ETH type, so the tag
> needs to be removed (6 Bytes) and then we do have TYPE (0x17) and
> Length (0x06), which indicate the other HSR host IP address.
> 
> When I do apply your proposed changes we would have the DA and SA
> MAC addresses removed implicitly (as the struct hsr_tag and
> hsr_sup_tag are 6 bytes in size) and we end up with frame starting
> with HSR v1 tag - i.e.:
> 
> SKB_I100000000:                                     89 2f 00 34
> SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00                                          
> 
> 
> Hence mine question - is my setup or understanding wrong (as the PRP
> supervisory frame is encapsulated in HSR v1 frame)? 
> 
> I do use the same kernel on two KSZ9477-EVB boards with Port[12]
> connected together to work with HSR. I also to explicitly force the
> HSR driver to use v1 of HSR (by default v0 is enforced).
> 
> 
> 
> 
> 
> If you don't mind - I would also like to ask a question regarding the
> node_db for HSR.
> 
> Why the output of:
> 
> # cat /sys/kernel/debug/hsr/hsr0/node_table                 
> Node Table entries for (HSR) device
> MAC-Address-A,    MAC-Address-B,    time_in[A], time_in[B], 
> 00:10:a1:94:77:30 00:00:00:00:00:00    1689193,    1689199,
> 
> Address-B port, DAN-H
>  	0,        1
> 
> Has the MAC-Address-B equal to 00:00:00:00:00:00 ?
> 
> As I do have the same MAC addresses for both HSR ports (to facilitate
> frame duplication in KSZ9477 IC removal) I would expect to have this
> MAC address set to 00:10:a1:94:77:30 as well...
> 
> Is this expected? Or is there any other issue to fix?
> 

Tristram, do you have any feedback on those changes?

> 
> Thanks in advance for your help and support :-)
> 
> Links:
> 
> [0] -
> https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L281
> 
> [1] -
> https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L290
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma@denx.de




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 7874 bytes --]

Hi Tristram,

> Hi Tristram,
> 
> > > -       /* And leave the HSR tag. */
> > > +        * And leave the HSR tag.
> > > +        *
> > > +        * The HSRv1 supervisory frame encapsulates the v0 frame
> > > +        * with EtherType of 0x88FB
> > > +        */
> > >         if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> > > -               pull_size = sizeof(struct ethhdr);
> > > +               if (hsr->prot_version == HSR_V1)
> > > +                       /* In the above step the DA, SA and
> > > EtherType
> > > +                        * (0x892F - HSRv1) bytes has been
> > > removed.
> > > +                        *
> > > +                        * As the HSRv1 has the HSR header added,
> > > one need
> > > +                        * to remove path_and_LSDU_size and
> > > sequence_nr fields.
> > > +                        *
> > > +                        */
> > > +                       pull_size = 4;
> > > +               else
> > > +                       pull_size = sizeof(struct hsr_tag);
> > > +
> > >                 skb_pull(skb, pull_size);
> > >                 total_pull_size += pull_size;
> > >         }
> > > @@ -313,6 +328,19 @@ void hsr_handle_sup_frame(struct
> > > hsr_frame_info *frame) total_pull_size += pull_size;
> > > 
> > >         /* get HSR sup payload */
> > > +       if (hsr->prot_version == HSR_V1) {
> > > +               /* In the HSRv1 supervisor frame, when
> > > +                * one with EtherType = 0x88FB is extracted, the
> > > Node A
> > > +                * MAC address is preceded with type and length
> > > elements of TLV
> > > +                * data field.
> > > +                *
> > > +                * It needs to be removed to get the remote peer
> > > MAC address.
> > > +                */
> > > +               pull_size = sizeof(struct hsr_sup_tlv);
> > > +               skb_pull(skb, pull_size);
> > > +               total_pull_size += pull_size;
> > > +       }
> > > +
> > >         hsr_sp = (struct hsr_sup_payload *)skb->data;    
> > 
> > I thought the fix is simply this:
> > 
> > 	if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> > -		pull_size = sizeof(struct ethhdr);
> > +		pull_size = sizeof(struct hsr_tag);
> > 		skb_pull(skb, pull_size);
> > 		total_pull_size += pull_size;
> > 	}
> > 
> > -	pull_size = sizeof(struct hsr_tag);
> > +	pull_size = sizeof(struct hsr_sup_tag);
> > 
> > Note the sizes of hsr_tag and hsr_sup_tag are the same: 6 bytes.
> > The code in 5.15 before this refactored code uses those structures.
> > When using v0 the EtherType uses the PRP tag instead of the HSR tag
> > so the HSR related code is not executed.
> >   
> 
> This would not be enough it seems. Please find below skb->data dump
> when entering hsr_handle_sup_frame() [0]:
> 
> SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34
> SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00                                          
> 
> With the newest kernel (before applying this patch) in [1] we do
> remove: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f (which is equal to
> sizeof(struct ethhdr) = 6 + 6 + 2 B = 14 B)
> 
> So we do have:
> 
> 							  00 34
> SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00
> 
> And we need to remove rest of the HSR v1 tag (4 Bytes).
> 
> Then we do have:
> 
> SKB_I100000010:       88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00
> 
> The 0x88FB is the PRP/HSRv0 supervisory frame ETH type, so the tag
> needs to be removed (6 Bytes) and then we do have TYPE (0x17) and
> Length (0x06), which indicate the other HSR host IP address.
> 
> When I do apply your proposed changes we would have the DA and SA
> MAC addresses removed implicitly (as the struct hsr_tag and
> hsr_sup_tag are 6 bytes in size) and we end up with frame starting
> with HSR v1 tag - i.e.:
> 
> SKB_I100000000:                                     89 2f 00 34
> SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00                                          
> 
> 
> Hence mine question - is my setup or understanding wrong (as the PRP
> supervisory frame is encapsulated in HSR v1 frame)? 
> 
> I do use the same kernel on two KSZ9477-EVB boards with Port[12]
> connected together to work with HSR. I also to explicitly force the
> HSR driver to use v1 of HSR (by default v0 is enforced).
> 
> 

As I've double checked with tshark - the supervision frame format is
different for HSR v0 and v1:

HSR v0:
    [Protocols in frame:eth:ethertype:hsr_prp_supervision]

HSR v1:
    [Protocols in frame: eth:ethertype:hsr:hsr_prp_supervision]

In v0 the ":hsr:" is missing, hence the error.

> 
> 
> 
> If you don't mind - I would also like to ask a question regarding the
> node_db for HSR.
> 
> Why the output of:
> 
> # cat /sys/kernel/debug/hsr/hsr0/node_table                 
> Node Table entries for (HSR) device
> MAC-Address-A,    MAC-Address-B,    time_in[A], time_in[B], 
> 00:10:a1:94:77:30 00:00:00:00:00:00    1689193,    1689199,
> 
> Address-B port, DAN-H
>  	0,        1
> 
> Has the MAC-Address-B equal to 00:00:00:00:00:00 ?
> 
> As I do have the same MAC addresses for both HSR ports (to facilitate
> frame duplication in KSZ9477 IC removal) I would expect to have this
> MAC address set to 00:10:a1:94:77:30 as well...
> 
> Is this expected? Or is there any other issue to fix?

I think that this is caused by how KSZ9477 works with HSR core.

The HSR core is responsible for setting tag for frame. It sets the
"Lane ID" part of HSR tag to 0 as it relies on HSR frames duplication
in KSZ9477:

    Type: High-availability Seamless Redundancy (IEC62439 Part 3)
    (0x892f) 
    High-availability Seamless Redundancy (IEC62439 Part 3
    Chapter 5) 0000 .... .... .... = Path: 0
    000. .... .... .... = Network id: 0
    ...0 .... .... .... = Lane id: Lane A (0)
    .... 0000 0011 0100 = LSDU size: 52 [correct]

As KSZ9477 duplicates (clones) frames without any modification, only
frames for Lane id = 0 are sent. Hence the MAC-Address-B is always
equal to 00:00:00:00:00:00 as no supervisory frames are received with
Lane B id.

This however, may be different in other HSR switch IC's - especially in
those, which insert the HSR header to frames - as they know to which
HSR "lane" (egress port) the frame is delivered.


> 
> 
> Thanks in advance for your help and support :-)
> 
> Links:
> 
> [0] -
> https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L281
> 
> [1] -
> https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L290
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma@denx.de




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[Sebastian Andrzej Siewior]

On 2023-08-25 17:31:11 [+0200], Lukasz Majewski wrote:
> Provide fix to decode correctly supervisory frames when HSRv1 version of
> the HSR protocol is used.
> 
> Without this patch console is polluted with:
> ksz-switch spi1.0 lan1: hsr_addr_subst_dest: Unknown node
> 
> as a result of destination node's A MAC address equals to:
> 00:00:00:00:00:00.
> 
> cat /sys/kernel/debug/hsr/hsr0/node_table
> Node Table entries for (HSR) device
> MAC-Address-A,    MAC-Address-B,    time_in[A], time_in[B], Address-B
> 00:00:00:00:00:00 00:10:a1:94:77:30      400bf,       399c,	        0
> 
> It was caused by wrong frames decoding in the hsr_handle_sup_frame().
> 
> As the supervisor frame is encapsulated in HSRv1 frame:
> 
> SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34
> SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00
> 
> The code had to be adjusted accordingly and the MAC-Address-A now
> has the proper address (the MAC-Address-B now has all 0's).

Was this broken by commit
	eafaa88b3eb7f ("net: hsr: Add support for redbox supervision frames")

? Is this frame somehow special? I don't remember this…

> Signed-off-by: Lukasz Majewski <lukma@denx.de>

Sebastian

Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]

Hi Sebastian,

> On 2023-08-25 17:31:11 [+0200], Lukasz Majewski wrote:
> > Provide fix to decode correctly supervisory frames when HSRv1
> > version of the HSR protocol is used.
> > 
> > Without this patch console is polluted with:
> > ksz-switch spi1.0 lan1: hsr_addr_subst_dest: Unknown node
> > 
> > as a result of destination node's A MAC address equals to:
> > 00:00:00:00:00:00.
> > 
> > cat /sys/kernel/debug/hsr/hsr0/node_table
> > Node Table entries for (HSR) device
> > MAC-Address-A,    MAC-Address-B,    time_in[A], time_in[B],
> > Address-B 00:00:00:00:00:00 00:10:a1:94:77:30      400bf,
> > 399c,	        0
> > 
> > It was caused by wrong frames decoding in the
> > hsr_handle_sup_frame().
> > 
> > As the supervisor frame is encapsulated in HSRv1 frame:
> > 
> > SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34
> > SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > SKB_I100000040: 00 00
> > 
> > The code had to be adjusted accordingly and the MAC-Address-A now
> > has the proper address (the MAC-Address-B now has all 0's).  
> 
> Was this broken by commit
> 	eafaa88b3eb7f ("net: hsr: Add support for redbox supervision
> frames")
> 

Yes, it seems so.

> ? Is this frame somehow special? I don't remember this…
> 

Please refer to the whole thread - I've described this issue thoroughly
(including hex dump of frames):
https://lore.kernel.org/lkml/20230904175419.7bed196b@wsk/T/#m35cbfa4f1b8901d341fbc39659ace6a041f84c98

In short - the HSRv1 is not recognized correctly anymore:

HSR v0:
    [Protocols in frame: eth:ethertype:hsr_prp_supervision]
                                                                        
HSR v1:
    [Protocols in frame: eth:ethertype:hsr:hsr_prp_supervision]


> > Signed-off-by: Lukasz Majewski <lukma@denx.de>  
> 
> Sebastian


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2794 bytes --]

Hi Sebastian,

> Hi Sebastian,
> 
> > On 2023-08-25 17:31:11 [+0200], Lukasz Majewski wrote:  
> > > Provide fix to decode correctly supervisory frames when HSRv1
> > > version of the HSR protocol is used.
> > > 
> > > Without this patch console is polluted with:
> > > ksz-switch spi1.0 lan1: hsr_addr_subst_dest: Unknown node
> > > 
> > > as a result of destination node's A MAC address equals to:
> > > 00:00:00:00:00:00.
> > > 
> > > cat /sys/kernel/debug/hsr/hsr0/node_table
> > > Node Table entries for (HSR) device
> > > MAC-Address-A,    MAC-Address-B,    time_in[A], time_in[B],
> > > Address-B 00:00:00:00:00:00 00:10:a1:94:77:30      400bf,
> > > 399c,	        0
> > > 
> > > It was caused by wrong frames decoding in the
> > > hsr_handle_sup_frame().
> > > 
> > > As the supervisor frame is encapsulated in HSRv1 frame:
> > > 
> > > SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34
> > > SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> > > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > SKB_I100000040: 00 00
> > > 
> > > The code had to be adjusted accordingly and the MAC-Address-A now
> > > has the proper address (the MAC-Address-B now has all 0's).    
> > 
> > Was this broken by commit
> > 	eafaa88b3eb7f ("net: hsr: Add support for redbox supervision
> > frames")
> >   
> 
> Yes, it seems so.
> 
> > ? Is this frame somehow special? I don't remember this…
> >   
> 
> Please refer to the whole thread - I've described this issue
> thoroughly (including hex dump of frames):
> https://lore.kernel.org/lkml/20230904175419.7bed196b@wsk/T/#m35cbfa4f1b8901d341fbc39659ace6a041f84c98
> 
> In short - the HSRv1 is not recognized correctly anymore:
> 
> HSR v0:
>     [Protocols in frame: eth:ethertype:hsr_prp_supervision]
>                                                                         
> HSR v1:
>     [Protocols in frame: eth:ethertype:hsr:hsr_prp_supervision]
> 

Have you had time to review this patch?

Your comments are more than welcome.

> 
> > > Signed-off-by: Lukasz Majewski <lukma@denx.de>    
> > 
> > Sebastian  
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma@denx.de




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[Sebastian Andrzej Siewior]

On 2023-09-11 16:57:08 [+0200], Lukasz Majewski wrote:
> Hi Sebastian,
Hi,

> 
> Have you had time to review this patch?

got distracted a few times. I need a quiet moment… Will do this week…

> Your comments are more than welcome.

Sebastian

Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

Hi Sebastian,

> On 2023-09-11 16:57:08 [+0200], Lukasz Majewski wrote:
> > Hi Sebastian,  
> Hi,
> 
> > 
> > Have you had time to review this patch?  
> 
> got distracted a few times. I need a quiet moment… Will do this week…
> 

Ok. No problem. Thanks for the information.

> > Your comments are more than welcome.  
> 
> Sebastian




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[Sebastian Andrzej Siewior]

On 2023-09-12 10:18:28 [+0200], Lukasz Majewski wrote:
> Hi Sebastian,
Hi Lukasz,

> Ok. No problem. Thanks for the information.

So what happens if you try this:

diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
index b77f1189d19d1..6d14d935ee828 100644
--- a/net/hsr/hsr_framereg.c
+++ b/net/hsr/hsr_framereg.c
@@ -288,13 +288,13 @@ void hsr_handle_sup_frame(struct hsr_frame_info *frame)
 
 	/* And leave the HSR tag. */
 	if (ethhdr->h_proto == htons(ETH_P_HSR)) {
-		pull_size = sizeof(struct ethhdr);
+		pull_size = sizeof(struct hsr_tag);
 		skb_pull(skb, pull_size);
 		total_pull_size += pull_size;
 	}
 
 	/* And leave the HSR sup tag. */
-	pull_size = sizeof(struct hsr_tag);
+	pull_size = sizeof(struct hsr_sup_tag);
 	skb_pull(skb, pull_size);
 	total_pull_size += pull_size;
 

Sebastian

Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

Hi Sebastian Andrzej,

> On 2023-09-12 10:18:28 [+0200], Lukasz Majewski wrote:
> > Hi Sebastian,  
> Hi Lukasz,
> 
> > Ok. No problem. Thanks for the information.  
> 
> So what happens if you try this:
> 
> diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
> index b77f1189d19d1..6d14d935ee828 100644
> --- a/net/hsr/hsr_framereg.c
> +++ b/net/hsr/hsr_framereg.c
> @@ -288,13 +288,13 @@ void hsr_handle_sup_frame(struct hsr_frame_info
> *frame) 
>  	/* And leave the HSR tag. */
>  	if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> -		pull_size = sizeof(struct ethhdr);
> +		pull_size = sizeof(struct hsr_tag);
>  		skb_pull(skb, pull_size);
>  		total_pull_size += pull_size;
>  	}
>  
>  	/* And leave the HSR sup tag. */
> -	pull_size = sizeof(struct hsr_tag);
> +	pull_size = sizeof(struct hsr_sup_tag);
>  	skb_pull(skb, pull_size);
>  	total_pull_size += pull_size;
>  
> 
> Sebastian

Yes, this fixes this issue (caused by: SHA1: eafaa88b3eb7f).
Such solution has also been pointed out earlier by Tristram.

I will prepare v2 of this patch.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[Sebastian Andrzej Siewior]

On 2023-09-14 14:26:50 [+0200], Lukasz Majewski wrote:
> Hi Sebastian Andrzej,
Hi,

> 
> Yes, this fixes this issue (caused by: SHA1: eafaa88b3eb7f).
> Such solution has also been pointed out earlier by Tristram.

Right, now that I looked at it…

> I will prepare v2 of this patch.

don't worry, I take care of this.

> Best regards,
> 
> Lukasz Majewski

Sebastian