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

["Morten Brørup" <mb@smartsharesystems.com>]

[-- 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 --]