Hi Alin, Thanks for reviewing the patch. Please find my responses inline. Will send out a v2 patch addressing the review comments.
Thanks, Anand Kumar On 8/14/17, 8:14 PM, "Alin Serdean" <[email protected]> wrote: 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); [Anand Kumar] : We would still need it here since ICMP id is not included for computing hash. And > FIELD_COMPARE(src.port); > FIELD_COMPARE(dst.port); the outcome should be the same. [Anand Kumar] : Not required, will remove icmp_id field instead of port. FIELD_COMPARE(src.icmp_id); FIELD_COMPARE(dst.icmp_id); 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://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIFAg&c=uilaK90D4TOVoH58JNXRgQ&r=Q5z9tBe-nAOpE7LIHSPV8uy5-437agMXvkeHHMkR8Us&m=wfdj2OXBLKbF3522VGnqdqJe_orJVBbMTQpVIT3ccV8&s=tlFFROWZGldsFcJ65VVeac49_xLylav_QLgUwG4uBRg&e= _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
