| Message ID | 20170811204154.4372-1-kumaranand@vmware.com |
|---|---|
| State | Superseded |
| Headers | show |
Acked-by: Sairam Venugopal <vsairam@vmware.com> On 8/11/17, 1:41 PM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> wrote: > - Update the CT comparison function to compare individual fields instead of >NdisEqualMemory. >- Add in some padding for the ct_endpoint's union. >- Update the Orig Tuple to use ICMP Type and Code instead of Port for ICMP > >Co-authored-by: Sairam Venugopal <vsairam@vmware.com> >Signed-off-by: Anand Kumar <kumaranand@vmware.com> >--- > datapath-windows/ovsext/Conntrack.c | 27 +++++++++++++++++++++------ > datapath-windows/ovsext/Conntrack.h | 5 ++++- > 2 files changed, 25 insertions(+), 7 deletions(-) > >diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c >index 917ebee..81c2167 100644 >--- a/datapath-windows/ovsext/Conntrack.c >+++ b/datapath-windows/ovsext/Conntrack.c >@@ -373,10 +373,18 @@ OvsDetectCtPacket(OvsForwardingContext *fwdCtx, > BOOLEAN > OvsCtKeyAreSame(OVS_CT_KEY ctxKey, OVS_CT_KEY entryKey) > { >- return ((NdisEqualMemory(&ctxKey.src, &entryKey.src, >- sizeof(struct ct_endpoint))) && >- (NdisEqualMemory(&ctxKey.dst, &entryKey.dst, >- sizeof(struct ct_endpoint))) && >+ return ((ctxKey.src.addr.ipv4 == entryKey.src.addr.ipv4) && >+ (ctxKey.src.addr.ipv4_aligned == entryKey.src.addr.ipv4_aligned) && >+ (ctxKey.src.port == entryKey.src.port) && >+ (ctxKey.src.icmp_id == entryKey.src.icmp_id) && >+ (ctxKey.src.icmp_type == entryKey.src.icmp_type) && >+ (ctxKey.src.icmp_code == entryKey.src.icmp_code) && >+ (ctxKey.dst.addr.ipv4 == entryKey.dst.addr.ipv4) && >+ (ctxKey.dst.addr.ipv4_aligned == entryKey.dst.addr.ipv4_aligned) && >+ (ctxKey.dst.port == entryKey.dst.port) && >+ (ctxKey.dst.icmp_id == entryKey.dst.icmp_id) && >+ (ctxKey.dst.icmp_type == entryKey.dst.icmp_type) && >+ (ctxKey.dst.icmp_code == entryKey.dst.icmp_code) && > (ctxKey.dl_type == entryKey.dl_type) && > (ctxKey.nw_proto == entryKey.nw_proto) && > (ctxKey.zone == entryKey.zone)); >@@ -782,9 +790,16 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, > > key->ct.tuple_ipv4.ipv4_src = ctKey->src.addr.ipv4_aligned; > key->ct.tuple_ipv4.ipv4_dst = ctKey->dst.addr.ipv4_aligned; >- key->ct.tuple_ipv4.src_port = ctKey->src.port; >- key->ct.tuple_ipv4.dst_port = ctKey->dst.port; > key->ct.tuple_ipv4.ipv4_proto = ctKey->nw_proto; >+ >+ /* Orig tuple Port is overloaded to take in ICMP-Type & Code */ >+ /* This mimics the behavior in lib/conntrack.c*/ >+ key->ct.tuple_ipv4.src_port = ctKey->nw_proto != IPPROTO_ICMP ? >+ ctKey->src.port : >+ htons(ctKey->src.icmp_type); >+ key->ct.tuple_ipv4.dst_port = ctKey->nw_proto != IPPROTO_ICMP ? >+ ctKey->dst.port : >+ htons(ctKey->src.icmp_code); > } > > if (entryCreated && entry) { >diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h >index 04ca299..4904c7e 100644 >--- a/datapath-windows/ovsext/Conntrack.h >+++ b/datapath-windows/ovsext/Conntrack.h >@@ -41,7 +41,10 @@ struct ct_addr { > struct ct_endpoint { > struct ct_addr addr; > union { >- ovs_be16 port; >+ struct { >+ ovs_be16 port; >+ uint16 pad_port; >+ }; > struct { > ovs_be16 icmp_id; > uint8_t icmp_type; >-- >2.9.3.windows.1 > >_______________________________________________ >dev mailing list >dev@openvswitch.org >https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=jv5eh3bLA-gmtkDuLhimROtMSTOYl817LE81oKgfKo8&s=zIJGnXCflvEaG9FWrbvkouqo9uSfZ-KoRq7H_GzPiMo&e=
Hi Sai and Anand, Thanks a lot for the patch. I have a few questions regarding the approach. Please see inline. > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Anand Kumar > Sent: Friday, August 11, 2017 11:42 PM > To: dev@openvswitch.org > Subject: [ovs-dev] [PATCH] datapath-windows: Update ICMP-Type and Code > comparison in CT lookup > > - Update the CT comparison function to compare individual fields instead of > NdisEqualMemory. [Alin Serdean] I don't like this change, especially mixing both members of union, i.e: > + (ctxKey.dst.port == entryKey.dst.port) && > + (ctxKey.dst.icmp_id == entryKey.dst.icmp_id) && Why are you trying to change via member by member comparison? > - Add in some padding for the ct_endpoint's union. [Alin Serdean] Why is this needed? Another question do we still need the 'pad' member inside ct_endpoint (https://github.com/openvswitch/ovs/blob/master/datapath-windows/ovsext/Conntrack.h#L51) ? > - Update the Orig Tuple to use ICMP Type and Code instead of Port for ICMP [Alin Serdean] Agreed > > Co-authored-by: Sairam Venugopal <vsairam@vmware.com> > Signed-off-by: Anand Kumar <kumaranand@vmware.com>
Hi Alin,
Thanks for the review. Please find my responses inline.
Will address the review comments and send out v2 version of the patch.
Thanks,
Anand Kumar
On 8/14/17, 8:04 PM, "Alin Serdean" <aserdean@cloudbasesolutions.com> wrote:
Hi Sai and Anand,
Thanks a lot for the patch. I have a few questions regarding the approach. Please see inline.
> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> bounces@openvswitch.org] On Behalf Of Anand Kumar
> Sent: Friday, August 11, 2017 11:42 PM
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH] datapath-windows: Update ICMP-Type and Code
> comparison in CT lookup
>
> - Update the CT comparison function to compare individual fields instead of
> NdisEqualMemory.
[Alin Serdean] I don't like this change, especially mixing both members of union, i.e:
> + (ctxKey.dst.port == entryKey.dst.port) &&
> + (ctxKey.dst.icmp_id == entryKey.dst.icmp_id) &&
Why are you trying to change via member by member comparison?
[Anand Kumar]: Done. Previously, we ran into issues while verifying NAT with ICMP, which now is fixed with my other patch.
> - Add in some padding for the ct_endpoint's union.
[Alin Serdean] Why is this needed?
[Anand Kumar] This is needed because size of the union is 32 bits.
Another question do we still need the 'pad' member inside ct_endpoint (https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs_blob_master_datapath-2Dwindows_ovsext_Conntrack.h-23L51&d=DwIFAg&c=uilaK90D4TOVoH58JNXRgQ&r=Q5z9tBe-nAOpE7LIHSPV8uy5-437agMXvkeHHMkR8Us&m=1Rj8RlVaAovz2iLrWHvT6N469qFN036rzjLkBJweZDw&s=hky8AVI9m8GjZOD8r21WYvEHBPcDcVVZiwMKqk07IwU&e= ) ?
[Anand Kumar] Yes, the padding at the end of structure is no longer needed. I will address in next version.
> - Update the Orig Tuple to use ICMP Type and Code instead of Port for ICMP
[Alin Serdean] Agreed
>
> Co-authored-by: Sairam Venugopal <vsairam@vmware.com>
> Signed-off-by: Anand Kumar <kumaranand@vmware.com>
diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c index 917ebee..81c2167 100644 --- a/datapath-windows/ovsext/Conntrack.c +++ b/datapath-windows/ovsext/Conntrack.c @@ -373,10 +373,18 @@ OvsDetectCtPacket(OvsForwardingContext *fwdCtx, BOOLEAN OvsCtKeyAreSame(OVS_CT_KEY ctxKey, OVS_CT_KEY entryKey) { - return ((NdisEqualMemory(&ctxKey.src, &entryKey.src, - sizeof(struct ct_endpoint))) && - (NdisEqualMemory(&ctxKey.dst, &entryKey.dst, - sizeof(struct ct_endpoint))) && + return ((ctxKey.src.addr.ipv4 == entryKey.src.addr.ipv4) && + (ctxKey.src.addr.ipv4_aligned == entryKey.src.addr.ipv4_aligned) && + (ctxKey.src.port == entryKey.src.port) && + (ctxKey.src.icmp_id == entryKey.src.icmp_id) && + (ctxKey.src.icmp_type == entryKey.src.icmp_type) && + (ctxKey.src.icmp_code == entryKey.src.icmp_code) && + (ctxKey.dst.addr.ipv4 == entryKey.dst.addr.ipv4) && + (ctxKey.dst.addr.ipv4_aligned == entryKey.dst.addr.ipv4_aligned) && + (ctxKey.dst.port == entryKey.dst.port) && + (ctxKey.dst.icmp_id == entryKey.dst.icmp_id) && + (ctxKey.dst.icmp_type == entryKey.dst.icmp_type) && + (ctxKey.dst.icmp_code == entryKey.dst.icmp_code) && (ctxKey.dl_type == entryKey.dl_type) && (ctxKey.nw_proto == entryKey.nw_proto) && (ctxKey.zone == entryKey.zone)); @@ -782,9 +790,16 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, key->ct.tuple_ipv4.ipv4_src = ctKey->src.addr.ipv4_aligned; key->ct.tuple_ipv4.ipv4_dst = ctKey->dst.addr.ipv4_aligned; - key->ct.tuple_ipv4.src_port = ctKey->src.port; - key->ct.tuple_ipv4.dst_port = ctKey->dst.port; key->ct.tuple_ipv4.ipv4_proto = ctKey->nw_proto; + + /* Orig tuple Port is overloaded to take in ICMP-Type & Code */ + /* This mimics the behavior in lib/conntrack.c*/ + key->ct.tuple_ipv4.src_port = ctKey->nw_proto != IPPROTO_ICMP ? + ctKey->src.port : + htons(ctKey->src.icmp_type); + key->ct.tuple_ipv4.dst_port = ctKey->nw_proto != IPPROTO_ICMP ? + ctKey->dst.port : + htons(ctKey->src.icmp_code); } if (entryCreated && entry) { diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h index 04ca299..4904c7e 100644 --- a/datapath-windows/ovsext/Conntrack.h +++ b/datapath-windows/ovsext/Conntrack.h @@ -41,7 +41,10 @@ struct ct_addr { struct ct_endpoint { struct ct_addr addr; union { - ovs_be16 port; + struct { + ovs_be16 port; + uint16 pad_port; + }; struct { ovs_be16 icmp_id; uint8_t icmp_type;
- Update the CT comparison function to compare individual fields instead of NdisEqualMemory. - Add in some padding for the ct_endpoint's union. - Update the Orig Tuple to use ICMP Type and Code instead of Port for ICMP Co-authored-by: Sairam Venugopal <vsairam@vmware.com> Signed-off-by: Anand Kumar <kumaranand@vmware.com> --- datapath-windows/ovsext/Conntrack.c | 27 +++++++++++++++++++++------ datapath-windows/ovsext/Conntrack.h | 5 ++++- 2 files changed, 25 insertions(+), 7 deletions(-)