We should revisit how we do hashes and compares over the 'OVS_CT_KEY' at some point.
As you pointed "/* icmp_id and port overlap in the union */" You can drop the lines: > HASH_ADD(src.port); > HASH_ADD(dst.port); And > FIELD_COMPARE(src.port); > FIELD_COMPARE(dst.port); the outcome should be the same. Everything else looks good. Thanks, Alin. > -----Original Message----- > From: [email protected] [mailto:ovs-dev- > [email protected]] On Behalf Of Anand Kumar > Sent: Friday, August 11, 2017 6:59 AM > To: [email protected] > Subject: [ovs-dev] [PATCH] 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/id/code into account > during the lookups. Fix this to make it similar with the other conntrack > module. > > Signed-off-by: Anand Kumar <[email protected]> > --- > datapath-windows/ovsext/Conntrack-nat.c | 23 ++++++++++++++++++++-- > - > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath- > windows/ovsext/Conntrack-nat.c > index ae6b92c..eb6f9db 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,12 @@ OvsNatKeyAreSame(const OVS_CT_KEY *key1, const > OVS_CT_KEY *key2) > FIELD_COMPARE(src.port); > FIELD_COMPARE(dst.port); > FIELD_COMPARE(zone); > + FIELD_COMPARE(src.icmp_id); > + FIELD_COMPARE(dst.icmp_id); > + 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 +265,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 +284,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 > +323,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); > -- > 2.9.3.windows.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
