[ovs-dev] ofproto-dpif: Fix for recirc issue with mpls traffic with dp_hash
diff mbox series

Message ID 1563514519-11061-1-git-send-email-rudrasurya.r@altencalsoftlabs.com
State New
Headers show
Series
  • [ovs-dev] ofproto-dpif: Fix for recirc issue with mpls traffic with dp_hash
Related show

Commit Message

Vishal Deep Ajmera via dev July 19, 2019, 5:35 a.m. UTC
Fix infinite recirculation loop for MPLS packets sent to dp_hash-based
 select group

Issue:
When a MPLS encapsulated packet is received, the MPLS header is removed,
a recirculation id assigned and then recirculated into the pipeline.
If the flow rules require the packet to be then sent over DP-HASH based
select group buckets, the packet has to be recirculated again. However,
the same recirculation id was used and this resulted in the packet being
repeatedly recirculated until it got dropped because the maximum recirculation
limit was hit.

Fix:
Include the  “was_mpls” boolean which indicates whether the packet was MPLS
encapsulated when computing the hash. After popping the MPLS header this will
result in a  different hash value than before and new recirculation id will
get generated.

DPCTL flows with and without the fix are shown below
Without Fix:
recirc_id(0x1),dp_hash(0x5194bf18/0xf),in_port(2),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:20, bytes:1960,
used:0.329s, actions:1
recirc_id(0x1),in_port(2),packet_type(ns=0,id=0),eth_type(0x0800),
ipv4(frag=no), packets:20, bytes:1960, used:0.329s,
actions:hash(sym_l4(0)),recirc(0x1)
recirc_id(0),in_port(2),packet_type(ns=0,id=0),eth_type(0x8847),
mpls(label=22/0xfffff,tc=0/0,ttl=64/0x0,bos=1/1), packets:20, bytes:2040,
used:0.329s, actions:pop_mpls(eth_type=0x800),recirc(0x1)

With Fix:
recirc_id(0x2),dp_hash(0x5194bf18/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:12481, bytes:1223138,
used:0.588s, actions:1
recirc_id(0x1),in_port(3),packet_type(ns=0,id=0),eth_type(0x0800),
ipv4(frag=no), packets:74431, bytes:7294238, used:0.386s,
actions:hash(sym_l4(0)),recirc(0x2)
recirc_id(0x2),dp_hash(0xb952470d/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:12441, bytes:1219218,
used:0.482s, actions:1
recirc_id(0x2),dp_hash(0xeff6ad76/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:12385, bytes:1213730,
used:0.908s, actions:1
recirc_id(0),in_port(3),packet_type(ns=0,id=0),eth_type(0x8847),
mpls(label=22/0xfffff,tc=0/0,ttl=64/0x0,bos=1/1), packets:74431,
bytes:7591962, used:0.386s, actions:pop_mpls(eth_type=0x800),recirc(0x1)
recirc_id(0x2),dp_hash(0xb6233fbe/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:12369, bytes:1212162,
used:0.386s, actions:1
recirc_id(0x2),dp_hash(0xa3670459/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:24751, bytes:2425598,
used:0.483s, actions:1

Signed-off-by: Surya Rudra <rudrasurya.r@altencalsoftlabs.com>
---
 ofproto/ofproto-dpif-rid.c   | 2 ++
 ofproto/ofproto-dpif-rid.h   | 1 +
 ofproto/ofproto-dpif-xlate.c | 2 ++
 3 files changed, 5 insertions(+)

Comments

Vishal Deep Ajmera via dev July 31, 2019, 5:11 a.m. UTC | #1
Hi All,

Please consider this as gentle reminder.

Thanks & Regards,
Surya.

-----Original Message-----
From: Surya Rudra <rudrasurya.r@altencalsoftlabs.com> 
Sent: 19 July 2019 11:05 AM
To: ovs-dev@openvswitch.org
Cc: Surya Rudra <rudrasurya.r@altencalsoftlabs.com>
Subject: [PATCH] ofproto-dpif: Fix for recirc issue with mpls traffic with dp_hash

Fix infinite recirculation loop for MPLS packets sent to dp_hash-based  select group

Issue:
When a MPLS encapsulated packet is received, the MPLS header is removed, a recirculation id assigned and then recirculated into the pipeline.
If the flow rules require the packet to be then sent over DP-HASH based select group buckets, the packet has to be recirculated again. However, the same recirculation id was used and this resulted in the packet being repeatedly recirculated until it got dropped because the maximum recirculation limit was hit.

Fix:
Include the  “was_mpls” boolean which indicates whether the packet was MPLS encapsulated when computing the hash. After popping the MPLS header this will result in a  different hash value than before and new recirculation id will get generated.

DPCTL flows with and without the fix are shown below Without Fix:
recirc_id(0x1),dp_hash(0x5194bf18/0xf),in_port(2),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:20, bytes:1960, used:0.329s, actions:1 recirc_id(0x1),in_port(2),packet_type(ns=0,id=0),eth_type(0x0800),
ipv4(frag=no), packets:20, bytes:1960, used:0.329s,
actions:hash(sym_l4(0)),recirc(0x1)
recirc_id(0),in_port(2),packet_type(ns=0,id=0),eth_type(0x8847),
mpls(label=22/0xfffff,tc=0/0,ttl=64/0x0,bos=1/1), packets:20, bytes:2040, used:0.329s, actions:pop_mpls(eth_type=0x800),recirc(0x1)

With Fix:
recirc_id(0x2),dp_hash(0x5194bf18/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:12481, bytes:1223138, used:0.588s, actions:1 recirc_id(0x1),in_port(3),packet_type(ns=0,id=0),eth_type(0x0800),
ipv4(frag=no), packets:74431, bytes:7294238, used:0.386s,
actions:hash(sym_l4(0)),recirc(0x2)
recirc_id(0x2),dp_hash(0xb952470d/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:12441, bytes:1219218, used:0.482s, actions:1 recirc_id(0x2),dp_hash(0xeff6ad76/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:12385, bytes:1213730, used:0.908s, actions:1 recirc_id(0),in_port(3),packet_type(ns=0,id=0),eth_type(0x8847),
mpls(label=22/0xfffff,tc=0/0,ttl=64/0x0,bos=1/1), packets:74431, bytes:7591962, used:0.386s, actions:pop_mpls(eth_type=0x800),recirc(0x1)
recirc_id(0x2),dp_hash(0xb6233fbe/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:12369, bytes:1212162, used:0.386s, actions:1 recirc_id(0x2),dp_hash(0xa3670459/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:24751, bytes:2425598, used:0.483s, actions:1

Signed-off-by: Surya Rudra <rudrasurya.r@altencalsoftlabs.com>
---
 ofproto/ofproto-dpif-rid.c   | 2 ++
 ofproto/ofproto-dpif-rid.h   | 1 +
 ofproto/ofproto-dpif-xlate.c | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c index 79412c2..29aafc2 100644
--- a/ofproto/ofproto-dpif-rid.c
+++ b/ofproto/ofproto-dpif-rid.c
@@ -130,6 +130,7 @@ frozen_state_hash(const struct frozen_state *state)
     hash = hash_bytes64((const uint64_t *) &state->metadata,
                         sizeof state->metadata, hash);
     hash = hash_boolean(state->conntracked, hash);
+    hash = hash_boolean(state->was_mpls, hash);
     if (state->stack && state->stack_size) {
         hash = hash_bytes(state->stack, state->stack_size, hash);
     }
@@ -158,6 +159,7 @@ frozen_state_equal(const struct frozen_state *a, const struct frozen_state *b)
             && !memcmp(a->stack, b->stack, a->stack_size)
             && a->mirrors == b->mirrors
             && a->conntracked == b->conntracked
+            && a->was_mpls == b->was_mpls
             && ofpacts_equal(a->ofpacts, a->ofpacts_len,
                              b->ofpacts, b->ofpacts_len)
             && ofpacts_equal(a->action_set, a->action_set_len, diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h index ab5b87a..147ef9c 100644
--- a/ofproto/ofproto-dpif-rid.h
+++ b/ofproto/ofproto-dpif-rid.h
@@ -143,6 +143,7 @@ struct frozen_state {
     size_t stack_size;
     mirror_mask_t mirrors;        /* Mirrors already output. */
     bool conntracked;             /* Conntrack occurred prior to freeze. */
+    bool was_mpls;                /* MPLS packet */
     struct uuid xport_uuid;       /* UUID of 1st port packet received on. */
 
     /* Actions to be translated when thawing. */ diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 73966a4..d4edf9c 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4764,6 +4764,7 @@ xlate_controller_action(struct xlate_ctx *ctx, int len,
         .stack_size = ctx->stack.size,
         .mirrors = ctx->mirrors,
         .conntracked = ctx->conntracked,
+        .was_mpls = ctx->was_mpls,
         .ofpacts = NULL,
         .ofpacts_len = 0,
         .action_set = NULL,
@@ -4838,6 +4839,7 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
         .stack_size = ctx->stack.size,
         .mirrors = ctx->mirrors,
         .conntracked = ctx->conntracked,
+        .was_mpls = ctx->was_mpls,
         .xport_uuid = ctx->xin->xport_uuid,
         .ofpacts = ctx->frozen_actions.data,
         .ofpacts_len = ctx->frozen_actions.size,
--
2.7.4
Vishal Deep Ajmera via dev Aug. 12, 2019, 11:18 a.m. UTC | #2
Hi All,

Please consider this as second gentle reminder.
Could you please review this fix and provide your valuable suggestions and comments.

Thanks & Regards,
Surya.

-----Original Message-----
From: Rudra Surya Bhaskara Rao <rudrasurya.r@altencalsoftlabs.com> 
Sent: 31 July 2019 10:41 AM
To: 'ovs-dev@openvswitch.org' <ovs-dev@openvswitch.org>
Subject: RE: [PATCH] ofproto-dpif: Fix for recirc issue with mpls traffic with dp_hash

Hi All,

Please consider this as gentle reminder.

Thanks & Regards,
Surya.

-----Original Message-----
From: Surya Rudra <rudrasurya.r@altencalsoftlabs.com> 
Sent: 19 July 2019 11:05 AM
To: ovs-dev@openvswitch.org
Cc: Surya Rudra <rudrasurya.r@altencalsoftlabs.com>
Subject: [PATCH] ofproto-dpif: Fix for recirc issue with mpls traffic with dp_hash

Fix infinite recirculation loop for MPLS packets sent to dp_hash-based  select group

Issue:
When a MPLS encapsulated packet is received, the MPLS header is removed, a recirculation id assigned and then recirculated into the pipeline.
If the flow rules require the packet to be then sent over DP-HASH based select group buckets, the packet has to be recirculated again. However, the same recirculation id was used and this resulted in the packet being repeatedly recirculated until it got dropped because the maximum recirculation limit was hit.

Fix:
Include the  “was_mpls” boolean which indicates whether the packet was MPLS encapsulated when computing the hash. After popping the MPLS header this will result in a  different hash value than before and new recirculation id will get generated.

DPCTL flows with and without the fix are shown below Without Fix:
recirc_id(0x1),dp_hash(0x5194bf18/0xf),in_port(2),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:20, bytes:1960, used:0.329s, actions:1 recirc_id(0x1),in_port(2),packet_type(ns=0,id=0),eth_type(0x0800),
ipv4(frag=no), packets:20, bytes:1960, used:0.329s,
actions:hash(sym_l4(0)),recirc(0x1)
recirc_id(0),in_port(2),packet_type(ns=0,id=0),eth_type(0x8847),
mpls(label=22/0xfffff,tc=0/0,ttl=64/0x0,bos=1/1), packets:20, bytes:2040, used:0.329s, actions:pop_mpls(eth_type=0x800),recirc(0x1)

With Fix:
recirc_id(0x2),dp_hash(0x5194bf18/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:12481, bytes:1223138, used:0.588s, actions:1 recirc_id(0x1),in_port(3),packet_type(ns=0,id=0),eth_type(0x0800),
ipv4(frag=no), packets:74431, bytes:7294238, used:0.386s,
actions:hash(sym_l4(0)),recirc(0x2)
recirc_id(0x2),dp_hash(0xb952470d/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:12441, bytes:1219218, used:0.482s, actions:1 recirc_id(0x2),dp_hash(0xeff6ad76/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:12385, bytes:1213730, used:0.908s, actions:1 recirc_id(0),in_port(3),packet_type(ns=0,id=0),eth_type(0x8847),
mpls(label=22/0xfffff,tc=0/0,ttl=64/0x0,bos=1/1), packets:74431, bytes:7591962, used:0.386s, actions:pop_mpls(eth_type=0x800),recirc(0x1)
recirc_id(0x2),dp_hash(0xb6233fbe/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:12369, bytes:1212162, used:0.386s, actions:1 recirc_id(0x2),dp_hash(0xa3670459/0xf),in_port(3),packet_type(ns=0,id=0),
eth_type(0x0800),ipv4(frag=no), packets:24751, bytes:2425598, used:0.483s, actions:1

Signed-off-by: Surya Rudra <rudrasurya.r@altencalsoftlabs.com>
---
 ofproto/ofproto-dpif-rid.c   | 2 ++
 ofproto/ofproto-dpif-rid.h   | 1 +
 ofproto/ofproto-dpif-xlate.c | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c index 79412c2..29aafc2 100644
--- a/ofproto/ofproto-dpif-rid.c
+++ b/ofproto/ofproto-dpif-rid.c
@@ -130,6 +130,7 @@ frozen_state_hash(const struct frozen_state *state)
     hash = hash_bytes64((const uint64_t *) &state->metadata,
                         sizeof state->metadata, hash);
     hash = hash_boolean(state->conntracked, hash);
+    hash = hash_boolean(state->was_mpls, hash);
     if (state->stack && state->stack_size) {
         hash = hash_bytes(state->stack, state->stack_size, hash);
     }
@@ -158,6 +159,7 @@ frozen_state_equal(const struct frozen_state *a, const struct frozen_state *b)
             && !memcmp(a->stack, b->stack, a->stack_size)
             && a->mirrors == b->mirrors
             && a->conntracked == b->conntracked
+            && a->was_mpls == b->was_mpls
             && ofpacts_equal(a->ofpacts, a->ofpacts_len,
                              b->ofpacts, b->ofpacts_len)
             && ofpacts_equal(a->action_set, a->action_set_len, diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h index ab5b87a..147ef9c 100644
--- a/ofproto/ofproto-dpif-rid.h
+++ b/ofproto/ofproto-dpif-rid.h
@@ -143,6 +143,7 @@ struct frozen_state {
     size_t stack_size;
     mirror_mask_t mirrors;        /* Mirrors already output. */
     bool conntracked;             /* Conntrack occurred prior to freeze. */
+    bool was_mpls;                /* MPLS packet */
     struct uuid xport_uuid;       /* UUID of 1st port packet received on. */
 
     /* Actions to be translated when thawing. */ diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 73966a4..d4edf9c 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4764,6 +4764,7 @@ xlate_controller_action(struct xlate_ctx *ctx, int len,
         .stack_size = ctx->stack.size,
         .mirrors = ctx->mirrors,
         .conntracked = ctx->conntracked,
+        .was_mpls = ctx->was_mpls,
         .ofpacts = NULL,
         .ofpacts_len = 0,
         .action_set = NULL,
@@ -4838,6 +4839,7 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
         .stack_size = ctx->stack.size,
         .mirrors = ctx->mirrors,
         .conntracked = ctx->conntracked,
+        .was_mpls = ctx->was_mpls,
         .xport_uuid = ctx->xin->xport_uuid,
         .ofpacts = ctx->frozen_actions.data,
         .ofpacts_len = ctx->frozen_actions.size,
--
2.7.4
Ben Pfaff Aug. 22, 2019, 8:15 p.m. UTC | #3
On Fri, Jul 19, 2019 at 11:05:19AM +0530, Surya Rudra via dev wrote:
> Fix infinite recirculation loop for MPLS packets sent to dp_hash-based
>  select group

Thanks for the bug fix.  I applied this to master.

Patch
diff mbox series

diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
index 79412c2..29aafc2 100644
--- a/ofproto/ofproto-dpif-rid.c
+++ b/ofproto/ofproto-dpif-rid.c
@@ -130,6 +130,7 @@  frozen_state_hash(const struct frozen_state *state)
     hash = hash_bytes64((const uint64_t *) &state->metadata,
                         sizeof state->metadata, hash);
     hash = hash_boolean(state->conntracked, hash);
+    hash = hash_boolean(state->was_mpls, hash);
     if (state->stack && state->stack_size) {
         hash = hash_bytes(state->stack, state->stack_size, hash);
     }
@@ -158,6 +159,7 @@  frozen_state_equal(const struct frozen_state *a, const struct frozen_state *b)
             && !memcmp(a->stack, b->stack, a->stack_size)
             && a->mirrors == b->mirrors
             && a->conntracked == b->conntracked
+            && a->was_mpls == b->was_mpls
             && ofpacts_equal(a->ofpacts, a->ofpacts_len,
                              b->ofpacts, b->ofpacts_len)
             && ofpacts_equal(a->action_set, a->action_set_len,
diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
index ab5b87a..147ef9c 100644
--- a/ofproto/ofproto-dpif-rid.h
+++ b/ofproto/ofproto-dpif-rid.h
@@ -143,6 +143,7 @@  struct frozen_state {
     size_t stack_size;
     mirror_mask_t mirrors;        /* Mirrors already output. */
     bool conntracked;             /* Conntrack occurred prior to freeze. */
+    bool was_mpls;                /* MPLS packet */
     struct uuid xport_uuid;       /* UUID of 1st port packet received on. */
 
     /* Actions to be translated when thawing. */
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 73966a4..d4edf9c 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4764,6 +4764,7 @@  xlate_controller_action(struct xlate_ctx *ctx, int len,
         .stack_size = ctx->stack.size,
         .mirrors = ctx->mirrors,
         .conntracked = ctx->conntracked,
+        .was_mpls = ctx->was_mpls,
         .ofpacts = NULL,
         .ofpacts_len = 0,
         .action_set = NULL,
@@ -4838,6 +4839,7 @@  finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
         .stack_size = ctx->stack.size,
         .mirrors = ctx->mirrors,
         .conntracked = ctx->conntracked,
+        .was_mpls = ctx->was_mpls,
         .xport_uuid = ctx->xin->xport_uuid,
         .ofpacts = ctx->frozen_actions.data,
         .ofpacts_len = ctx->frozen_actions.size,