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

From: Lukasz Majewski
Date: Thu Aug 31 2023 - 09:38:44 EST

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@xxxxxxx




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@xxxxxxx

Attachment: pgpdD2FMGyvOP.pgp
Description: OpenPGP digital signature