diff mbox series

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

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

Commit Message

Jan Scheurich Nov. 6, 2017, 11:24 p.m. UTC 
- Fix 2 incorrect length checks
- Remove unnecessary limit of MD length to 16 bytes
- Remove incorrect comments stating MD format 2 was not supported
- Pad metadata in encap_nsh with zeroes to 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. 7, 2017, 12:22 a.m. UTC | #1 
On Mon, Nov 06, 2017 at 11:24:54PM +0000, Jan Scheurich wrote:
> - Fix 2 incorrect length checks
> - Remove unnecessary limit of MD length to 16 bytes
> - Remove incorrect comments stating MD format 2 was not supported
> - Pad metadata in encap_nsh with zeroes to multiple of 4 bytes
> 
> Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com>

Thanks for the bug fixes.

Instead of using ROUND_UP and manual arithmetic to compute a pad size, I
suggest using the PAD_SIZE macro.

Otherwise, this looks good to me.

Is this suitable for backporting to branch-2.8?

Thanks,

Ben.
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..66ecc61 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 = ROUND_UP(mdlen, 4) - mdlen;
+                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: