Re: [PATCH dpdk v4] net: fix VLAN packet type

[Kevin Traynor <ktraynor@redhat.com>]

On 4/25/26 10:40 AM, David Marchand wrote:
> On Thu, 23 Apr 2026 at 13:25, Robin Jarry <rjarry@redhat.com> wrote:
>> diff --git a/lib/net/rte_net.c b/lib/net/rte_net.c
>> index 458b4814a9c9..a871318b21c2 100644
>> --- a/lib/net/rte_net.c
>> +++ b/lib/net/rte_net.c
>> @@ -357,12 +357,14 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>>                 const struct rte_vlan_hdr *vh;
>>                 struct rte_vlan_hdr vh_copy;
>>
>> +               if (vlan_depth == 0) {
>> +                       pkt_type =
>> +                               proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN) ?
>> +                                        RTE_PTYPE_L2_ETHER_VLAN :
>> +                                        RTE_PTYPE_L2_ETHER_QINQ;
>> +               }
> 
> This code is becoming too complex.
> The original usecase with more than 2 stacked vlan is a bit strange,
> but the max depth limit seems just arbitrary (why 8?).
> We have clear boundaries, with the size of the packet (see below,
> check on vh == NULL).
> 
> The offending commit also allows stacking mpls on top of vlan, without
> mentioning it.
> I think we want this behavior, but still was it intended?
> 
> In the end, reverting the previous fix then just advancing off and
> breaking once proto is not a vlan/qinq type gives a much simpler fix
> when compared to v25.11.
> 
> (ignoring indent changes with -w)
> 
> $ git diff -w v25.11 lib/net/rte_net.c
> diff --git a/lib/net/rte_net.c b/lib/net/rte_net.c
> index c70b57fdc0..f58d699c83 100644
> --- a/lib/net/rte_net.c
> +++ b/lib/net/rte_net.c
> @@ -349,30 +349,28 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>         if (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_IPV4))
>                 goto l3; /* fast path if packet is IPv4 */
> 
> -       if (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN)) {
> +       if (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN) ||
> +                       proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_QINQ)) {
>                 const struct rte_vlan_hdr *vh;
>                 struct rte_vlan_hdr vh_copy;
> 
> +               if (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN))
>                         pkt_type = RTE_PTYPE_L2_ETHER_VLAN;
> +               else
> +                       pkt_type = RTE_PTYPE_L2_ETHER_QINQ;
> +
> +               do {
>                         vh = rte_pktmbuf_read(m, off, sizeof(*vh), &vh_copy);
>                         if (unlikely(vh == NULL))
>                                 return pkt_type;
>                         off += sizeof(*vh);
>                         hdr_lens->l2_len += sizeof(*vh);
>                         proto = vh->eth_proto;
> -       } else if (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_QINQ)) {
> -               const struct rte_vlan_hdr *vh;
> -               struct rte_vlan_hdr vh_copy;
> +               } while (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN) ||
> +                               proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_QINQ));
> +       }
> 
> -               pkt_type = RTE_PTYPE_L2_ETHER_QINQ;
> -               vh = rte_pktmbuf_read(m, off + sizeof(*vh), sizeof(*vh),
> -                       &vh_copy);
> -               if (unlikely(vh == NULL))
> -                       return pkt_type;
> -               off += 2 * sizeof(*vh);
> -               hdr_lens->l2_len += 2 * sizeof(*vh);
> -               proto = vh->eth_proto;
> -       } else if ((proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_MPLS)) ||
> +       if ((proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_MPLS)) ||
>                 (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_MPLSM))) {
>                 unsigned int i;
>                 const struct rte_mpls_hdr *mh;
> 
> 
> This is untested, but what do you think?
> 
> 

This version LGTM. Maybe we should consider returning NULL on invalid
packets