[PATCH] app/testpmd: fix GTP header parsing in csum FWD engine

[PATCH] app/testpmd: fix GTP header parsing in csum FWD engine

[Gregory Etelson]

GTP header can be followed by an optional 32 bits extension.
GTP notifies about the extension presence through the E, S or PN
header bits.

Csum GTP header parser did not check the extension bits value.

The patch updates GTP header length if header extension bits are set.

Cc: stable@dpdk.org

Fixes: d8e5e69f3a9b ("app/testpmd: add GTP parsing and Tx checksum offload")
Signed-off-by: Gregory Etelson <getelson@nvidia.com>
---
 app/test-pmd/csumonly.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 5274d498ee..f8abcded2b 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -223,15 +223,14 @@ parse_gtp(struct rte_udp_hdr *udp_hdr,
 
 	gtp_hdr = (struct rte_gtp_hdr *)((char *)udp_hdr +
 		  sizeof(struct rte_udp_hdr));
-
+	if (gtp_hdr->e || gtp_hdr->s || gtp_hdr->pn)
+		gtp_len += sizeof(struct rte_gtp_hdr_ext_word);
 	/*
 	 * Check message type. If message type is 0xff, it is
 	 * a GTP data packet. If not, it is a GTP control packet
 	 */
 	if (gtp_hdr->msg_type == 0xff) {
-		ip_ver = *(uint8_t *)((char *)udp_hdr +
-			 sizeof(struct rte_udp_hdr) +
-			 sizeof(struct rte_gtp_hdr));
+		ip_ver = *(uint8_t *)((char *)gtp_hdr + gtp_len);
 		ip_ver = (ip_ver) & 0xf0;
 
 		if (ip_ver == RTE_GTP_TYPE_IPV4) {
-- 
2.35.1

Re: [PATCH] app/testpmd: fix GTP header parsing in csum FWD engine

[Singh, Aman Deep]

[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]

Looks good to me.

On 3/10/2022 7:50 PM, Gregory Etelson wrote:
> GTP header can be followed by an optional 32 bits extension.
> GTP notifies about the extension presence through the E, S or PN
> header bits.
>
> Csum GTP header parser did not check the extension bits value.
>
> The patch updates GTP header length if header extension bits are set.
Can we rephrase above line "if at-least one of the extension bit is set."
To make it more clear.
>
> Cc:stable@dpdk.org
>
> Fixes: d8e5e69f3a9b ("app/testpmd: add GTP parsing and Tx checksum offload")
> Signed-off-by: Gregory Etelson<getelson@nvidia.com>
Acked-by: Aman Singh <aman.deep.singh@intel.com>
> ---
>   app/test-pmd/csumonly.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index 5274d498ee..f8abcded2b 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -223,15 +223,14 @@ parse_gtp(struct rte_udp_hdr *udp_hdr,
>   
>   	gtp_hdr = (struct rte_gtp_hdr *)((char *)udp_hdr +
>   		  sizeof(struct rte_udp_hdr));
> -
> +	if (gtp_hdr->e || gtp_hdr->s || gtp_hdr->pn)
> +		gtp_len += sizeof(struct rte_gtp_hdr_ext_word);
>   	/*
>   	 * Check message type. If message type is 0xff, it is
>   	 * a GTP data packet. If not, it is a GTP control packet
>   	 */
>   	if (gtp_hdr->msg_type == 0xff) {
> -		ip_ver = *(uint8_t *)((char *)udp_hdr +
> -			 sizeof(struct rte_udp_hdr) +
> -			 sizeof(struct rte_gtp_hdr));
> +		ip_ver = *(uint8_t *)((char *)gtp_hdr + gtp_len);
>   		ip_ver = (ip_ver) & 0xf0;
>   
>   		if (ip_ver == RTE_GTP_TYPE_IPV4) {

[-- Attachment #2: Type: text/html, Size: 2585 bytes --]

RE: [PATCH] app/testpmd: fix GTP header parsing in csum FWD engine

[Gregory Etelson]

[-- Attachment #1: Type: text/plain, Size: 2487 bytes --]

Hello,

From: Singh, Aman Deep <aman.deep.singh@intel.com>
Sent: Friday, March 11, 2022 15:35
To: Gregory Etelson <getelson@nvidia.com>; dev@dpdk.org
Cc: Matan Azrad <matan@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>; stable@dpdk.org; Xiaoyun Li <xiaoyun.li@intel.com>; Yuying Zhang <yuying.zhang@intel.com>; Ting Xu <ting.xu@intel.com>; Ferruh Yigit <ferruh.yigit@intel.com>
Subject: Re: [PATCH] app/testpmd: fix GTP header parsing in csum FWD engine

External email: Use caution opening links or attachments


Looks good to me.
On 3/10/2022 7:50 PM, Gregory Etelson wrote:

GTP header can be followed by an optional 32 bits extension.

GTP notifies about the extension presence through the E, S or PN

header bits.



Csum GTP header parser did not check the extension bits value.



The patch updates GTP header length if header extension bits are set.
Can we rephrase above line "if at-least one of the extension bit is set."
To make it more clear.

[Gregory] I’ll post v2 with updated comment.







Cc: stable@dpdk.org<mailto:stable@dpdk.org>



Fixes: d8e5e69f3a9b ("app/testpmd: add GTP parsing and Tx checksum offload")

Signed-off-by: Gregory Etelson <getelson@nvidia.com><mailto:getelson@nvidia.com>
Acked-by: Aman Singh <aman.deep.singh@intel.com><mailto:aman.deep.singh@intel.com>



---

 app/test-pmd/csumonly.c | 7 +++----

 1 file changed, 3 insertions(+), 4 deletions(-)



diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c

index 5274d498ee..f8abcded2b 100644

--- a/app/test-pmd/csumonly.c

+++ b/app/test-pmd/csumonly.c

@@ -223,15 +223,14 @@ parse_gtp(struct rte_udp_hdr *udp_hdr,



        gtp_hdr = (struct rte_gtp_hdr *)((char *)udp_hdr +

                  sizeof(struct rte_udp_hdr));

-

+       if (gtp_hdr->e || gtp_hdr->s || gtp_hdr->pn)

+               gtp_len += sizeof(struct rte_gtp_hdr_ext_word);

        /*

         * Check message type. If message type is 0xff, it is

         * a GTP data packet. If not, it is a GTP control packet

         */

        if (gtp_hdr->msg_type == 0xff) {

-               ip_ver = *(uint8_t *)((char *)udp_hdr +

-                        sizeof(struct rte_udp_hdr) +

-                        sizeof(struct rte_gtp_hdr));

+               ip_ver = *(uint8_t *)((char *)gtp_hdr + gtp_len);

                ip_ver = (ip_ver) & 0xf0;



                if (ip_ver == RTE_GTP_TYPE_IPV4) {

[-- Attachment #2: Type: text/html, Size: 7927 bytes --]

[PATCH v2] app/testpmd: fix GTP header parsing in csum FWD engine

[Gregory Etelson]

GTP header can be followed by an optional 32 bits extension.
GTP notifies about the extension presence through the E, S or PN
header bits.

Csum GTP header parser did not check the extension bits value.

The patch updates GTP header length if at-least one of the
extension bits is set.

Cc: stable@dpdk.org

Fixes: d8e5e69f3a9b ("app/testpmd: add GTP parsing and Tx checksum offload")
Signed-off-by: Gregory Etelson <getelson@nvidia.com>
Acked-by: Aman Singh <aman.deep.singh@intel.com>
---
v2: Update the patch comment.
---
 app/test-pmd/csumonly.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 5274d498ee..f8abcded2b 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -223,15 +223,14 @@ parse_gtp(struct rte_udp_hdr *udp_hdr,
 
 	gtp_hdr = (struct rte_gtp_hdr *)((char *)udp_hdr +
 		  sizeof(struct rte_udp_hdr));
-
+	if (gtp_hdr->e || gtp_hdr->s || gtp_hdr->pn)
+		gtp_len += sizeof(struct rte_gtp_hdr_ext_word);
 	/*
 	 * Check message type. If message type is 0xff, it is
 	 * a GTP data packet. If not, it is a GTP control packet
 	 */
 	if (gtp_hdr->msg_type == 0xff) {
-		ip_ver = *(uint8_t *)((char *)udp_hdr +
-			 sizeof(struct rte_udp_hdr) +
-			 sizeof(struct rte_gtp_hdr));
+		ip_ver = *(uint8_t *)((char *)gtp_hdr + gtp_len);
 		ip_ver = (ip_ver) & 0xf0;
 
 		if (ip_ver == RTE_GTP_TYPE_IPV4) {
-- 
2.35.1

Re: [PATCH v2] app/testpmd: fix GTP header parsing in csum FWD engine

[Thomas Monjalon]

13/03/2022 10:01, Gregory Etelson:
> GTP header can be followed by an optional 32 bits extension.
> GTP notifies about the extension presence through the E, S or PN
> header bits.
> 
> Csum GTP header parser did not check the extension bits value.
> 
> The patch updates GTP header length if at-least one of the
> extension bits is set.
> 
> Cc: stable@dpdk.org
> 
> Fixes: d8e5e69f3a9b ("app/testpmd: add GTP parsing and Tx checksum offload")
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> Acked-by: Aman Singh <aman.deep.singh@intel.com>

It should be "Fixes" line, "Cc" line, blank line, and "Signed-off/Ack" lines.
Applied, thanks.