[ovs-dev] datapath-windows: Correct endianness for deleting zone.

Message ID 1512459015-18138-1-git-send-email-jpettit@ovn.org
State New
Headers show
Series
  • [ovs-dev] datapath-windows: Correct endianness for deleting zone.
Related show

Commit Message

Justin Pettit Dec. 5, 2017, 7:30 a.m.
The zone Netlink attribute is supposed to be in network-byte order, but
the Windows code for deleting conntrack entries was treating it as
host-byte order.

Found by inspection.

Signed-off-by: Justin Pettit <jpettit@ovn.org>
---
 datapath-windows/ovsext/Conntrack.c | 2 +-
 lib/netlink-conntrack.c             | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Alin Gabriel Serdean Dec. 6, 2017, 5:41 p.m. | #1
> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> bounces@openvswitch.org] On Behalf Of Justin Pettit
> Sent: Tuesday, December 5, 2017 9:30 AM
> To: dev@openvswitch.org; Alin Gabriel Serdean <aserdean@ovn.org>;
> Sairam Venugopal <vsairam@vmware.com>
> Subject: [ovs-dev] [PATCH] datapath-windows: Correct endianness for
> deleting zone.
> 
> The zone Netlink attribute is supposed to be in network-byte order, but
the
> Windows code for deleting conntrack entries was treating it as host-byte
> order.
> 
> Found by inspection.
> 
> Signed-off-by: Justin Pettit <jpettit@ovn.org>

Thanks for the patch, Justin.  I tested the patch, and everything looks
good.
I want to hold on the acknowledge until all of us are on the same page.

This will make the windows datapath consistent with future patches from the
userspace.
For consistency I would like to backport the patch all the way to
branch-2.6.

What do you think Sai?

Thanks,
Alin.
Sairam Venugopal Dec. 7, 2017, 10:57 p.m. | #2
Thanks for the patch. Discussed about the patch offline. 

This is required since we have now introduced a newer api for deleting 5-tuples
and this breaks the assumption of zone-id not being in the right byte order.

Acked-by: Sairam Venugopal <vsairam@vmware.com>





On 12/4/17, 11:30 PM, "Justin Pettit" <jpettit@ovn.org> wrote:

>The zone Netlink attribute is supposed to be in network-byte order, but
>the Windows code for deleting conntrack entries was treating it as
>host-byte order.
>
>Found by inspection.
>
>Signed-off-by: Justin Pettit <jpettit@ovn.org>
>---
> datapath-windows/ovsext/Conntrack.c | 2 +-
> lib/netlink-conntrack.c             | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
>index 3203411a8b7a..edc0ec9c5324 100644
>--- a/datapath-windows/ovsext/Conntrack.c
>+++ b/datapath-windows/ovsext/Conntrack.c
>@@ -1076,7 +1076,7 @@ OvsCtDeleteCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
>     }
> 
>     if (ctAttrs[CTA_ZONE]) {
>-        zone = NlAttrGetU16(ctAttrs[CTA_ZONE]);
>+        zone = ntohs(NlAttrGetU16(ctAttrs[CTA_ZONE]));
>     }
> 
>     status = OvsCtFlush(zone);
>diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
>index 1e1bb2f79d1d..3c651b6c856a 100644
>--- a/lib/netlink-conntrack.c
>+++ b/lib/netlink-conntrack.c
>@@ -251,7 +251,7 @@ nl_ct_flush_zone(uint16_t flush_zone)
> 
>     nl_msg_put_nfgenmsg(&buf, 0, AF_UNSPEC, NFNL_SUBSYS_CTNETLINK,
>                         IPCTNL_MSG_CT_DELETE, NLM_F_REQUEST);
>-    nl_msg_put_be16(&buf, CTA_ZONE, flush_zone);
>+    nl_msg_put_be16(&buf, CTA_ZONE, htons(flush_zone));
> 
>     err = nl_transact(NETLINK_NETFILTER, &buf, NULL);
>     ofpbuf_uninit(&buf);
>-- 
>2.7.4
>
Justin Pettit Dec. 7, 2017, 11:07 p.m. | #3
> On Dec 7, 2017, at 2:57 PM, Sairam Venugopal <vsairam@vmware.com> wrote:
> 
> Thanks for the patch. Discussed about the patch offline. 
> 
> This is required since we have now introduced a newer api for deleting 5-tuples
> and this breaks the assumption of zone-id not being in the right byte order.
> 
> Acked-by: Sairam Venugopal <vsairam@vmware.com>

Thanks.  I pushed this to master.

--Justin
Sairam Venugopal Dec. 7, 2017, 11:26 p.m. | #4
We can hold off on back-porting this. This change mainly affects the delete by 5-tuple API.

https://github.com/openvswitch/ovs/commit/817a76577fec3f03310d7d3a5a10df01340ee8ad

Unless we plan to backport that patch, we should hold off on backporting this as well.

Thanks,
Sairam



On 12/6/17, 9:41 AM, "aserdean@ovn.org" <aserdean@ovn.org> wrote:

>> -----Original Message-----
>> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
>> bounces@openvswitch.org] On Behalf Of Justin Pettit
>> Sent: Tuesday, December 5, 2017 9:30 AM
>> To: dev@openvswitch.org; Alin Gabriel Serdean <aserdean@ovn.org>;
>> Sairam Venugopal <vsairam@vmware.com>
>> Subject: [ovs-dev] [PATCH] datapath-windows: Correct endianness for
>> deleting zone.
>> 
>> The zone Netlink attribute is supposed to be in network-byte order, but
>the
>> Windows code for deleting conntrack entries was treating it as host-byte
>> order.
>> 
>> Found by inspection.
>> 
>> Signed-off-by: Justin Pettit <jpettit@ovn.org>
>
>Thanks for the patch, Justin.  I tested the patch, and everything looks
>good.
>I want to hold on the acknowledge until all of us are on the same page.
>
>This will make the windows datapath consistent with future patches from the
>userspace.
>For consistency I would like to backport the patch all the way to
>branch-2.6.
>
>What do you think Sai?
>
>Thanks,
>Alin.
>

Patch

diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index 3203411a8b7a..edc0ec9c5324 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -1076,7 +1076,7 @@  OvsCtDeleteCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
     }
 
     if (ctAttrs[CTA_ZONE]) {
-        zone = NlAttrGetU16(ctAttrs[CTA_ZONE]);
+        zone = ntohs(NlAttrGetU16(ctAttrs[CTA_ZONE]));
     }
 
     status = OvsCtFlush(zone);
diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
index 1e1bb2f79d1d..3c651b6c856a 100644
--- a/lib/netlink-conntrack.c
+++ b/lib/netlink-conntrack.c
@@ -251,7 +251,7 @@  nl_ct_flush_zone(uint16_t flush_zone)
 
     nl_msg_put_nfgenmsg(&buf, 0, AF_UNSPEC, NFNL_SUBSYS_CTNETLINK,
                         IPCTNL_MSG_CT_DELETE, NLM_F_REQUEST);
-    nl_msg_put_be16(&buf, CTA_ZONE, flush_zone);
+    nl_msg_put_be16(&buf, CTA_ZONE, htons(flush_zone));
 
     err = nl_transact(NETLINK_NETFILTER, &buf, NULL);
     ofpbuf_uninit(&buf);