diff mbox

[ovs-dev] datapath-windows: Fix conntrack lookups for reversed keys

Message ID BY2PR0501MB2119F5FD5B2E44ADCE58EC53A2B30@BY2PR0501MB2119.namprd05.prod.outlook.com
State Not Applicable
Headers show

Commit Message

Shashank Ram Aug. 1, 2017, 10:08 p.m. UTC

Comments

Alin Serdean Aug. 3, 2017, 5:11 p.m. UTC | #1 
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>


> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> bounces@openvswitch.org] On Behalf Of Shashank Ram
> Sent: Wednesday, August 2, 2017 1:08 AM
> To: Anand Kumar <kumaranand@vmware.com>; dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] datapath-windows: Fix conntrack lookups for
> reversed keys
> 
> 
> 
> 
> ________________________________________
> From: ovs-dev-bounces@openvswitch.org <ovs-dev-
> bounces@openvswitch.org> on behalf of Anand Kumar
> <kumaranand@vmware.com>
> Sent: Tuesday, August 1, 2017 3:01 PM
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH] datapath-windows: Fix conntrack lookups for
> reversed keys
> 
> From: Sairam Venugopal <vsairam@vmware.com>
> 
> The conntrack table needs to be queried for entries in either directions to
> determine if the packet is in forward direction or reply direction.
> 
> The current behavior ends up reversing the incoming packet's 5-Tuple for
> every entry in the loop instead of doing it only once.
> 
> Testing Done:
> - Verified that ICMP requests are no longer treated as replies in Conntrack.
> 
> Change-Id: I826a164cfb9137e2167c404ff5c9bfd9dfaa33ad
> Co-authored-by: Sairam Venugopal <vsairam@vmware.com>
> Signed-off-by: Anand Kumar <kumaranand@vmware.com>
> ---
>  datapath-windows/ovsext/Conntrack.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-
> windows/ovsext/Conntrack.c
> index 8ea1e65..917ebee 100644
> --- a/datapath-windows/ovsext/Conntrack.c
> +++ b/datapath-windows/ovsext/Conntrack.c
> @@ -401,7 +401,14 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx)
>      POVS_CT_ENTRY entry;
>      BOOLEAN reply = FALSE;
>      POVS_CT_ENTRY found = NULL;
> -    OVS_CT_KEY key = ctx->key;
> +
> +    /* Reverse NAT must be performed before OvsCtLookup, so here
> +     * we simply need to flip the src and dst in key and compare
> +     * they are equal. Note that flipped key is not equal to
> +     * rev_key due to NAT effect.
> +     */
> +    OVS_CT_KEY revCtxKey = ctx->key;
> +    OvsCtKeyReverse(&revCtxKey);
> 
>      if (!ctTotalEntries) {
>          return found;
> @@ -410,19 +417,13 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx)
>      LIST_FORALL(&ovsConntrackTable[ctx->hash & CT_HASH_TABLE_MASK],
> link) {
>          entry = CONTAINING_RECORD(link, OVS_CT_ENTRY, link);
> 
> -        if (OvsCtKeyAreSame(key,entry->key)) {
> +        if (OvsCtKeyAreSame(ctx->key, entry->key)) {
>              found = entry;
>              reply = FALSE;
>              break;
>          }
> 
> -        /* Reverse NAT must be performed before OvsCtLookup, so here
> -         * we simply need to flip the src and dst in key and compare
> -         * they are equal. Note that flipped key is not equal to
> -         * rev_key due to NAT effect.
> -         */
> -        OvsCtKeyReverse(&key);
> -        if (OvsCtKeyAreSame(key, entry->key)) {
> +        if (OvsCtKeyAreSame(revCtxKey, entry->key)) {
>              found = entry;
>              reply = TRUE;
>              break;
> --
> 2.9.3.windows.1
> 
> _______________________________________________
> 
> Acked-by: Shashank Ram <rams@vmware.com>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff Aug. 3, 2017, 9:38 p.m. UTC | #2 
Thanks Anand, Shashank, Alin.  I applied this to master.

On Thu, Aug 03, 2017 at 05:11:36PM +0000, Alin Serdean wrote:
> Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
> 
> 
> > -----Original Message-----
> > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> > bounces@openvswitch.org] On Behalf Of Shashank Ram
> > Sent: Wednesday, August 2, 2017 1:08 AM
> > To: Anand Kumar <kumaranand@vmware.com>; dev@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH] datapath-windows: Fix conntrack lookups for
> > reversed keys
> > 
> > 
> > 
> > 
> > ________________________________________
> > From: ovs-dev-bounces@openvswitch.org <ovs-dev-
> > bounces@openvswitch.org> on behalf of Anand Kumar
> > <kumaranand@vmware.com>
> > Sent: Tuesday, August 1, 2017 3:01 PM
> > To: dev@openvswitch.org
> > Subject: [ovs-dev] [PATCH] datapath-windows: Fix conntrack lookups for
> > reversed keys
> > 
> > From: Sairam Venugopal <vsairam@vmware.com>
> > 
> > The conntrack table needs to be queried for entries in either directions to
> > determine if the packet is in forward direction or reply direction.
> > 
> > The current behavior ends up reversing the incoming packet's 5-Tuple for
> > every entry in the loop instead of doing it only once.
> > 
> > Testing Done:
> > - Verified that ICMP requests are no longer treated as replies in Conntrack.
> > 
> > Change-Id: I826a164cfb9137e2167c404ff5c9bfd9dfaa33ad
> > Co-authored-by: Sairam Venugopal <vsairam@vmware.com>
> > Signed-off-by: Anand Kumar <kumaranand@vmware.com>
> > ---
> >  datapath-windows/ovsext/Conntrack.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-
> > windows/ovsext/Conntrack.c
> > index 8ea1e65..917ebee 100644
> > --- a/datapath-windows/ovsext/Conntrack.c
> > +++ b/datapath-windows/ovsext/Conntrack.c
> > @@ -401,7 +401,14 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx)
> >      POVS_CT_ENTRY entry;
> >      BOOLEAN reply = FALSE;
> >      POVS_CT_ENTRY found = NULL;
> > -    OVS_CT_KEY key = ctx->key;
> > +
> > +    /* Reverse NAT must be performed before OvsCtLookup, so here
> > +     * we simply need to flip the src and dst in key and compare
> > +     * they are equal. Note that flipped key is not equal to
> > +     * rev_key due to NAT effect.
> > +     */
> > +    OVS_CT_KEY revCtxKey = ctx->key;
> > +    OvsCtKeyReverse(&revCtxKey);
> > 
> >      if (!ctTotalEntries) {
> >          return found;
> > @@ -410,19 +417,13 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx)
> >      LIST_FORALL(&ovsConntrackTable[ctx->hash & CT_HASH_TABLE_MASK],
> > link) {
> >          entry = CONTAINING_RECORD(link, OVS_CT_ENTRY, link);
> > 
> > -        if (OvsCtKeyAreSame(key,entry->key)) {
> > +        if (OvsCtKeyAreSame(ctx->key, entry->key)) {
> >              found = entry;
> >              reply = FALSE;
> >              break;
> >          }
> > 
> > -        /* Reverse NAT must be performed before OvsCtLookup, so here
> > -         * we simply need to flip the src and dst in key and compare
> > -         * they are equal. Note that flipped key is not equal to
> > -         * rev_key due to NAT effect.
> > -         */
> > -        OvsCtKeyReverse(&key);
> > -        if (OvsCtKeyAreSame(key, entry->key)) {
> > +        if (OvsCtKeyAreSame(revCtxKey, entry->key)) {
> >              found = entry;
> >              reply = TRUE;
> >              break;
> > --
> > 2.9.3.windows.1
> > 
> > _______________________________________________
> > 
> > Acked-by: Shashank Ram <rams@vmware.com>
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox

Patch

diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index 8ea1e65..917ebee 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -401,7 +401,14 @@  OvsCtLookup(OvsConntrackKeyLookupCtx *ctx)
     POVS_CT_ENTRY entry;
     BOOLEAN reply = FALSE;
     POVS_CT_ENTRY found = NULL;
-    OVS_CT_KEY key = ctx->key;
+
+    /* Reverse NAT must be performed before OvsCtLookup, so here
+     * we simply need to flip the src and dst in key and compare
+     * they are equal. Note that flipped key is not equal to
+     * rev_key due to NAT effect.
+     */
+    OVS_CT_KEY revCtxKey = ctx->key;
+    OvsCtKeyReverse(&revCtxKey);

     if (!ctTotalEntries) {
         return found;
@@ -410,19 +417,13 @@  OvsCtLookup(OvsConntrackKeyLookupCtx *ctx)
     LIST_FORALL(&ovsConntrackTable[ctx->hash & CT_HASH_TABLE_MASK], link) {
         entry = CONTAINING_RECORD(link, OVS_CT_ENTRY, link);

-        if (OvsCtKeyAreSame(key,entry->key)) {
+        if (OvsCtKeyAreSame(ctx->key, entry->key)) {
             found = entry;
             reply = FALSE;
             break;
         }

-        /* Reverse NAT must be performed before OvsCtLookup, so here
-         * we simply need to flip the src and dst in key and compare
-         * they are equal. Note that flipped key is not equal to
-         * rev_key due to NAT effect.
-         */
-        OvsCtKeyReverse(&key);
-        if (OvsCtKeyAreSame(key, entry->key)) {
+        if (OvsCtKeyAreSame(revCtxKey, entry->key)) {
             found = entry;
             reply = TRUE;
             break;