lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 8 Sep 2020 12:38:14 -0400
From:   Murali Karicheri <m-karicheri2@...com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
CC:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>, <nsekhar@...com>,
        Grygorii Strashko <grygorii.strashko@...com>
Subject: Re: [PATCH net-next 1/1] net: hsr/prp: add vlan support

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@...com> wrote:
>>
>> This patch add support for creating vlan interfaces
>> over hsr/prp interface.
>>
>> Signed-off-by: Murali Karicheri <m-karicheri2@...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@...0868495:~/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);
  }


> 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?

Thanks.
>>          hsr_set_path_id(hsr_ethhdr, port);
>>          set_hsr_tag_LSDU_size(&hsr_ethhdr->hsr_tag, lsdu_size);
>> @@ -511,8 +523,6 @@ static int fill_frame_info(struct hsr_frame_info *frame,
>>          if (frame->is_vlan) {
>>                  vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
>>                  proto = vlan_hdr->vlanhdr.h_vlan_encapsulated_proto;
>> -               /* FIXME: */
>> -               netdev_warn_once(skb->dev, "VLAN not yet supported");
>>          }
>>
>>          frame->is_from_san = false;
>> --
>> 2.17.1
>>

-- 
Murali Karicheri
Texas Instruments

Powered by blists - more mailing lists