| Message ID | 20170815222904.5080-1-kumaranand@vmware.com |
|---|---|
| State | Accepted |
| Headers | show |
Thanks a lot for the patch and changes! Acked-by: Alin Gabriel Serdean <aserdean@ovn.org> > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Anand Kumar > Sent: Wednesday, August 16, 2017 1:29 AM > To: dev@openvswitch.org > Subject: [ovs-dev] [PATCH v2] datapath-windows: Do not modify port field > for ICMP during SNAT/DNAT > > During SNAT/DNAT, we should not be updating the port field of ct_endpoint > struct, as ICMP packets do not have port information. Since port and icmp_id > are overlapped in ct_endpoint struct, icmp_id gets changed. > As a result, NAT look up fails to find a matching entry. > > This patch addresses this issue by not modifying icmp_id field during > SNAT/DNAT only for ICMP traffic > > The current NAT module doesn't take the ICMP type/code into account > during the lookups. Fix this to make it similar with the other conntrack > module. > > Signed-off-by: Anand Kumar <kumaranand@vmware.com>
I applied this on master and branch-2.8 Thanks, Alin. > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Alin Serdean > Sent: Thursday, August 17, 2017 10:47 PM > To: Anand Kumar <kumaranand@vmware.com>; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2] datapath-windows: Do not modify port > field for ICMP during SNAT/DNAT > > Thanks a lot for the patch and changes! > > Acked-by: Alin Gabriel Serdean <aserdean@ovn.org> > > > -----Original Message----- > > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > > bounces@openvswitch.org] On Behalf Of Anand Kumar > > Sent: Wednesday, August 16, 2017 1:29 AM > > To: dev@openvswitch.org > > Subject: [ovs-dev] [PATCH v2] datapath-windows: Do not modify port > > field for ICMP during SNAT/DNAT > > > > During SNAT/DNAT, we should not be updating the port field of > > ct_endpoint struct, as ICMP packets do not have port information. > > Since port and icmp_id are overlapped in ct_endpoint struct, icmp_id gets > changed. > > As a result, NAT look up fails to find a matching entry. > > > > This patch addresses this issue by not modifying icmp_id field during > > SNAT/DNAT only for ICMP traffic > > > > The current NAT module doesn't take the ICMP type/code into account > > during the lookups. Fix this to make it similar with the other > > conntrack module. > > > > Signed-off-by: Anand Kumar <kumaranand@vmware.com> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c index ae6b92c..c778f12 100644 --- a/datapath-windows/ovsext/Conntrack-nat.c +++ b/datapath-windows/ovsext/Conntrack-nat.c @@ -22,6 +22,12 @@ OvsHashNatKey(const OVS_CT_KEY *key) HASH_ADD(src.port); HASH_ADD(dst.port); HASH_ADD(zone); + /* icmp_id and port overlap in the union */ + HASH_ADD(src.icmp_type); + HASH_ADD(dst.icmp_type); + HASH_ADD(src.icmp_code); + HASH_ADD(dst.icmp_code); + #undef HASH_ADD return hash; } @@ -44,6 +50,11 @@ OvsNatKeyAreSame(const OVS_CT_KEY *key1, const OVS_CT_KEY *key2) FIELD_COMPARE(src.port); FIELD_COMPARE(dst.port); FIELD_COMPARE(zone); + /* icmp_id and port overlap in the union */ + FIELD_COMPARE(src.icmp_type); + FIELD_COMPARE(dst.icmp_type); + FIELD_COMPARE(src.icmp_code); + FIELD_COMPARE(dst.icmp_code); return TRUE; #undef FIELD_COMPARE } @@ -253,6 +264,7 @@ OvsNatAddEntry(OVS_NAT_ENTRY* entry) * Update an Conntrack entry with NAT information. Translated address and * port will be generated and write back to the conntrack entry as a * result. + * Note: For ICMP, only address is translated. *---------------------------------------------------------------------------- */ BOOLEAN @@ -271,7 +283,7 @@ OvsNatTranslateCtEntry(OVS_CT_ENTRY *entry) BOOLEAN allPortsTried; BOOLEAN originalPortsTried; struct ct_addr firstAddr; - + uint32_t hash = OvsNatHashRange(entry, 0); if ((entry->natInfo.natAction & NAT_ACTION_SRC) && @@ -310,10 +322,14 @@ OvsNatTranslateCtEntry(OVS_CT_ENTRY *entry) for (;;) { if (entry->natInfo.natAction & NAT_ACTION_SRC) { entry->rev_key.dst.addr = ctAddr; - entry->rev_key.dst.port = htons(port); + if (entry->rev_key.nw_proto != IPPROTO_ICMP) { + entry->rev_key.dst.port = htons(port); + } } else { entry->rev_key.src.addr = ctAddr; - entry->rev_key.src.port = htons(port); + if (entry->rev_key.nw_proto != IPPROTO_ICMP) { + entry->rev_key.src.port = htons(port); + } } OVS_NAT_ENTRY *natEntry = OvsNatLookup(&entry->rev_key, TRUE);
During SNAT/DNAT, we should not be updating the port field of ct_endpoint struct, as ICMP packets do not have port information. Since port and icmp_id are overlapped in ct_endpoint struct, icmp_id gets changed. As a result, NAT look up fails to find a matching entry. This patch addresses this issue by not modifying icmp_id field during SNAT/DNAT only for ICMP traffic The current NAT module doesn't take the ICMP type/code into account during the lookups. Fix this to make it similar with the other conntrack module. Signed-off-by: Anand Kumar <kumaranand@vmware.com> --- datapath-windows/ovsext/Conntrack-nat.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)