Re: [PATCH net-next 1/1] net: hsr/prp: add vlan support

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Tue, Sep 8, 2020 at 6:39 PM Murali Karicheri <m-karicheri2@ti.com> wrote:
>
> Hi Willem,
>
> Thanks for the response!
> On 9/4/20 11:45 AM, Willem de Bruijn wrote:
> > On Tue, Sep 1, 2020 at 9:54 PM Murali Karicheri <m-karicheri2@ti.com> wrote:
> >>
> >> This patch add support for creating vlan interfaces
> >> over hsr/prp interface.
> >>
> >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> >> ---
> >>   net/hsr/hsr_device.c  |  4 ----
> >>   net/hsr/hsr_forward.c | 16 +++++++++++++---
> >>   2 files changed, 13 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> >> index ab953a1a0d6c..e1951579a3ad 100644
> >> --- a/net/hsr/hsr_device.c
> >> +++ b/net/hsr/hsr_device.c
> >> @@ -477,10 +477,6 @@ void hsr_dev_setup(struct net_device *dev)
> >>
> >>          /* Prevent recursive tx locking */
> >>          dev->features |= NETIF_F_LLTX;
> >> -       /* VLAN on top of HSR needs testing and probably some work on
> >> -        * hsr_header_create() etc.
> >> -        */
> >> -       dev->features |= NETIF_F_VLAN_CHALLENGED;
> >>          /* Not sure about this. Taken from bridge code. netdev_features.h says
> >>           * it means "Does not change network namespaces".
> >>           */
> >> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> >> index cadfccd7876e..de21df30b0d9 100644
> >> --- a/net/hsr/hsr_forward.c
> >> +++ b/net/hsr/hsr_forward.c
> >> @@ -208,6 +208,7 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
> >>                                      struct hsr_port *port, u8 proto_version)
> >>   {
> >>          struct hsr_ethhdr *hsr_ethhdr;
> >> +       unsigned char *pc;
> >>          int lsdu_size;
> >>
> >>          /* pad to minimum packet size which is 60 + 6 (HSR tag) */
> >> @@ -218,7 +219,18 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
> >>          if (frame->is_vlan)
> >>                  lsdu_size -= 4;
> >>
> >> -       hsr_ethhdr = (struct hsr_ethhdr *)skb_mac_header(skb);
> >> +       pc = skb_mac_header(skb);
> >> +       if (frame->is_vlan)
> >> +               /* This 4-byte shift (size of a vlan tag) does not
> >> +                * mean that the ethhdr starts there. But rather it
> >> +                * provides the proper environment for accessing
> >> +                * the fields, such as hsr_tag etc., just like
> >> +                * when the vlan tag is not there. This is because
> >> +                * the hsr tag is after the vlan tag.
> >> +                */
> >> +               hsr_ethhdr = (struct hsr_ethhdr *)(pc + VLAN_HLEN);
> >> +       else
> >> +               hsr_ethhdr = (struct hsr_ethhdr *)pc;
> >
> > Instead, I would pass the header from the caller, which knows the
> > offset because it moves the previous headers to make space.
> >
> So if I understood you correctly a diff for the above would like this
> where pass dst + movelen as  struct hsr_ethhdr *hsr_ethhdr
> pointer to hsr_fill_tag(), right?
>
> a0868495local@uda0868495:~/Projects/upstream-kernel$ git diff
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index de21df30b0d9..4d9192c8bcf8 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -204,11 +204,10 @@ static void hsr_set_path_id(struct hsr_ethhdr
> *hsr_ethhdr,
>   }
>
>   static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
> +                                   struct hsr_ethhdr *hsr_ethhdr,
>                                      struct hsr_frame_info *frame,
>                                      struct hsr_port *port, u8
> proto_version)
>   {
> -       struct hsr_ethhdr *hsr_ethhdr;
> -       unsigned char *pc;
>          int lsdu_size;
>
>          /* pad to minimum packet size which is 60 + 6 (HSR tag) */
> @@ -219,19 +218,6 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff
> *skb,
>          if (frame->is_vlan)
>                  lsdu_size -= 4;
>
> -       pc = skb_mac_header(skb);
> -       if (frame->is_vlan)
> -               /* This 4-byte shift (size of a vlan tag) does not
> -                * mean that the ethhdr starts there. But rather it
> -                * provides the proper environment for accessing
> -                * the fields, such as hsr_tag etc., just like
> -                * when the vlan tag is not there. This is because
> -                * the hsr tag is after the vlan tag.
> -                */
> -               hsr_ethhdr = (struct hsr_ethhdr *)(pc + VLAN_HLEN);
> -       else
> -               hsr_ethhdr = (struct hsr_ethhdr *)pc;
> -
>          hsr_set_path_id(hsr_ethhdr, port);
>          set_hsr_tag_LSDU_size(&hsr_ethhdr->hsr_tag, lsdu_size);
>          hsr_ethhdr->hsr_tag.sequence_nr = htons(frame->sequence_nr);
> @@ -280,10 +266,12 @@ struct sk_buff *hsr_create_tagged_frame(struct
> hsr_frame_info *frame,
>          memmove(dst, src, movelen);
>          skb_reset_mac_header(skb);
>
> -       /* skb_put_padto free skb on error and hsr_fill_tag returns NULL in
> -        * that case
> +       /* dst point to the start of hsr tag. So pass it to fill the
> +        * hsr info. Also skb_put_padto free skb on error and hsr_fill_tag
> +        * returns NULL in that case.
>           */
> -       return hsr_fill_tag(skb, frame, port, port->hsr->prot_version);
> +       return hsr_fill_tag(skb, (struct hsr_ethhdr *)(dst + movelen),
> +                           frame, port, port->hsr->prot_version);

It's a bit hard to see, since this is a draft patch on top of the
existing patch. But I think so.

Only, instead of dst + movelen, you can use src.

>   }
>
>
> > Also, supporting VLAN probably also requires supporting 802.1ad QinQ,
> > which means code should parse the headers instead of hardcoding
> > VLAN_HLEN.
> >
>
> iec-62439-3 standard only talks about VLAN (TPID 0x8100), not about
> QinQ. So what I could do is to check and bail out if 802.1ad frame is
> received at the interface from upper layer. Something like below and
> frame will get dropped.
>
> @@ -519,6 +507,8 @@ static int fill_frame_info(struct hsr_frame_info *frame,
>
>          if (proto == htons(ETH_P_8021Q))
>                  frame->is_vlan = true;
> +       else if (proto == htons(ETH_P_8021AD))
> +               return -1; /* Don't support 802.1ad */
>
>          if (frame->is_vlan) {
>                  vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
>
> What do you think?

It's good to err on the safe side. That should probably be a separate patch.