On Mon, 3 Nov 2025 17:41:27 +0200
Gregory Etelson <[email protected]> wrote:
> The rte_net_get_ptype() supported only 2 types of VLAN headers frames
> that are defined in the IEEE standards 802.1Q and 802.1ad:
>
> frames with a single 0x8100 VLAN header:
> eth type VLAN / vlan / [IPv4 | IPv6 ]
>
> frames with 0x88A8 QinQ header followed by 0x8100 VLAN:
> eth type QinQ / vlan type VLAN / vlan / [IPv4 | IPv6 ]
>
> The function did not parse frames where VLAN headers were stacked in
> different configurations.
> Such frames should also be allowed to provide HW vendor flexibility.
> As a result, ptype bitmask and header length returned from
> rte_net_get_ptype() for a custom VLAN frame were wrong.
>
> For example, the parser result for the frame
> eth type QinQ / vlan type QinQ / vlan type VLAN / vlan / ipv4
> was:
> pkt_type:=0x120007
> RTE_PTYPE_L2_ETHER_QINQ 0x00000007 OK
> RTE_PTYPE_INNER_L2_ETHER_VLAN 0x00020000 wrong
> RTE_PTYPE_INNER_L3_IPV4 0x00100000 wrong
>
> hdr_lens:={
> l2_len = 22 wrong
> inner_l2_len = 4 wrong
> l3_len = 0 wrong
> inner_l3_len = 20 wrong
> }
>
> The patch changes:
> 1. Allow frames with an arbitrary number of VLAN headers.
>
> 2. Set each parsed VLAN type in the returned ptype bitmask.
> Multiple VLAN headers are referenced by a single
> RTE_PTYPE_L2_ETHER_VLAN bit.
> Multiple QinQ headers are references by a single
> RTE_PTYPE_L2_ETHER_QINQ bit.
>
> 3. Preserve RTE_PTYPE_L2_ETHER bit if VLAN or QinQ type was detected.
>
> Fixes: eb173c8def0a ("net: support VLAN in software packet type parser")
> Fixes: 218a163efd67 ("net: support QinQ in software packet type parser")
>
> Cc: [email protected]
>
> Signed-off-by: Gregory Etelson <[email protected]>
This patch seems to have been unreviewed for some time so forwarded
to AI for review. Agree in general but my take is that a stacking
depth limit should be enforced; too many network products have had
CVE's for malformed nasty packets.
Please address review comments and resubmit
# DPDK Patch Review: net: fix parsing of frames with arbitrary VLAN stacks
**Patch:** net-fix-parsing-of-frames-with-arbitrary-VLAN-stacks.patch
**Author:** Gregory Etelson <[email protected]>
**Patchwork ID:** 158350
**Reviewer:** Stephen Hemminger
**Date:** January 14, 2026
---
## Summary
This patch fixes `rte_net_get_ptype()` to handle arbitrary VLAN header stacks
instead of only the two standard configurations (single VLAN, QinQ+VLAN). The
fix is correct and well-motivated.
---
## Commit Message Checklist
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✓ | 50 characters |
| Lowercase after colon | ✓ | |
| Correct prefix | ✓ | `net:` correct for `lib/net/` |
| Imperative mood | ✓ | "fix parsing" |
| No trailing period | ✓ | |
| Body ≤75 chars | ✓ | Lines within limit |
| Body doesn't start with "It" | ✓ | Starts with "The" |
| Fixes: tags | ✓ | Two fixes with 12-char SHA |
| Cc: [email protected] | ✓ | Present |
| Signed-off-by | ✓ | Real name and email |
| Tag order | ✓ | Fixes → Cc → Signed-off-by |
**Commit message is well-written** — good problem description with concrete
examples showing incorrect vs expected behavior.
---
## Code Review
### Correctness: ✓ Good
The refactoring is sound:
- Replaces fragile if/else with a `while` loop that iterates through any
VLAN/QinQ stack
- Uses `|=` instead of `=` to accumulate ptype bits, preserving
`RTE_PTYPE_L2_ETHER`
- Each header is read and validated individually via `rte_pktmbuf_read()`
- Loop terminates safely when packet data exhausted (NULL return) or non-VLAN
ethertype encountered
### Old Behavior Flaw
The QinQ branch assumed exactly one inner VLAN and skipped 2 headers at once,
reading only the second. This failed for stacks like QinQ/QinQ/VLAN.
### New Behavior
Processes each VLAN/QinQ header individually regardless of nesting depth.
---
## Suggestions
### Warning (should consider)
1. **Unbounded loop concern** — While the NULL check provides safety against
buffer overrun, a malicious or malformed packet with many consecutive VLAN tags
could cause CPU spin. Consider adding a reasonable iteration limit:
```c
#define RTE_NET_VLAN_MAX_DEPTH 8 /* or similar reasonable bound */
unsigned int vlan_depth = 0;
while ((proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN) ||
proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_QINQ)) &&
vlan_depth++ < RTE_NET_VLAN_MAX_DEPTH) {
```
This is probably overkill for typical use but would make the parser more
defensive.
### Info (minor style)
2. **Ternary formatting** — The nested ternary inside the `|=` is a bit dense.
Consider:
```c
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;
```
This is clearer and costs nothing. The ternary version works fine though.
---
## Verdict
**Reviewed-by worthy** — The patch correctly fixes a real parsing limitation.
The code change is minimal, correct, and well-explained.
The only substantive suggestion is considering a loop depth limit for defensive
programming, but this is optional given the existing NULL-check safety. The
patch is good as-is.
---
## Suggested Reply
```
On Mon, 3 Nov 2025 17:41:27 +0200, Gregory Etelson wrote:
> The rte_net_get_ptype() supported only 2 types of VLAN headers frames
> that are defined in the IEEE standards 802.1Q and 802.1ad:
The fix looks correct. One minor suggestion: consider adding a loop
depth limit (e.g., 8) as a defensive measure against malformed packets
with excessive VLAN tags, though the existing NULL check does provide
buffer safety.
```
