[PATCH] app/testpmd: fix VLAN header parsing

[PATCH] app/testpmd: fix VLAN header parsing

[Raslan Darawsheh]

Updated the `get_ethertype_by_ptype` function to correctly parse VLAN
headers.

Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
Cc: haijie1@huawei.com

Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
---
 app/test-pmd/csumonly.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 5b906eaa53..302cc4cc66 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -468,6 +468,7 @@ get_ethertype_by_ptype(struct rte_ether_hdr *eth_hdr, uint32_t ptype)
 {
 	struct rte_vlan_hdr *vlan_hdr;
 	uint16_t ethertype;
+	uint32_t i = 0;
 
 	switch (ptype) {
 	case RTE_PTYPE_L3_IPV4:
@@ -486,10 +487,11 @@ get_ethertype_by_ptype(struct rte_ether_hdr *eth_hdr, uint32_t ptype)
 		return _htons(RTE_ETHER_TYPE_IPV6);
 	default:
 		ethertype = eth_hdr->ether_type;
-		while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN) ||
-			eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
+		while (ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
+			ethertype == _htons(RTE_ETHER_TYPE_QINQ)) {
 			vlan_hdr = (struct rte_vlan_hdr *)
-				((char *)eth_hdr + sizeof(*eth_hdr));
+				((char *)eth_hdr + sizeof(*eth_hdr) +
+				(i * sizeof(struct rte_vlan_hdr)));
 			ethertype = vlan_hdr->eth_proto;
 		}
 		return ethertype;
-- 
2.39.5 (Apple Git-154)

RE: [PATCH] app/testpmd: fix VLAN header parsing

[Morten Brørup]

> From: Raslan Darawsheh [mailto:rasland@nvidia.com]
> Sent: Sunday, 23 March 2025 13.28
> 
> Updated the `get_ethertype_by_ptype` function to correctly parse VLAN
> headers.
> 
> Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
> Cc: haijie1@huawei.com
> 
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> ---
>  app/test-pmd/csumonly.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index 5b906eaa53..302cc4cc66 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -468,6 +468,7 @@ get_ethertype_by_ptype(struct rte_ether_hdr
> *eth_hdr, uint32_t ptype)
>  {
>  	struct rte_vlan_hdr *vlan_hdr;
>  	uint16_t ethertype;
> +	uint32_t i = 0;
> 
>  	switch (ptype) {
>  	case RTE_PTYPE_L3_IPV4:
> @@ -486,10 +487,11 @@ get_ethertype_by_ptype(struct rte_ether_hdr
> *eth_hdr, uint32_t ptype)
>  		return _htons(RTE_ETHER_TYPE_IPV6);
>  	default:
>  		ethertype = eth_hdr->ether_type;
> -		while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN)
> ||
> -			eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
> +		while (ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
> +			ethertype == _htons(RTE_ETHER_TYPE_QINQ)) {
>  			vlan_hdr = (struct rte_vlan_hdr *)
> -				((char *)eth_hdr + sizeof(*eth_hdr));
> +				((char *)eth_hdr + sizeof(*eth_hdr) +
> +				(i * sizeof(struct rte_vlan_hdr)));

Doesn't work; "i" is not incremented, and remains 0.
Instead do this: Initialize (struct rte_vlan_hdr *)vlan_hdr=RTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, ether_type)) before the loop, and move vlan_hdr+=RTE_VLAN_HLEN inside the loop.

>  			ethertype = vlan_hdr->eth_proto;
>  		}
>  		return ethertype;
> --
> 2.39.5 (Apple Git-154)

Re: [PATCH] app/testpmd: fix VLAN header parsing

[Stephen Hemminger]

On Sun, 23 Mar 2025 14:28:22 +0200
Raslan Darawsheh <rasland@nvidia.com> wrote:

> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> ---
>  app/test-pmd/csumonly.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index 5b906eaa53..302cc4cc66 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -468,6 +468,7 @@ get_ethertype_by_ptype(struct rte_ether_hdr *eth_hdr, uint32_t ptype)
>  {
>  	struct rte_vlan_hdr *vlan_hdr;
>  	uint16_t ethertype;
> +	uint32_t i = 0;
>  
>  	switch (ptype) {
>  	case RTE_PTYPE_L3_IPV4:
> @@ -486,10 +487,11 @@ get_ethertype_by_ptype(struct rte_ether_hdr *eth_hdr, uint32_t ptype)
>  		return _htons(RTE_ETHER_TYPE_IPV6);
>  	default:
>  		ethertype = eth_hdr->ether_type;
> -		while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN) ||
> -			eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
> +		while (ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
> +			ethertype == _htons(RTE_ETHER_TYPE_QINQ)) {
>  			vlan_hdr = (struct rte_vlan_hdr *)
> -				((char *)eth_hdr + sizeof(*eth_hdr));
> +				((char *)eth_hdr + sizeof(*eth_hdr) +
> +				(i * sizeof(struct rte_vlan_hdr)));
>  			ethertype = vlan_hdr->eth_proto;
>  		}
>  		return ethertype;

A loop like this is prone to getting attacked with a malicious packet.
You should cut it off after a few vlan headers.
Also. what if packet is truncated, shouldn't be reading past end of data.
And what if packet is fragmented, you need to use rte_pktmbuf_read()

Re: [PATCH] app/testpmd: fix VLAN header parsing

[Thomas Monjalon]

23/03/2025 13:28, Raslan Darawsheh:
> Updated the `get_ethertype_by_ptype` function to correctly parse VLAN
> headers.

You should describe what is broken and how.

> Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
> Cc: haijie1@huawei.com

Do you think it is critical for the release?

Re: [PATCH] app/testpmd: fix VLAN header parsing

[Raslan Darawsheh]

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

>> From: Raslan Darawsheh [mailto:rasland@nvidia.com]
>> Sent: Sunday, 23 March 2025 13.28
>>
>> Updated the `get_ethertype_by_ptype` function to correctly parse VLAN
>> headers.
>>
>> Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
>> Cc: haijie1@huawei.com
>>
>>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
>> ---
>>  app/test-pmd/csumonly.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> index 5b906eaa53..302cc4cc66 100644
>> --- a/app/test-pmd/csumonly.c
>> +++ b/app/test-pmd/csumonly.c
>> @@ -468,6 +468,7 @@ get_ethertype_by_ptype(struct rte_ether_hdr
>> *eth_hdr, uint32_t ptype)
>>  {
>>        struct rte_vlan_hdr *vlan_hdr;
>>        uint16_t ethertype;
>> +     uint32_t i = 0;
>>
>>        switch (ptype) {
>>        case RTE_PTYPE_L3_IPV4:
>> @@ -486,10 +487,11 @@ get_ethertype_by_ptype(struct rte_ether_hdr
>> *eth_hdr, uint32_t ptype)
>>                return _htons(RTE_ETHER_TYPE_IPV6);
>>        default:
>>                ethertype = eth_hdr->ether_type;
>> -             while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN)
>> ||
>> -                     eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
>> +             while (ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
>> +                     ethertype == _htons(RTE_ETHER_TYPE_QINQ)) {
>>                        vlan_hdr = (struct rte_vlan_hdr *)
>> -                             ((char *)eth_hdr + sizeof(*eth_hdr));
>> +                             ((char *)eth_hdr + sizeof(*eth_hdr) +
>> +                             (i * sizeof(struct rte_vlan_hdr)));

>Doesn't work; "i" is not incremented, and remains 0.
>Instead do this: Initialize (struct rte_vlan_hdr *)vlan_hdr=RTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, ether_type)) before the loop, and move vlan_hdr+=RTE_VLAN_HLEN inside the loop.
Yes you are correct I had a (i++ *) but must have slipped while rebasing will handle your way in v2
>>                        ethertype = vlan_hdr->eth_proto;
>>                }
>>                return ethertype;
>> --
>> 2.39.5 (Apple Git-154)

Kindest regards
Raslan Darawsheh

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

Re: [PATCH] app/testpmd: fix VLAN header parsing

[Raslan Darawsheh]

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


>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
>> ---
>>  app/test-pmd/csumonly.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> index 5b906eaa53..302cc4cc66 100644
>> --- a/app/test-pmd/csumonly.c
>> +++ b/app/test-pmd/csumonly.c
>> @@ -468,6 +468,7 @@ get_ethertype_by_ptype(struct rte_ether_hdr *eth_hdr, uint32_t ptype)
>>  {
>>        struct rte_vlan_hdr *vlan_hdr;
>>        uint16_t ethertype;
>> +     uint32_t i = 0;
>>
>>        switch (ptype) {
>>        case RTE_PTYPE_L3_IPV4:
>> @@ -486,10 +487,11 @@ get_ethertype_by_ptype(struct rte_ether_hdr *eth_hdr, uint32_t ptype)
>>                return _htons(RTE_ETHER_TYPE_IPV6);
>>        default:
>>                ethertype = eth_hdr->ether_type;
>> -             while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN) ||
>> -                     eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
>> +             while (ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
>> +                     ethertype == _htons(RTE_ETHER_TYPE_QINQ)) {
>>                        vlan_hdr = (struct rte_vlan_hdr *)
>> -                             ((char *)eth_hdr + sizeof(*eth_hdr));
v> +                             ((char *)eth_hdr + sizeof(*eth_hdr) +
v> +                             (i * sizeof(struct rte_vlan_hdr)));
>>                        ethertype = vlan_hdr->eth_proto;
>>                }
>>                return ethertype;

>A loop like this is prone to getting attacked with a malicious packet.
>You should cut it off after a few vlan headers.
>Also. what if packet is truncated, shouldn't be reading past end of data.
>And what if packet is fragmented, you need to use rte_pktmbuf_read()
I’m trying to fix the current loop not really changing the logic, and I’m not sure we handled these cases originally.
If needed, we can issue a separate patch for fixing these cases.

Kindest regards
Raslan Darawsheh

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

Re: [PATCH] app/testpmd: fix VLAN header parsing

[Raslan Darawsheh]

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


>> Updated the `get_ethertype_by_ptype` function to correctly parse VLAN
>> headers.

>You should describe what is broken and how.
I’ll handle in v2

>> Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
>> Cc: haijie1@huawei.com

>Do you think it is critical for the release?
Yes, as it’s causing an infinite loop if you send a QinQ packet with csum fwd, and it will cause tested to get stuck forever.



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

RE: [PATCH] app/testpmd: fix VLAN header parsing

[Morten Brørup]

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

Comment inline below.

 

From: Raslan Darawsheh [mailto:rasland@nvidia.com] 
Sent: Monday, 24 March 2025 08.35



 

>> From: Raslan Darawsheh [mailto:rasland@nvidia.com]
>> Sent: Sunday, 23 March 2025 13.28
>> 
>> Updated the `get_ethertype_by_ptype` function to correctly parse VLAN
>> headers.
>> 
>> Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
>> Cc: haijie1@huawei.com
>> 
>>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
>> ---
>>  app/test-pmd/csumonly.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> index 5b906eaa53..302cc4cc66 100644
>> --- a/app/test-pmd/csumonly.c
>> +++ b/app/test-pmd/csumonly.c
>> @@ -468,6 +468,7 @@ get_ethertype_by_ptype(struct rte_ether_hdr
>> *eth_hdr, uint32_t ptype)
>>  {
>>        struct rte_vlan_hdr *vlan_hdr;
>>        uint16_t ethertype;
>> +     uint32_t i = 0;
>> 
>>        switch (ptype) {
>>        case RTE_PTYPE_L3_IPV4:
>> @@ -486,10 +487,11 @@ get_ethertype_by_ptype(struct rte_ether_hdr
>> *eth_hdr, uint32_t ptype)
>>                return _htons(RTE_ETHER_TYPE_IPV6);
>>        default:
>>                ethertype = eth_hdr->ether_type;
>> -             while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN)
>> ||
>> -                     eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
>> +             while (ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
>> +                     ethertype == _htons(RTE_ETHER_TYPE_QINQ)) {
>>                        vlan_hdr = (struct rte_vlan_hdr *)
>> -                             ((char *)eth_hdr + sizeof(*eth_hdr));
>> +                             ((char *)eth_hdr + sizeof(*eth_hdr) +
>> +                             (i * sizeof(struct rte_vlan_hdr)));

>Doesn't work; "i" is not incremented, and remains 0.
>Instead do this: Initialize (struct rte_vlan_hdr *)vlan_hdr=RTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, ether_type)) before the loop, and move vlan_hdr+=RTE_VLAN_HLEN inside the loop.
Yes you are correct I had a (i++ *) but must have slipped while rebasing will handle your way in v2 

MB: Noticed a bug in my suggestion: Due to the type of vlan_hdr, moving it should be vlan_hdr++, not vlan_hdr+=RTE_VLAN_HLEN.

MB: And you might want to impose a maximum boundary to the loop, as suggested by Stephen. E.g. break out of the loop if vlan_hdr>RTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, ether_type))+4*sizeof(struct rte_vlan_hdr). Supporting up to 4 stacked VLAN tags should suffice.


>>                        ethertype = vlan_hdr->eth_proto;
>>                }
>>                return ethertype;
>> --
>> 2.39.5 (Apple Git-154)

 

Kindest regards

Raslan Darawsheh


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

RE: [PATCH] app/testpmd: fix VLAN header parsing

[Morten Brørup]

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

Comment inline below.

 

From: Raslan Darawsheh [mailto:rasland@nvidia.com] 
Sent: Monday, 24 March 2025 08.35

 

>> From: Raslan Darawsheh [mailto:rasland@nvidia.com]
>> Sent: Sunday, 23 March 2025 13.28
>> 
>> Updated the `get_ethertype_by_ptype` function to correctly parse VLAN
>> headers.
>> 
>> Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
>> Cc: haijie1@huawei.com
>> 
>>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
>> ---
>>  app/test-pmd/csumonly.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> index 5b906eaa53..302cc4cc66 100644
>> --- a/app/test-pmd/csumonly.c
>> +++ b/app/test-pmd/csumonly.c
>> @@ -468,6 +468,7 @@ get_ethertype_by_ptype(struct rte_ether_hdr
>> *eth_hdr, uint32_t ptype)
>>  {
>>        struct rte_vlan_hdr *vlan_hdr;
>>        uint16_t ethertype;
>> +     uint32_t i = 0;
>> 
>>        switch (ptype) {
>>        case RTE_PTYPE_L3_IPV4:
>> @@ -486,10 +487,11 @@ get_ethertype_by_ptype(struct rte_ether_hdr
>> *eth_hdr, uint32_t ptype)
>>                return _htons(RTE_ETHER_TYPE_IPV6);
>>        default:
>>                ethertype = eth_hdr->ether_type;
>> -             while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN)
>> ||
>> -                     eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
>> +             while (ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
>> +                     ethertype == _htons(RTE_ETHER_TYPE_QINQ)) {
>>                        vlan_hdr = (struct rte_vlan_hdr *)
>> -                             ((char *)eth_hdr + sizeof(*eth_hdr));
>> +                             ((char *)eth_hdr + sizeof(*eth_hdr) +
>> +                             (i * sizeof(struct rte_vlan_hdr)));

>Doesn't work; "i" is not incremented, and remains 0.
>Instead do this: Initialize (struct rte_vlan_hdr *)vlan_hdr=RTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, ether_type)) before the loop, and move vlan_hdr+=RTE_VLAN_HLEN inside the loop.
Yes you are correct I had a (i++ *) but must have slipped while rebasing will handle your way in v2 

MB: Noticed a bug in my suggestion: Due to the type of vlan_hdr, moving it should be vlan_hdr++, not vlan_hdr+=RTE_VLAN_HLEN.

MB: And you might want to impose a maximum boundary to the loop, as suggested by Stephen. E.g. break out of the loop if vlan_hdr>RTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, ether_type)+4*sizeof(struct rte_vlan_hdr)). Supporting up to 4 stacked VLAN tags should suffice.


>>                        ethertype = vlan_hdr->eth_proto;
>>                }
>>                return ethertype;
>> --
>> 2.39.5 (Apple Git-154)

 

Kindest regards

Raslan Darawsheh


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

Re: [PATCH] app/testpmd: fix VLAN header parsing

[Raslan Darawsheh]

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

Comment inline below.

From: Raslan Darawsheh [mailto:rasland@nvidia.com]
Sent: Monday, 24 March 2025 08.35

>> From: Raslan Darawsheh [mailto:rasland@nvidia.com]
>> Sent: Sunday, 23 March 2025 13.28
>>
>> Updated the `get_ethertype_by_ptype` function to correctly parse VLAN
>> headers.
>>
>> Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
>> Cc: haijie1@huawei.com
>>
>>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
>> ---
>>  app/test-pmd/csumonly.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> index 5b906eaa53..302cc4cc66 100644
>> --- a/app/test-pmd/csumonly.c
>> +++ b/app/test-pmd/csumonly.c
>> @@ -468,6 +468,7 @@ get_ethertype_by_ptype(struct rte_ether_hdr
>> *eth_hdr, uint32_t ptype)
>>  {
>>        struct rte_vlan_hdr *vlan_hdr;
>>        uint16_t ethertype;
>> +     uint32_t i = 0;
>>
>>        switch (ptype) {
>>        case RTE_PTYPE_L3_IPV4:
>> @@ -486,10 +487,11 @@ get_ethertype_by_ptype(struct rte_ether_hdr
>> *eth_hdr, uint32_t ptype)
>>                return _htons(RTE_ETHER_TYPE_IPV6);
>>        default:
>>                ethertype = eth_hdr->ether_type;
>> -             while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN)
>> ||
>> -                     eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
>> +             while (ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
>> +                     ethertype == _htons(RTE_ETHER_TYPE_QINQ)) {
>>                        vlan_hdr = (struct rte_vlan_hdr *)
>> -                             ((char *)eth_hdr + sizeof(*eth_hdr));
>> +                             ((char *)eth_hdr + sizeof(*eth_hdr) +
>> +                             (i * sizeof(struct rte_vlan_hdr)));

>Doesn't work; "i" is not incremented, and remains 0.
>Instead do this: Initialize (struct rte_vlan_hdr *)vlan_hdr=RTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, ether_type)) before the loop, and move vlan_hdr+=RTE_VLAN_HLEN inside the loop.
Yes you are correct I had a (i++ *) but must have slipped while rebasing will handle your way in v2
MB: Noticed a bug in my suggestion: Due to the type of vlan_hdr, moving it should be vlan_hdr++, not vlan_hdr+=RTE_VLAN_HLEN.
                 Yes sure will handle,
MB: And you might want to impose a maximum boundary to the loop, as suggested by Stephen. E.g. break out of the loop if vlan_hdr>RTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, ether_type)+4*sizeof(struct rte_vlan_hdr)). Supporting up to 4 stacked VLAN tags should suffice.
              I’m not sure this should be added to this patch, as I tend to believe it’s less critical to handle for this release, and also, this might cause inconsistent results, think of the case if we send five stacked vlan tags, then we’ll return ethertype as VLAN as we skipped the last one.

>>                        ethertype = vlan_hdr->eth_proto;
>>                }
>>                return ethertype;
>> --
>> 2.39.5 (Apple Git-154)

Kindest regards
Raslan Darawsheh

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

RE: [PATCH] app/testpmd: fix VLAN header parsing

[Morten Brørup]

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

Answer inline below.

 

From: Raslan Darawsheh [mailto:rasland@nvidia.com] 
Sent: Monday, 24 March 2025 09.32



Comment inline below.

 

From: Raslan Darawsheh [mailto:rasland@nvidia.com] 
Sent: Monday, 24 March 2025 08.35

 

>> From: Raslan Darawsheh [mailto:rasland@nvidia.com]
>> Sent: Sunday, 23 March 2025 13.28
>> 
>> Updated the `get_ethertype_by_ptype` function to correctly parse VLAN
>> headers.
>> 
>> Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
>> Cc: haijie1@huawei.com
>> 
>>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
>> ---
>>  app/test-pmd/csumonly.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> index 5b906eaa53..302cc4cc66 100644
>> --- a/app/test-pmd/csumonly.c
>> +++ b/app/test-pmd/csumonly.c
>> @@ -468,6 +468,7 @@ get_ethertype_by_ptype(struct rte_ether_hdr
>> *eth_hdr, uint32_t ptype)
>>  {
>>        struct rte_vlan_hdr *vlan_hdr;
>>        uint16_t ethertype;
>> +     uint32_t i = 0;
>> 
>>        switch (ptype) {
>>        case RTE_PTYPE_L3_IPV4:
>> @@ -486,10 +487,11 @@ get_ethertype_by_ptype(struct rte_ether_hdr
>> *eth_hdr, uint32_t ptype)
>>                return _htons(RTE_ETHER_TYPE_IPV6);
>>        default:
>>                ethertype = eth_hdr->ether_type;
>> -             while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN)
>> ||
>> -                     eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
>> +             while (ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
>> +                     ethertype == _htons(RTE_ETHER_TYPE_QINQ)) {
>>                        vlan_hdr = (struct rte_vlan_hdr *)
>> -                             ((char *)eth_hdr + sizeof(*eth_hdr));
>> +                             ((char *)eth_hdr + sizeof(*eth_hdr) +
>> +                             (i * sizeof(struct rte_vlan_hdr)));

>Doesn't work; "i" is not incremented, and remains 0.
>Instead do this: Initialize (struct rte_vlan_hdr *)vlan_hdr=RTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, ether_type)) before the loop, and move vlan_hdr+=RTE_VLAN_HLEN inside the loop.
Yes you are correct I had a (i++ *) but must have slipped while rebasing will handle your way in v2 

MB: Noticed a bug in my suggestion: Due to the type of vlan_hdr, moving it should be vlan_hdr++, not vlan_hdr+=RTE_VLAN_HLEN.

                 Yes sure will handle, 

MB: And you might want to impose a maximum boundary to the loop, as suggested by Stephen. E.g. break out of the loop if vlan_hdr>RTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, ether_type)+4*sizeof(struct rte_vlan_hdr)). Supporting up to 4 stacked VLAN tags should suffice.

              I'm not sure this should be added to this patch, as I tend to believe it's less critical to handle for this release, and also, this might cause inconsistent results, think of the case if we send five stacked vlan tags, then we'll return ethertype as VLAN as we skipped the last one.

MB:

It should go in this patch, to prevent code analysis tools from warning about unbounded loop causing potential buffer overrun - the bug described by Stephen.

IMO, a stack of 4 VLAN tags should suffice, but you can make it 8 to give it some extra margin. More than 8 VLAN tags is never going to happen with real traffic.

In reality, most NICs can only strip 1 or 2 VLAN tags, and would return ethertype as VLAN. But this can be ignored for this patch.


>>                        ethertype = vlan_hdr->eth_proto;
>>                }
>>                return ethertype;
>> --
>> 2.39.5 (Apple Git-154)

 

Kindest regards

Raslan Darawsheh


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

[PATCH v2] app/testpmd: fix VLAN header parsing

[Raslan Darawsheh]

When processing VLAN or QinQ packets, an infinite loop occurred,
preventing the csum forward engine from proceeding and causing
testpmd to hang during shutdown attempts.

Updated the `get_ethertype_by_ptype` function to correctly parse
VLAN headers.

Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
Cc: haijie1@huawei.com

Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
---
v2: update commit log
    fixed pointer handling to actually move the pointer
---
 app/test-pmd/csumonly.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 5b906eaa53..7bd1040c72 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -486,10 +486,11 @@ get_ethertype_by_ptype(struct rte_ether_hdr *eth_hdr, uint32_t ptype)
 		return _htons(RTE_ETHER_TYPE_IPV6);
 	default:
 		ethertype = eth_hdr->ether_type;
-		while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN) ||
-			eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
-			vlan_hdr = (struct rte_vlan_hdr *)
-				((char *)eth_hdr + sizeof(*eth_hdr));
+		vlan_hdr = (struct rte_vlan_hdr *) RTE_PTR_ADD(eth_hdr,
+				 offsetof(struct rte_ether_hdr, ether_type));
+		while ((ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
+			ethertype == _htons(RTE_ETHER_TYPE_QINQ))) {
+			vlan_hdr++;
 			ethertype = vlan_hdr->eth_proto;
 		}
 		return ethertype;
-- 
2.39.5 (Apple Git-154)

RE: [PATCH v2] app/testpmd: fix VLAN header parsing

[Morten Brørup]

> From: Raslan Darawsheh [mailto:rasland@nvidia.com]
> Sent: Monday, 24 March 2025 09.50
> 
> When processing VLAN or QinQ packets, an infinite loop occurred,
> preventing the csum forward engine from proceeding and causing
> testpmd to hang during shutdown attempts.
> 
> Updated the `get_ethertype_by_ptype` function to correctly parse
> VLAN headers.
> 
> Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
> Cc: haijie1@huawei.com
> 
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> ---
> v2: update commit log
>     fixed pointer handling to actually move the pointer
> ---
>  app/test-pmd/csumonly.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index 5b906eaa53..7bd1040c72 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -486,10 +486,11 @@ get_ethertype_by_ptype(struct rte_ether_hdr
> *eth_hdr, uint32_t ptype)
>  		return _htons(RTE_ETHER_TYPE_IPV6);
>  	default:
>  		ethertype = eth_hdr->ether_type;
> -		while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN)
> ||
> -			eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
> -			vlan_hdr = (struct rte_vlan_hdr *)
> -				((char *)eth_hdr + sizeof(*eth_hdr));
> +		vlan_hdr = (struct rte_vlan_hdr *) RTE_PTR_ADD(eth_hdr,
> +				 offsetof(struct rte_ether_hdr, ether_type));
> +		while ((ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
> +			ethertype == _htons(RTE_ETHER_TYPE_QINQ))) {

Two details:
1. Lines continuing on the next line must be indented by two TABs.
2. Please add the boundary check, as requested by Stephen. (I realize this mail may have crossed my response arguing why it is required.)

> +			vlan_hdr++;
>  			ethertype = vlan_hdr->eth_proto;
>  		}
>  		return ethertype;
> --
> 2.39.5 (Apple Git-154)

[PATCH v3] app/testpmd: fix VLAN header parsing

[Raslan Darawsheh]

When processing VLAN or QinQ packets, an infinite loop occurred,
preventing the csum forward engine from proceeding and causing
testpmd to hang during shutdown attempts.

Updated the `get_ethertype_by_ptype` function to correctly parse
VLAN headers.

Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
Cc: haijie1@huawei.com

Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
---
v3: fixed wrong indentation
    add boundaries check for 8 VLANs at max
 
v2: update commit log
    fixed pointer handling to actually move the pointer
---
 app/test-pmd/csumonly.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 5b906eaa53..710967ca4d 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -59,6 +59,8 @@
 #define GRE_SUPPORTED_FIELDS	(GRE_CHECKSUM_PRESENT | GRE_KEY_PRESENT |\
 				 GRE_SEQUENCE_PRESENT)
 
+#define MAX_VLAN_HEADERS 8
+
 /* We cannot use rte_cpu_to_be_16() on a constant in a switch/case */
 #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
 #define _htons(x) ((uint16_t)((((x) & 0x00ffU) << 8) | (((x) & 0xff00U) >> 8)))
@@ -466,9 +468,10 @@ pkts_ip_csum_recalc(struct rte_mbuf **pkts_burst, const uint16_t nb_pkts, uint64
 static uint32_t
 get_ethertype_by_ptype(struct rte_ether_hdr *eth_hdr, uint32_t ptype)
 {
-	struct rte_vlan_hdr *vlan_hdr;
+	struct rte_vlan_hdr *vlan_hdr, *max_vlans;
 	uint16_t ethertype;
 
+
 	switch (ptype) {
 	case RTE_PTYPE_L3_IPV4:
 	case RTE_PTYPE_L3_IPV4_EXT:
@@ -486,10 +489,13 @@ get_ethertype_by_ptype(struct rte_ether_hdr *eth_hdr, uint32_t ptype)
 		return _htons(RTE_ETHER_TYPE_IPV6);
 	default:
 		ethertype = eth_hdr->ether_type;
-		while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN) ||
-			eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
-			vlan_hdr = (struct rte_vlan_hdr *)
-				((char *)eth_hdr + sizeof(*eth_hdr));
+		vlan_hdr = (struct rte_vlan_hdr *) RTE_PTR_ADD(eth_hdr,
+					offsetof(struct rte_ether_hdr, ether_type));
+		max_vlans = vlan_hdr + MAX_VLAN_HEADERS;
+		while (((ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
+			ethertype == _htons(RTE_ETHER_TYPE_QINQ))) &&
+			vlan_hdr < max_vlans) {
+			vlan_hdr++;
 			ethertype = vlan_hdr->eth_proto;
 		}
 		return ethertype;
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2] app/testpmd: fix VLAN header parsing

[Raslan Darawsheh]

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

>> -                     eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
>> -                     vlan_hdr = (struct rte_vlan_hdr *)
>> -                             ((char *)eth_hdr + sizeof(*eth_hdr));
>> +             vlan_hdr = (struct rte_vlan_hdr *) RTE_PTR_ADD(eth_hdr,
>> +                              offsetof(struct rte_ether_hdr, ether_type));
>> +             while ((ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
>> +                     ethertype == _htons(RTE_ETHER_TYPE_QINQ))) {

>Two details:
>1. Lines continuing on the next line must be indented by two TABs.
>2. Please add the boundary check, as requested by Stephen. (I realize this mail may have crossed my response arguing why it is required.)

Sure, I’ve sent a V3 having these fixed thanks for your review and comments.

>> +                     vlan_hdr++;
>>                        ethertype = vlan_hdr->eth_proto;
>>                }
>>                return ethertype;
>> --
>> 2.39.5 (Apple Git-154)

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

RE: [PATCH v3] app/testpmd: fix VLAN header parsing

[Morten Brørup]

> From: Raslan Darawsheh [mailto:rasland@nvidia.com]
> Sent: Monday, 24 March 2025 10.34
> 
> When processing VLAN or QinQ packets, an infinite loop occurred,
> preventing the csum forward engine from proceeding and causing
> testpmd to hang during shutdown attempts.
> 
> Updated the `get_ethertype_by_ptype` function to correctly parse
> VLAN headers.
> 
> Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
> Cc: haijie1@huawei.com
> 
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> ---
> v3: fixed wrong indentation
>     add boundaries check for 8 VLANs at max
> 
> v2: update commit log
>     fixed pointer handling to actually move the pointer
> ---
>  app/test-pmd/csumonly.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index 5b906eaa53..710967ca4d 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -59,6 +59,8 @@
>  #define GRE_SUPPORTED_FIELDS	(GRE_CHECKSUM_PRESENT | GRE_KEY_PRESENT
> |\
>  				 GRE_SEQUENCE_PRESENT)
> 
> +#define MAX_VLAN_HEADERS 8
> +
>  /* We cannot use rte_cpu_to_be_16() on a constant in a switch/case */
>  #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
>  #define _htons(x) ((uint16_t)((((x) & 0x00ffU) << 8) | (((x) &
> 0xff00U) >> 8)))
> @@ -466,9 +468,10 @@ pkts_ip_csum_recalc(struct rte_mbuf **pkts_burst,
> const uint16_t nb_pkts, uint64
>  static uint32_t
>  get_ethertype_by_ptype(struct rte_ether_hdr *eth_hdr, uint32_t ptype)
>  {
> -	struct rte_vlan_hdr *vlan_hdr;
> +	struct rte_vlan_hdr *vlan_hdr, *max_vlans;
>  	uint16_t ethertype;
> 
> +

Please omit adding an extra empty line.

>  	switch (ptype) {
>  	case RTE_PTYPE_L3_IPV4:
>  	case RTE_PTYPE_L3_IPV4_EXT:
> @@ -486,10 +489,13 @@ get_ethertype_by_ptype(struct rte_ether_hdr
> *eth_hdr, uint32_t ptype)
>  		return _htons(RTE_ETHER_TYPE_IPV6);
>  	default:
>  		ethertype = eth_hdr->ether_type;
> -		while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN)
> ||
> -			eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
> -			vlan_hdr = (struct rte_vlan_hdr *)
> -				((char *)eth_hdr + sizeof(*eth_hdr));
> +		vlan_hdr = (struct rte_vlan_hdr *) RTE_PTR_ADD(eth_hdr,
> +					offsetof(struct rte_ether_hdr,
> ether_type));

Above line indentation with 2, not 3 TABs.

> +		max_vlans = vlan_hdr + MAX_VLAN_HEADERS;
> +		while (((ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||

Superfluous parenthesis: "while ( ((x || y)) && z)".

> +			ethertype == _htons(RTE_ETHER_TYPE_QINQ))) &&
> +			vlan_hdr < max_vlans) {

Above two lines indentation with 2, not 1 TAB.

> +			vlan_hdr++;
>  			ethertype = vlan_hdr->eth_proto;
>  		}
>  		return ethertype;
> --
> 2.39.5 (Apple Git-154)

With above nits fixed (can be fixed when merging),
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH v3] app/testpmd: fix VLAN header parsing

[Thomas Monjalon]

24/03/2025 14:18, Morten Brørup:
> > From: Raslan Darawsheh [mailto:rasland@nvidia.com]
> > Sent: Monday, 24 March 2025 10.34
> > 
> > When processing VLAN or QinQ packets, an infinite loop occurred,
> > preventing the csum forward engine from proceeding and causing
> > testpmd to hang during shutdown attempts.
> > 
> > Updated the `get_ethertype_by_ptype` function to correctly parse
> > VLAN headers.
> > 
> > Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
> > Cc: haijie1@huawei.com
> > 
> > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> > ---
> > v3: fixed wrong indentation
> >     add boundaries check for 8 VLANs at max
> > 
> > v2: update commit log
> >     fixed pointer handling to actually move the pointer
> > ---
> >  app/test-pmd/csumonly.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> > index 5b906eaa53..710967ca4d 100644
> > --- a/app/test-pmd/csumonly.c
> > +++ b/app/test-pmd/csumonly.c
> > @@ -59,6 +59,8 @@
> >  #define GRE_SUPPORTED_FIELDS	(GRE_CHECKSUM_PRESENT | GRE_KEY_PRESENT
> > |\
> >  				 GRE_SEQUENCE_PRESENT)
> > 
> > +#define MAX_VLAN_HEADERS 8
> > +
> >  /* We cannot use rte_cpu_to_be_16() on a constant in a switch/case */
> >  #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> >  #define _htons(x) ((uint16_t)((((x) & 0x00ffU) << 8) | (((x) &
> > 0xff00U) >> 8)))
> > @@ -466,9 +468,10 @@ pkts_ip_csum_recalc(struct rte_mbuf **pkts_burst,
> > const uint16_t nb_pkts, uint64
> >  static uint32_t
> >  get_ethertype_by_ptype(struct rte_ether_hdr *eth_hdr, uint32_t ptype)
> >  {
> > -	struct rte_vlan_hdr *vlan_hdr;
> > +	struct rte_vlan_hdr *vlan_hdr, *max_vlans;
> >  	uint16_t ethertype;
> > 
> > +
> 
> Please omit adding an extra empty line.
> 
> >  	switch (ptype) {
> >  	case RTE_PTYPE_L3_IPV4:
> >  	case RTE_PTYPE_L3_IPV4_EXT:
> > @@ -486,10 +489,13 @@ get_ethertype_by_ptype(struct rte_ether_hdr
> > *eth_hdr, uint32_t ptype)
> >  		return _htons(RTE_ETHER_TYPE_IPV6);
> >  	default:
> >  		ethertype = eth_hdr->ether_type;
> > -		while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN)
> > ||
> > -			eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
> > -			vlan_hdr = (struct rte_vlan_hdr *)
> > -				((char *)eth_hdr + sizeof(*eth_hdr));
> > +		vlan_hdr = (struct rte_vlan_hdr *) RTE_PTR_ADD(eth_hdr,

useless type cast

> > +					offsetof(struct rte_ether_hdr,
> > ether_type));
> 
> Above line indentation with 2, not 3 TABs.
> 
> > +		max_vlans = vlan_hdr + MAX_VLAN_HEADERS;
> > +		while (((ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
> 
> Superfluous parenthesis: "while ( ((x || y)) && z)".
> 
> > +			ethertype == _htons(RTE_ETHER_TYPE_QINQ))) &&
> > +			vlan_hdr < max_vlans) {
> 
> Above two lines indentation with 2, not 1 TAB.
> 
> > +			vlan_hdr++;
> >  			ethertype = vlan_hdr->eth_proto;
> >  		}
> >  		return ethertype;
> > --
> > 2.39.5 (Apple Git-154)
> 
> With above nits fixed (can be fixed when merging),
> Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

All nits fixed and applied, thanks.