Re: [ovs-dev] [PATCH v2] datapath-windows: Update flow key in SET action

2020-07-29 Thread Alin Serdean 
Applied on master. Thank you for the fixes and testing!


Alin.

From: Jinjun Gao
Sent: Wednesday, July 29, 2020 6:29 AM
To: d...@openvswitch.org; Alin 
Serdean; 
kumaran...@vmware.com
Cc: r...@vmware.com; Jinjun 
Gao
Subject: [PATCH v2] datapath-windows: Update flow key in SET action

The flow key is not updated when process OVS_ACTION_ATTR_SET action.
It will impact follow-up actions, such as, conntrack module cannot
find created conntrack entry if passing old flow key to it.

Reported-by: Rui Cao 
Signed-off-by: Jinjun Gao 
---
v2: Separated assignment to happy MSFT SDV tool
---
 datapath-windows/ovsext/Actions.c | 31 ---
 datapath-windows/ovsext/Actions.h |  3 +++
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index 4a11cea..4f43369 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1259,6 +1259,7 @@ OvsActionMplsPush(OvsForwardingContext *ovsFwdCtx,
  */
 static __inline NDIS_STATUS
 OvsUpdateEthHeader(OvsForwardingContext *ovsFwdCtx,
+   OvsFlowKey *key,
const struct ovs_key_ethernet *ethAttr)
 {
 PNET_BUFFER curNb;
@@ -1285,9 +1286,11 @@ OvsUpdateEthHeader(OvsForwardingContext *ovsFwdCtx,
 }
 ethHdr = (EthHdr *)(bufferStart + NET_BUFFER_CURRENT_MDL_OFFSET(curNb));

-RtlCopyMemory(ethHdr->Destination, ethAttr->eth_dst,
-   sizeof ethHdr->Destination);
-RtlCopyMemory(ethHdr->Source, ethAttr->eth_src, sizeof ethHdr->Source);
+RtlCopyMemory(ethHdr->Destination, ethAttr->eth_dst, ETH_ADDR_LENGTH);
+RtlCopyMemory(ethHdr->Source, ethAttr->eth_src, ETH_ADDR_LENGTH);
+/* Update l2 flow key */
+RtlCopyMemory(key->l2.dlDst, ethAttr->eth_dst, ETH_ADDR_LENGTH);
+RtlCopyMemory(key->l2.dlSrc, ethAttr->eth_src, ETH_ADDR_LENGTH);

 return NDIS_STATUS_SUCCESS;
 }
@@ -1376,6 +1379,7 @@ PUINT8 OvsGetHeaderBySize(OvsForwardingContext *ovsFwdCtx,
  */
 NDIS_STATUS
 OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
+  OvsFlowKey *key,
   const struct ovs_key_udp *udpAttr)
 {
 PUINT8 bufferStart;
@@ -1400,15 +1404,19 @@ OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
 udpHdr->check = ChecksumUpdate16(udpHdr->check, udpHdr->source,
  udpAttr->udp_src);
 udpHdr->source = udpAttr->udp_src;
+key->ipKey.l4.tpSrc = udpAttr->udp_src;
 }
 if (udpHdr->dest != udpAttr->udp_dst) {
 udpHdr->check = ChecksumUpdate16(udpHdr->check, udpHdr->dest,
  udpAttr->udp_dst);
 udpHdr->dest = udpAttr->udp_dst;
+key->ipKey.l4.tpDst = udpAttr->udp_dst;
 }
 } else {
 udpHdr->source = udpAttr->udp_src;
+key->ipKey.l4.tpSrc = udpAttr->udp_src;
 udpHdr->dest = udpAttr->udp_dst;
+key->ipKey.l4.tpDst = udpAttr->udp_dst;
 }

 return NDIS_STATUS_SUCCESS;
@@ -1423,6 +1431,7 @@ OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
  */
 NDIS_STATUS
 OvsUpdateTcpPorts(OvsForwardingContext *ovsFwdCtx,
+  OvsFlowKey *key,
   const struct ovs_key_tcp *tcpAttr)
 {
 PUINT8 bufferStart;
@@ -1447,11 +1456,13 @@ OvsUpdateTcpPorts(OvsForwardingContext *ovsFwdCtx,
 tcpHdr->check = ChecksumUpdate16(tcpHdr->check, tcpHdr->source,
  tcpAttr->tcp_src);
 tcpHdr->source = tcpAttr->tcp_src;
+key->ipKey.l4.tpSrc = tcpAttr->tcp_src;
 }
 if (tcpHdr->dest != tcpAttr->tcp_dst) {
 tcpHdr->check = ChecksumUpdate16(tcpHdr->check, tcpHdr->dest,
  tcpAttr->tcp_dst);
 tcpHdr->dest = tcpAttr->tcp_dst;
+key->ipKey.l4.tpDst = tcpAttr->tcp_dst;
 }

 return NDIS_STATUS_SUCCESS;
@@ -1579,6 +1590,7 @@ OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
  */
 NDIS_STATUS
 OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
+OvsFlowKey *key,
 const struct ovs_key_ipv4 *ipAttr)
 {
 PUINT8 bufferStart;
@@ -1632,6 +1644,7 @@ OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
 ipAttr->ipv4_src);
 }
 ipHdr->saddr = ipAttr->ipv4_src;
+key->ipKey.nwSrc = ipAttr->ipv4_src;
 }
 if (ipHdr->daddr != ipAttr->ipv4_dst) {
 if (tcpHdr) {
@@ -1647,6 +1660,7 @@ OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
 ipAttr->ipv4_dst);
 }
 ipHdr->daddr = ipAttr->ipv4_dst;
+key->ipKey.nwDst = ipAttr->ipv4_dst;
 }
 if (ipHdr->protocol !=

[ovs-dev] [PATCH v2] datapath-windows: Update flow key in SET action

2020-07-28 Thread Jinjun Gao 
The flow key is not updated when process OVS_ACTION_ATTR_SET action.
It will impact follow-up actions, such as, conntrack module cannot
find created conntrack entry if passing old flow key to it.

Reported-by: Rui Cao 
Signed-off-by: Jinjun Gao 
---
v2: Separated assignment to happy MSFT SDV tool
---
 datapath-windows/ovsext/Actions.c | 31 ---
 datapath-windows/ovsext/Actions.h |  3 +++
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index 4a11cea..4f43369 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1259,6 +1259,7 @@ OvsActionMplsPush(OvsForwardingContext *ovsFwdCtx,
  */
 static __inline NDIS_STATUS
 OvsUpdateEthHeader(OvsForwardingContext *ovsFwdCtx,
+   OvsFlowKey *key,
const struct ovs_key_ethernet *ethAttr)
 {
 PNET_BUFFER curNb;
@@ -1285,9 +1286,11 @@ OvsUpdateEthHeader(OvsForwardingContext *ovsFwdCtx,
 }
 ethHdr = (EthHdr *)(bufferStart + NET_BUFFER_CURRENT_MDL_OFFSET(curNb));

-RtlCopyMemory(ethHdr->Destination, ethAttr->eth_dst,
-   sizeof ethHdr->Destination);
-RtlCopyMemory(ethHdr->Source, ethAttr->eth_src, sizeof ethHdr->Source);
+RtlCopyMemory(ethHdr->Destination, ethAttr->eth_dst, ETH_ADDR_LENGTH);
+RtlCopyMemory(ethHdr->Source, ethAttr->eth_src, ETH_ADDR_LENGTH);
+/* Update l2 flow key */
+RtlCopyMemory(key->l2.dlDst, ethAttr->eth_dst, ETH_ADDR_LENGTH);
+RtlCopyMemory(key->l2.dlSrc, ethAttr->eth_src, ETH_ADDR_LENGTH);

 return NDIS_STATUS_SUCCESS;
 }
@@ -1376,6 +1379,7 @@ PUINT8 OvsGetHeaderBySize(OvsForwardingContext *ovsFwdCtx,
  */
 NDIS_STATUS
 OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
+  OvsFlowKey *key,
   const struct ovs_key_udp *udpAttr)
 {
 PUINT8 bufferStart;
@@ -1400,15 +1404,19 @@ OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
 udpHdr->check = ChecksumUpdate16(udpHdr->check, udpHdr->source,
  udpAttr->udp_src);
 udpHdr->source = udpAttr->udp_src;
+key->ipKey.l4.tpSrc = udpAttr->udp_src;
 }
 if (udpHdr->dest != udpAttr->udp_dst) {
 udpHdr->check = ChecksumUpdate16(udpHdr->check, udpHdr->dest,
  udpAttr->udp_dst);
 udpHdr->dest = udpAttr->udp_dst;
+key->ipKey.l4.tpDst = udpAttr->udp_dst;
 }
 } else {
 udpHdr->source = udpAttr->udp_src;
+key->ipKey.l4.tpSrc = udpAttr->udp_src;
 udpHdr->dest = udpAttr->udp_dst;
+key->ipKey.l4.tpDst = udpAttr->udp_dst;
 }

 return NDIS_STATUS_SUCCESS;
@@ -1423,6 +1431,7 @@ OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
  */
 NDIS_STATUS
 OvsUpdateTcpPorts(OvsForwardingContext *ovsFwdCtx,
+  OvsFlowKey *key,
   const struct ovs_key_tcp *tcpAttr)
 {
 PUINT8 bufferStart;
@@ -1447,11 +1456,13 @@ OvsUpdateTcpPorts(OvsForwardingContext *ovsFwdCtx,
 tcpHdr->check = ChecksumUpdate16(tcpHdr->check, tcpHdr->source,
  tcpAttr->tcp_src);
 tcpHdr->source = tcpAttr->tcp_src;
+key->ipKey.l4.tpSrc = tcpAttr->tcp_src;
 }
 if (tcpHdr->dest != tcpAttr->tcp_dst) {
 tcpHdr->check = ChecksumUpdate16(tcpHdr->check, tcpHdr->dest,
  tcpAttr->tcp_dst);
 tcpHdr->dest = tcpAttr->tcp_dst;
+key->ipKey.l4.tpDst = tcpAttr->tcp_dst;
 }

 return NDIS_STATUS_SUCCESS;
@@ -1579,6 +1590,7 @@ OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
  */
 NDIS_STATUS
 OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
+OvsFlowKey *key,
 const struct ovs_key_ipv4 *ipAttr)
 {
 PUINT8 bufferStart;
@@ -1632,6 +1644,7 @@ OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
 ipAttr->ipv4_src);
 }
 ipHdr->saddr = ipAttr->ipv4_src;
+key->ipKey.nwSrc = ipAttr->ipv4_src;
 }
 if (ipHdr->daddr != ipAttr->ipv4_dst) {
 if (tcpHdr) {
@@ -1647,6 +1660,7 @@ OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
 ipAttr->ipv4_dst);
 }
 ipHdr->daddr = ipAttr->ipv4_dst;
+key->ipKey.nwDst = ipAttr->ipv4_dst;
 }
 if (ipHdr->protocol != ipAttr->ipv4_proto) {
 UINT16 oldProto = (ipHdr->protocol << 16) & 0xff00;
@@ -1661,6 +1675,7 @@ OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
 ipHdr->check = ChecksumUpdate16(ipHdr->check, oldProto, newProto);
 }
 ipHdr->protocol = ipAttr->ipv4_proto;
+key->ipKey.nwProto = ipAttr->ipv4_proto;
 }
 if (ipHdr->ttl != ipAttr->ipv4_ttl) {
 UINT16 oldTtl = (ipHdr->ttl) & 0xff;
@@ -1669,6 +1684,7 @@