public inbox for dev@dpdk.org
 help / color / mirror / Atom feed
* [PATCH] net: fix parsing of frames with arbitrary VLAN stacks
@ 2025-11-03 15:41 Gregory Etelson
  2026-01-14 19:33 ` Stephen Hemminger
  0 siblings, 1 reply; 4+ messages in thread
From: Gregory Etelson @ 2025-11-03 15:41 UTC (permalink / raw)
  To: dev
  Cc: getelson, mkashani, rasland, thomas, stable, Olivier Matz,
	Didier Pallard

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: stable@dpdk.org

Signed-off-by: Gregory Etelson <getelson@nvidia.com>
---
 lib/net/rte_net.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/lib/net/rte_net.c b/lib/net/rte_net.c
index 44fb6c0f51..96fb054f55 100644
--- a/lib/net/rte_net.c
+++ b/lib/net/rte_net.c
@@ -349,30 +349,24 @@ 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)) {
+	while (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;
 
-		pkt_type = RTE_PTYPE_L2_ETHER_VLAN;
+		pkt_type |=
+			proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN) ?
+				 RTE_PTYPE_L2_ETHER_VLAN :
+				 RTE_PTYPE_L2_ETHER_QINQ;
 		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;
+	}
 
-		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;
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] net: fix parsing of frames with arbitrary VLAN stacks
  2025-11-03 15:41 [PATCH] net: fix parsing of frames with arbitrary VLAN stacks Gregory Etelson
@ 2026-01-14 19:33 ` Stephen Hemminger
  2026-01-15 13:14   ` [PATCH v2] " Gregory Etelson
  0 siblings, 1 reply; 4+ messages in thread
From: Stephen Hemminger @ 2026-01-14 19:33 UTC (permalink / raw)
  To: Gregory Etelson
  Cc: dev, mkashani, rasland, thomas, stable, Olivier Matz,
	Didier Pallard

On Mon, 3 Nov 2025 17:41:27 +0200
Gregory Etelson <getelson@nvidia.com> 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: stable@dpdk.org
> 
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>

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 <getelson@nvidia.com>  
**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: stable@dpdk.org | ✓ | 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.

```


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v2] net: fix parsing of frames with arbitrary VLAN stacks
  2026-01-14 19:33 ` Stephen Hemminger
@ 2026-01-15 13:14   ` Gregory Etelson
  2026-01-21 13:53     ` Thomas Monjalon
  0 siblings, 1 reply; 4+ messages in thread
From: Gregory Etelson @ 2026-01-15 13:14 UTC (permalink / raw)
  To: stephen
  Cc: dev, didier.pallard, getelson, mkashani, olivier.matz, rasland,
	stable, thomas

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 up to RTE_NET_VLAN_MAX_DEPTH:=8
   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: stable@dpdk.org

Signed-off-by: Gregory Etelson <getelson@nvidia.com>
---
v2: Limit maximal supported number of VLAN headers to 8.
---
 lib/net/rte_net.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/lib/net/rte_net.c b/lib/net/rte_net.c
index c70b57fdc0..458b4814a9 100644
--- a/lib/net/rte_net.c
+++ b/lib/net/rte_net.c
@@ -320,6 +320,9 @@ rte_net_skip_ip6_ext(uint16_t proto, const struct rte_mbuf *m, uint32_t *off,
 	return -1;
 }
 
+/* limit number of supported VLAN headers */
+#define RTE_NET_VLAN_MAX_DEPTH 8
+
 /* parse mbuf data to get packet type */
 RTE_EXPORT_SYMBOL(rte_net_get_ptype)
 uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
@@ -329,7 +332,7 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
 	const struct rte_ether_hdr *eh;
 	struct rte_ether_hdr eh_copy;
 	uint32_t pkt_type = RTE_PTYPE_L2_ETHER;
-	uint32_t off = 0;
+	uint32_t off = 0, vlan_depth = 0;
 	uint16_t proto;
 	int ret;
 
@@ -349,30 +352,26 @@ 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)) {
+	while (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;
 
-		pkt_type = RTE_PTYPE_L2_ETHER_VLAN;
+		if (++vlan_depth > RTE_NET_VLAN_MAX_DEPTH)
+			return 0;
+		pkt_type |=
+			proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN) ?
+				 RTE_PTYPE_L2_ETHER_VLAN :
+				 RTE_PTYPE_L2_ETHER_QINQ;
 		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;
+	}
 
-		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;
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] net: fix parsing of frames with arbitrary VLAN stacks
  2026-01-15 13:14   ` [PATCH v2] " Gregory Etelson
@ 2026-01-21 13:53     ` Thomas Monjalon
  0 siblings, 0 replies; 4+ messages in thread
From: Thomas Monjalon @ 2026-01-21 13:53 UTC (permalink / raw)
  To: Gregory Etelson
  Cc: stephen, dev, didier.pallard, getelson, mkashani, olivier.matz,
	rasland, stable

15/01/2026 14:14, Gregory Etelson:
> 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 up to RTE_NET_VLAN_MAX_DEPTH:=8
>    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: stable@dpdk.org
> 
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>

There's no ack, probably because the parsing code looks complex.
v1 was sent in November with a review helped by AI.
v2 solves the only issue found.
So it's time to merge.

Applied, thanks.




^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-01-21 13:53 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-03 15:41 [PATCH] net: fix parsing of frames with arbitrary VLAN stacks Gregory Etelson
2026-01-14 19:33 ` Stephen Hemminger
2026-01-15 13:14   ` [PATCH v2] " Gregory Etelson
2026-01-21 13:53     ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox