Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when processing packet in POP_VLAN action

2021-09-30 Thread alinserdean 
Thank you for addressing the comments I applied the incremental version and 
backported it until 2.7.

Thank you,
Alin.

-Original Message-
From: Wilson Peng  
Sent: Thursday, September 30, 2021 8:02 AM
To: Alin-Gabriel Serdean ; ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when processing 
packet in POP_VLAN action

Alin,

Thanks for your comments, I have sent out the updated patch, please help check.
https://patchwork.ozlabs.org/project/openvswitch/patch/20210930045626.9250-1-pweis...@vmware.com/

Regards
Wilson

在 2021/9/29 23:21，“Alin-Gabriel Serdean” 写入:

Indeed. Thanks for pointing that out.

To be honest that looks like a bug.
We should return an error if the it is not a valid VLAN frame.

-Original Message-
From: Wilson Peng  
Sent: Wednesday, September 29, 2021 5:52 PM
To: Alin-Gabriel Serdean ; ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when 
processing packet in POP_VLAN action

Alin,

Original code diff is to avoid the case below(it will return 
NDIS_STATUS_SUCCESS before RtlMoveMemory in function OvsPopFieldInPacketBuf).

Regards
Wilson

In function OvsPopFieldInPacketBuf(..)
 if (!bufferData) {
EthHdr *ethHdr = (EthHdr *)bufferStart;
/* If the frame is not VLAN make it a no op */
if (ethHdr->Type != ETH_TYPE_802_1PQ_NBO) {
return NDIS_STATUS_SUCCESS;>  may exit here.
}
}

  RtlMoveMemory(bufferStart + shiftLength, bufferStart, shiftOffset);
  NdisAdvanceNetBufferDataStart(curNb, shiftLength, FALSE, NULL);

在 2021/9/29 21:55，“dev 代表 Alin-Gabriel 
Serdean” 写入:

From: aserd...@ovn.org

Thank you for raising awareness about this issue. You are right the
offsets are not being updated when a pop vlan action is hit, they are
updated only on pop_mpls.

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenvswitch%2Fovs%2Fblob%2Fmaster%2Fdatapath-windows%2Fovsext%2FActions.c%23L1173-L1174data=04%7C01%7Cpweisong%40vmware.com%7Ca65191ed9abd47b21a8608d9835cddfe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637685257162775283%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=oonWmImrDVicJ%2BBdGaYvmNDiUsueMXwOOjSn9MuFmBE%3Dreserved=0
Can you please update the offsets in the corresponding pop vlan
function. I.e.:
---
 datapath-windows/ovsext/Actions.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index e130c2f96..34aa5c5e4 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1141,11 +1141,21 @@ OvsPopVlanInPktBuf(OvsForwardingContext 
*ovsFwdCtx)
  * Declare a dummy vlanTag structure since we need to compute the 
size
  * of shiftLength. The NDIS one is a unionized structure.
  */
+NDIS_STATUS status;
+OVS_PACKET_HDR_INFO* layers = >layers;
 NDIS_PACKET_8021Q_INFO vlanTag = {0};
 UINT32 shiftLength = sizeof(vlanTag.TagHeader);
 UINT32 shiftOffset = sizeof(DL_EUI48) + sizeof(DL_EUI48);

-return OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength, 
NULL);
+status = OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, 
shiftLength,
+NULL);
+
+if (status == NDIS_STATUS_SUCCESS) {
+layers->l3Offset -= (UINT16) shiftLength;
+layers->l4Offset -= (UINT16) shiftLength;
+}
+
+return status;
 }


-- 
2.32.0

___
dev mailing list
d...@openvswitch.org

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=04%7C01%7Cpweisong%40vmware.com%7Ca65191ed9abd47b21a8608d9835cddfe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637685257162775283%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=29gFWHNVRPTixEkYAQeD6HevknOxi6%2BYerYFAUM3FTI%3Dreserved=0




___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when processing packet in POP_VLAN action

2021-09-29 Thread Wilson Peng 
Alin,

Thanks for your comments, I have sent out the updated patch, please help check.
https://patchwork.ozlabs.org/project/openvswitch/patch/20210930045626.9250-1-pweis...@vmware.com/

Regards
Wilson

在 2021/9/29 23:21，“Alin-Gabriel Serdean” 写入:

Indeed. Thanks for pointing that out.

To be honest that looks like a bug.
We should return an error if the it is not a valid VLAN frame.

-Original Message-
From: Wilson Peng  
Sent: Wednesday, September 29, 2021 5:52 PM
To: Alin-Gabriel Serdean ; ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when 
processing packet in POP_VLAN action

Alin,

Original code diff is to avoid the case below(it will return 
NDIS_STATUS_SUCCESS before RtlMoveMemory in function OvsPopFieldInPacketBuf).

Regards
Wilson

In function OvsPopFieldInPacketBuf(..)
 if (!bufferData) {
EthHdr *ethHdr = (EthHdr *)bufferStart;
/* If the frame is not VLAN make it a no op */
if (ethHdr->Type != ETH_TYPE_802_1PQ_NBO) {
return NDIS_STATUS_SUCCESS;>  may exit here.
}
}

  RtlMoveMemory(bufferStart + shiftLength, bufferStart, shiftOffset);
  NdisAdvanceNetBufferDataStart(curNb, shiftLength, FALSE, NULL);

在 2021/9/29 21:55，“dev 代表 Alin-Gabriel 
Serdean” 写入:

From: aserd...@ovn.org

Thank you for raising awareness about this issue. You are right the
offsets are not being updated when a pop vlan action is hit, they are
updated only on pop_mpls.

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenvswitch%2Fovs%2Fblob%2Fmaster%2Fdatapath-windows%2Fovsext%2FActions.c%23L1173-L1174data=04%7C01%7Cpweisong%40vmware.com%7Ca65191ed9abd47b21a8608d9835cddfe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637685257162775283%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=oonWmImrDVicJ%2BBdGaYvmNDiUsueMXwOOjSn9MuFmBE%3Dreserved=0
Can you please update the offsets in the corresponding pop vlan
function. I.e.:
---
 datapath-windows/ovsext/Actions.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index e130c2f96..34aa5c5e4 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1141,11 +1141,21 @@ OvsPopVlanInPktBuf(OvsForwardingContext 
*ovsFwdCtx)
  * Declare a dummy vlanTag structure since we need to compute the 
size
  * of shiftLength. The NDIS one is a unionized structure.
  */
+NDIS_STATUS status;
+OVS_PACKET_HDR_INFO* layers = >layers;
 NDIS_PACKET_8021Q_INFO vlanTag = {0};
 UINT32 shiftLength = sizeof(vlanTag.TagHeader);
 UINT32 shiftOffset = sizeof(DL_EUI48) + sizeof(DL_EUI48);

-return OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength, 
NULL);
+status = OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, 
shiftLength,
+NULL);
+
+if (status == NDIS_STATUS_SUCCESS) {
+layers->l3Offset -= (UINT16) shiftLength;
+layers->l4Offset -= (UINT16) shiftLength;
+}
+
+return status;
 }


-- 
2.32.0

___
dev mailing list
d...@openvswitch.org

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=04%7C01%7Cpweisong%40vmware.com%7Ca65191ed9abd47b21a8608d9835cddfe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637685257162775283%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=29gFWHNVRPTixEkYAQeD6HevknOxi6%2BYerYFAUM3FTI%3Dreserved=0



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when processing packet in POP_VLAN action

2021-09-29 Thread Alin-Gabriel Serdean 
Indeed. Thanks for pointing that out.

To be honest that looks like a bug.
We should return an error if the it is not a valid VLAN frame.

-Original Message-
From: Wilson Peng  
Sent: Wednesday, September 29, 2021 5:52 PM
To: Alin-Gabriel Serdean ; ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when processing 
packet in POP_VLAN action

Alin,

Original code diff is to avoid the case below(it will return 
NDIS_STATUS_SUCCESS before RtlMoveMemory in function OvsPopFieldInPacketBuf).

Regards
Wilson

In function OvsPopFieldInPacketBuf(..)
 if (!bufferData) {
EthHdr *ethHdr = (EthHdr *)bufferStart;
/* If the frame is not VLAN make it a no op */
if (ethHdr->Type != ETH_TYPE_802_1PQ_NBO) {
return NDIS_STATUS_SUCCESS;>  may exit here.
}
}

  RtlMoveMemory(bufferStart + shiftLength, bufferStart, shiftOffset);
  NdisAdvanceNetBufferDataStart(curNb, shiftLength, FALSE, NULL);

在 2021/9/29 21:55，“dev 代表 Alin-Gabriel 
Serdean” 写入:

From: aserd...@ovn.org

Thank you for raising awareness about this issue. You are right the
offsets are not being updated when a pop vlan action is hit, they are
updated only on pop_mpls.

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenvswitch%2Fovs%2Fblob%2Fmaster%2Fdatapath-windows%2Fovsext%2FActions.c%23L1173-L1174data=04%7C01%7Cpweisong%40vmware.com%7C97a24c5e1c5642cde59d08d98350cbed%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637685205333861058%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=J4d0vndzCmjpdacwp5KBVntxzNsWepJkSxqvEPRTbCQ%3Dreserved=0
Can you please update the offsets in the corresponding pop vlan
function. I.e.:
---
 datapath-windows/ovsext/Actions.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index e130c2f96..34aa5c5e4 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1141,11 +1141,21 @@ OvsPopVlanInPktBuf(OvsForwardingContext *ovsFwdCtx)
  * Declare a dummy vlanTag structure since we need to compute the size
  * of shiftLength. The NDIS one is a unionized structure.
  */
+NDIS_STATUS status;
+OVS_PACKET_HDR_INFO* layers = >layers;
 NDIS_PACKET_8021Q_INFO vlanTag = {0};
 UINT32 shiftLength = sizeof(vlanTag.TagHeader);
 UINT32 shiftOffset = sizeof(DL_EUI48) + sizeof(DL_EUI48);

-return OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength, 
NULL);
+status = OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength,
+NULL);
+
+if (status == NDIS_STATUS_SUCCESS) {
+layers->l3Offset -= (UINT16) shiftLength;
+layers->l4Offset -= (UINT16) shiftLength;
+}
+
+return status;
 }


-- 
2.32.0

___
dev mailing list
d...@openvswitch.org

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=04%7C01%7Cpweisong%40vmware.com%7C97a24c5e1c5642cde59d08d98350cbed%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637685205333861058%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=cYplthGDj6Y3JKU33UWegukyXDh7ZehNv6wXVqfWD6s%3Dreserved=0


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when processing packet in POP_VLAN action

2021-09-29 Thread Wilson Peng 
Alin,

Original code diff is to avoid the case below(it will return 
NDIS_STATUS_SUCCESS before 
RtlMoveMemory in function OvsPopFieldInPacketBuf).

Regards
Wilson

In function OvsPopFieldInPacketBuf(..)
 if (!bufferData) {
EthHdr *ethHdr = (EthHdr *)bufferStart;
/* If the frame is not VLAN make it a no op */
if (ethHdr->Type != ETH_TYPE_802_1PQ_NBO) {
return NDIS_STATUS_SUCCESS;>  may exit here.
}
}

  RtlMoveMemory(bufferStart + shiftLength, bufferStart, shiftOffset);
  NdisAdvanceNetBufferDataStart(curNb, shiftLength, FALSE, NULL);

在 2021/9/29 21:55，“dev 代表 Alin-Gabriel 
Serdean” 写入:

From: aserd...@ovn.org

Thank you for raising awareness about this issue. You are right the
offsets are not being updated when a pop vlan action is hit, they are
updated only on pop_mpls.

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenvswitch%2Fovs%2Fblob%2Fmaster%2Fdatapath-windows%2Fovsext%2FActions.c%23L1173-L1174data=04%7C01%7Cpweisong%40vmware.com%7C97a24c5e1c5642cde59d08d98350cbed%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637685205333861058%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=J4d0vndzCmjpdacwp5KBVntxzNsWepJkSxqvEPRTbCQ%3Dreserved=0
Can you please update the offsets in the corresponding pop vlan
function. I.e.:
---
 datapath-windows/ovsext/Actions.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index e130c2f96..34aa5c5e4 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1141,11 +1141,21 @@ OvsPopVlanInPktBuf(OvsForwardingContext *ovsFwdCtx)
  * Declare a dummy vlanTag structure since we need to compute the size
  * of shiftLength. The NDIS one is a unionized structure.
  */
+NDIS_STATUS status;
+OVS_PACKET_HDR_INFO* layers = >layers;
 NDIS_PACKET_8021Q_INFO vlanTag = {0};
 UINT32 shiftLength = sizeof(vlanTag.TagHeader);
 UINT32 shiftOffset = sizeof(DL_EUI48) + sizeof(DL_EUI48);

-return OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength, 
NULL);
+status = OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength,
+NULL);
+
+if (status == NDIS_STATUS_SUCCESS) {
+layers->l3Offset -= (UINT16) shiftLength;
+layers->l4Offset -= (UINT16) shiftLength;
+}
+
+return status;
 }


-- 
2.32.0

___
dev mailing list
d...@openvswitch.org

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=04%7C01%7Cpweisong%40vmware.com%7C97a24c5e1c5642cde59d08d98350cbed%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637685205333861058%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=cYplthGDj6Y3JKU33UWegukyXDh7ZehNv6wXVqfWD6s%3Dreserved=0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when processing packet in POP_VLAN action

2021-09-29 Thread Alin-Gabriel Serdean 
From: aserd...@ovn.org

Thank you for raising awareness about this issue. You are right the
offsets are not being updated when a pop vlan action is hit, they are
updated only on pop_mpls.
https://github.com/openvswitch/ovs/blob/master/datapath-windows/ovsext/Actions.c#L1173-L1174
Can you please update the offsets in the corresponding pop vlan
function. I.e.:
---
 datapath-windows/ovsext/Actions.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index e130c2f96..34aa5c5e4 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1141,11 +1141,21 @@ OvsPopVlanInPktBuf(OvsForwardingContext *ovsFwdCtx)
  * Declare a dummy vlanTag structure since we need to compute the size
  * of shiftLength. The NDIS one is a unionized structure.
  */
+NDIS_STATUS status;
+OVS_PACKET_HDR_INFO* layers = >layers;
 NDIS_PACKET_8021Q_INFO vlanTag = {0};
 UINT32 shiftLength = sizeof(vlanTag.TagHeader);
 UINT32 shiftOffset = sizeof(DL_EUI48) + sizeof(DL_EUI48);
 
-return OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength, NULL);
+status = OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength,
+NULL);
+
+if (status == NDIS_STATUS_SUCCESS) {
+layers->l3Offset -= (UINT16) shiftLength;
+layers->l4Offset -= (UINT16) shiftLength;
+}
+
+return status;
 }
 
 
-- 
2.32.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev