diff mbox series

[ovs-dev,v2,1/2] NSH: Minor bugfixes

Message ID CFF8EF42F1132E4CBE2BF0AB6C21C58D852C3BFE@ESESSMB107.ericsson.se
State Not Applicable
Headers show
Series : NSH: Align with latest IETF draft and fix some bugs | expand

Commit Message

Jan Scheurich Nov. 7, 2017, 3:34 p.m. UTC 
- Fix 2 incorrect length checks
- Remove unnecessary limit of MD length to 16 bytes
- Remove incorrect comments stating MD2 was not supported
- Pad metadata in encap_nsh with zeroes if not multiple of 4 bytes

Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com>
---
 datapath/linux/compat/include/linux/openvswitch.h |  6 +++---
 lib/flow.c                                        | 13 ++++++-------
 lib/odp-util.c                                    |  9 +++++++--
 lib/packets.c                                     |  3 +--
 4 files changed, 17 insertions(+), 14 deletions(-)

--
1.9.1

Comments

Ben Pfaff Nov. 8, 2017, 4:27 a.m. UTC | #1 
On Tue, Nov 07, 2017 at 03:34:06PM +0000, Jan Scheurich wrote:
> - Fix 2 incorrect length checks
> - Remove unnecessary limit of MD length to 16 bytes
> - Remove incorrect comments stating MD2 was not supported
> - Pad metadata in encap_nsh with zeroes if not multiple of 4 bytes
> 
> Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com>

There's something odd about this patch and it does not apply:

    Using index info to reconstruct a base tree...
    error: patch failed: datapath/linux/compat/include/linux/openvswitch.h:793
    error: datapath/linux/compat/include/linux/openvswitch.h: patch does not apply
    error: Did you hand edit your patch?
    It does not apply to blobs recorded in its index.

I've never seen that particular error before.
Jan Scheurich Nov. 8, 2017, 8:48 a.m. UTC | #2 
Hi Ben,

I think the first patch does not apply because somehow the email process has converted the leading tab to 7 spaces in line 24 of the patch. 

Unfortunately I can't use git send-email in our company network due to firewalls restrictions. Could you try to use the attached patch files instead?

BR, Jan

> -----Original Message-----
> From: Ben Pfaff [mailto:blp@ovn.org]
> Sent: Wednesday, 08 November, 2017 05:27
> To: Jan Scheurich <jan.scheurich@ericsson.com>
> Cc: 'dev@openvswitch.org' <dev@openvswitch.org>; Yang, Yi Y (yi.y.yang@intel.com) <yi.y.yang@intel.com>; Jiri Benc
> (jbenc@redhat.com) <jbenc@redhat.com>
> Subject: Re: [PATCH v2 1/2] NSH: Minor bugfixes
> 
> On Tue, Nov 07, 2017 at 03:34:06PM +0000, Jan Scheurich wrote:
> > - Fix 2 incorrect length checks
> > - Remove unnecessary limit of MD length to 16 bytes
> > - Remove incorrect comments stating MD2 was not supported
> > - Pad metadata in encap_nsh with zeroes if not multiple of 4 bytes
> >
> > Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com>
> 
> There's something odd about this patch and it does not apply:
> 
>     Using index info to reconstruct a base tree...
>     error: patch failed: datapath/linux/compat/include/linux/openvswitch.h:793
>     error: datapath/linux/compat/include/linux/openvswitch.h: patch does not apply
>     error: Did you hand edit your patch?
>     It does not apply to blobs recorded in its index.
> 
> I've never seen that particular error before.
Ben Pfaff Nov. 8, 2017, 8:43 p.m. UTC | #3 
The attachments worked.  I applied them to master and branch-2.8.  I
added a NEWS entry:

   - NSH implementation now conforms to latest draft
     (draft-ietf-sfc-nsh-28).

Thank you!

On Wed, Nov 08, 2017 at 08:48:53AM +0000, Jan Scheurich wrote:
> Hi Ben,
> 
> I think the first patch does not apply because somehow the email process has converted the leading tab to 7 spaces in line 24 of the patch. 
> 
> Unfortunately I can't use git send-email in our company network due to firewalls restrictions. Could you try to use the attached patch files instead?
> 
> BR, Jan
> 
> > -----Original Message-----
> > From: Ben Pfaff [mailto:blp@ovn.org]
> > Sent: Wednesday, 08 November, 2017 05:27
> > To: Jan Scheurich <jan.scheurich@ericsson.com>
> > Cc: 'dev@openvswitch.org' <dev@openvswitch.org>; Yang, Yi Y (yi.y.yang@intel.com) <yi.y.yang@intel.com>; Jiri Benc
> > (jbenc@redhat.com) <jbenc@redhat.com>
> > Subject: Re: [PATCH v2 1/2] NSH: Minor bugfixes
> > 
> > On Tue, Nov 07, 2017 at 03:34:06PM +0000, Jan Scheurich wrote:
> > > - Fix 2 incorrect length checks
> > > - Remove unnecessary limit of MD length to 16 bytes
> > > - Remove incorrect comments stating MD2 was not supported
> > > - Pad metadata in encap_nsh with zeroes if not multiple of 4 bytes
> > >
> > > Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com>
> > 
> > There's something odd about this patch and it does not apply:
> > 
> >     Using index info to reconstruct a base tree...
> >     error: patch failed: datapath/linux/compat/include/linux/openvswitch.h:793
> >     error: datapath/linux/compat/include/linux/openvswitch.h: patch does not apply
> >     error: Did you hand edit your patch?
> >     It does not apply to blobs recorded in its index.
> > 
> > I've never seen that particular error before.
Jan Scheurich Nov. 8, 2017, 8:44 p.m. UTC | #4 
Thanks a lot!
/Jan

> -----Original Message-----
> From: Ben Pfaff [mailto:blp@ovn.org]
> Sent: Wednesday, 08 November, 2017 21:43
> To: Jan Scheurich <jan.scheurich@ericsson.com>
> Cc: 'dev@openvswitch.org' <dev@openvswitch.org>
> Subject: Re: [PATCH v2 1/2] NSH: Minor bugfixes
> 
> The attachments worked.  I applied them to master and branch-2.8.  I
> added a NEWS entry:
> 
>    - NSH implementation now conforms to latest draft
>      (draft-ietf-sfc-nsh-28).
> 
> Thank you!
> 
> On Wed, Nov 08, 2017 at 08:48:53AM +0000, Jan Scheurich wrote:
> > Hi Ben,
> >
> > I think the first patch does not apply because somehow the email process has converted the leading tab to 7 spaces in line 24 of the
> patch.
> >
> > Unfortunately I can't use git send-email in our company network due to firewalls restrictions. Could you try to use the attached patch
> files instead?
> >
> > BR, Jan
> >
> > > -----Original Message-----
> > > From: Ben Pfaff [mailto:blp@ovn.org]
> > > Sent: Wednesday, 08 November, 2017 05:27
> > > To: Jan Scheurich <jan.scheurich@ericsson.com>
> > > Cc: 'dev@openvswitch.org' <dev@openvswitch.org>; Yang, Yi Y (yi.y.yang@intel.com) <yi.y.yang@intel.com>; Jiri Benc
> > > (jbenc@redhat.com) <jbenc@redhat.com>
> > > Subject: Re: [PATCH v2 1/2] NSH: Minor bugfixes
> > >
> > > On Tue, Nov 07, 2017 at 03:34:06PM +0000, Jan Scheurich wrote:
> > > > - Fix 2 incorrect length checks
> > > > - Remove unnecessary limit of MD length to 16 bytes
> > > > - Remove incorrect comments stating MD2 was not supported
> > > > - Pad metadata in encap_nsh with zeroes if not multiple of 4 bytes
> > > >
> > > > Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com>
> > >
> > > There's something odd about this patch and it does not apply:
> > >
> > >     Using index info to reconstruct a base tree...
> > >     error: patch failed: datapath/linux/compat/include/linux/openvswitch.h:793
> > >     error: datapath/linux/compat/include/linux/openvswitch.h: patch does not apply
> > >     error: Did you hand edit your patch?
> > >     It does not apply to blobs recorded in its index.
> > >
> > > I've never seen that particular error before.
diff mbox series

Patch

diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
index bc6c94b..561f895 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -793,15 +793,15 @@  struct ovs_action_push_eth {
        struct ovs_key_ethernet addresses;
 };

-#define OVS_ENCAP_NSH_MAX_MD_LEN 16
+#define OVS_ENCAP_NSH_MAX_MD_LEN 248
 /*
  * struct ovs_action_encap_nsh - %OVS_ACTION_ATTR_ENCAP_NSH
  * @flags: NSH header flags.
  * @mdtype: NSH metadata type.
- * @mdlen: Length of NSH metadata in bytes.
+ * @mdlen: Length of NSH metadata in bytes, including padding.
  * @np: NSH next_protocol: Inner packet type.
  * @path_hdr: NSH service path id and service index.
- * @metadata: NSH metadata for MD type 1 or 2
+ * @metadata: NSH context metadata, padded to 4-bytes
  */
 struct ovs_action_encap_nsh {
     uint8_t flags;
diff --git a/lib/flow.c b/lib/flow.c
index 4d2b774..e30ca98 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -540,7 +540,7 @@  parse_nsh(const void **datap, size_t *sizep, struct flow_nsh *key)
     /* Check if it is long enough for NSH header, doesn't support
      * MD type 2 yet
      */
-    if (OVS_UNLIKELY(*sizep < NSH_M_TYPE1_LEN)) {
+    if (OVS_UNLIKELY(*sizep < NSH_BASE_HDR_LEN)) {
         return false;
     }

@@ -557,10 +557,6 @@  parse_nsh(const void **datap, size_t *sizep, struct flow_nsh *key)
         return false;
     }

-    if (length != NSH_M_TYPE1_LEN) {
-        return false;
-    }
-
     key->flags = flags;
     key->mdtype = nsh->md_type;
     key->np = nsh->next_proto;
@@ -571,14 +567,17 @@  parse_nsh(const void **datap, size_t *sizep, struct flow_nsh *key)

     switch (key->mdtype) {
         case NSH_M_TYPE1:
+            if (length != NSH_M_TYPE1_LEN) {
+                return false;
+            }
             for (size_t i = 0; i < 4; i++) {
                 key->c[i] = get_16aligned_be32(&nsh->md1.c[i]);
             }
             break;
         case NSH_M_TYPE2:
-            /* Don't support MD type 2 yet, so return false */
         default:
-            return false;
+            /* We don't parse other context headers yet. */
+            break;
     }

     data_pull(datap, sizep, length);
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 6304b3d..d8daede 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1864,12 +1864,17 @@  parse_odp_encap_nsh_action(const char *s, struct ofpbuf *actions)
         else if (encap_nsh.mdtype == NSH_M_TYPE2) {
             struct ofpbuf b;
             char buf[512];
-            size_t mdlen;
+            size_t mdlen, padding;
             if (ovs_scan_len(s, &n, "md2=0x%511[0-9a-fA-F]", buf)) {
                 ofpbuf_use_stub(&b, encap_nsh.metadata,
                                 OVS_ENCAP_NSH_MAX_MD_LEN);
                 ofpbuf_put_hex(&b, buf, &mdlen);
-                encap_nsh.mdlen = mdlen;
+                /* Pad metadata to 4 bytes. */
+                padding = PAD_SIZE(mdlen, 4);
+                if (padding > 0) {
+                    ofpbuf_push_zeros(&b, padding);
+                }
+                encap_nsh.mdlen = mdlen + padding;
                 ofpbuf_uninit(&b);
             }
             continue;
diff --git a/lib/packets.c b/lib/packets.c
index 51044df..c991e9f 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -439,8 +439,7 @@  encap_nsh(struct dp_packet *packet, const struct ovs_action_encap_nsh *encap)
             break;
         case NSH_M_TYPE2: {
             /* The MD2 metadata in encap is already padded to 4 bytes. */
-            size_t len = ROUND_UP(encap->mdlen, 4);
-            memcpy(&nsh->md2, encap->metadata, len);
+            memcpy(&nsh->md2, encap->metadata, encap->mdlen);
             break;
         }
         default: