Re: [PATCH] net: revert add IPv6 traffic class and flow label fields

[Thomas Monjalon <thomas@monjalon.net>]

Hello, are we waiting for a v2?


22/09/2026 20:14, Stephen Hemminger:
> On Tue, 22 Sep 2026 16:36:40 +0000
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > IPv6 header when the field crosses a byte border.
> > 
> > Let's consider a simplified struct for illustration:
> > 
> > struct example {
> > 	union {
> > 		rte_be32_t vtc_flow;
> > 		struct {
> > 			uint32_t version:4;
> > 			uint32_t ds:6;
> > 			uint32_t after:22;
> > 			uint32_t after:22;
> > 			uint32_t ds:6;
> > 			uint32_t version:4;
> > 		};
> > 	};
> > };
> > 
> > struct example e;
> > e.vtc_flow = 0x00000000;
> > e.ds = 0x3F; // binary: 111111
> > 
> > With big endian:
> > Value of e: 0000 111111 0000000000000000000000 = 0F C0 00 00
> > Memory at e's location: 0F C0 00 00
> > As expected!
> > 
> > With little endian:
> > Memory at e's location: 00 00 C0 0F
> > The reason being that the bytes are filled with bits starting with the
> > LSB, so when crossing a byte border, the "ds" field doesn't continue at
> > the following bits, i.e. the MSB of the next byte, but at the four LSB
> > of the next byte.
> > 
> > This wrapping cannot even be fixed by having separate, dedicated structs
> > for each field (with each their own "before" and "after" filler fields)
> > in the vtc_flow union.
> > 
> > There is a GCC attribute to fix this, but nothing similar is offered by
> > Clang or MSVC:
> > __attribute__((scalar_storage_order("big-endian")))
> > 
> > Bugzilla ID: 1679
> > Fixes: cba27998dc81 ("net: add IPv6 traffic class and flow label fields")
> > Cc: stable@dpdk.org
> > 
> > Reported-by: Maxime Leroy <maxime@leroys.fr>
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> 
> Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> Long form AI review. This may need addressing before merge.
> 
> The revert is correct and complete. cba27998dc81 only touched
> rte_ip6.h, there are no in-tree users of the removed fields, and the
> struct layout is unchanged so there is no ABI impact.
> 
> Keeping Cc: stable is right. The fields shipped in 24.11 and 25.11
> but access the wrong bits on every little endian target, so any
> direct user is already broken and a build failure is the better
> outcome. Please add a note for the stable release notes pointing to
> vtc_flow and the RTE_IPV6_HDR_*_MASK macros. Code that byte swaps
> vtc_flow in place before using the fields does work on little endian
> today and will stop compiling.
> 
> The commit message needs rework before this goes in.
> 
> The example struct does not compile: after, ds and version are each
> declared twice with no #if between the two orderings. Since it is
> the justification for the revert, either fix it or drop it and show
> the actual result.
> 
> The root cause is understated. It is not only ds crossing a byte
> boundary; every field is wrong on little endian, including version.
> A uint32_t bitfield is allocated against the host order value of
> the word, while vtc_flow holds network order. The little endian
> layout is only correct after byte swapping vtc_flow.
> 
> On x86_64 with the removed layout:
> 
>   header bytes 6b 91 23 45 (ver 6, DSCP EF, ECT(1), fl 0x12345)
>     -> version=4 ds=0x14 ecn=2 flow_label=0x3916b
>   version = 6  -> bytes 00 00 00 60
>   ds = 0x3f    -> bytes 00 00 c0 0f
> 
> Suggested wording:
> 
>   On little endian, a 32-bit bitfield is allocated from the least
>   significant bit of the host order word, but vtc_flow is stored in
>   network order. All four fields therefore read and write the wrong
>   bits. For example, setting version to 6 writes 0x60 into the last
>   byte of the word.
> 
>   ds and flow_label are not contiguous in memory on little endian,
>   so no reordering of bitfields can express them.
> 
> For reference, neither Linux nor FreeBSD tries this. Linux uses a u8
> bitfield only for version and the upper nibble of traffic class,
> then flow_lbl[3], with big endian masks (IPV6_FLOWINFO_MASK,
> IPV6_FLOWLABEL_MASK) for the rest. FreeBSD has no bitfields: a raw
> ip6_flow word, the ip6_vfc byte for version, and per byte order
> masks. DPDK already has the equivalent in rte_ipv6_check_version()
> and RTE_IPV6_HDR_*_MASK, so nothing is lost.
> 
> Release note: "defect bitfields" reads oddly and gives no migration
> path. Suggest:
> 
>   * net: Removed ``version``, ``ds``, ``ecn`` and ``flow_label``
>     bitfields from ``struct rte_ipv6_hdr``. They accessed the wrong
>     bits on little endian. Use ``vtc_flow`` with the
>     ``RTE_IPV6_HDR_*_MASK`` and ``RTE_IPV6_HDR_*_SHIFT`` macros.
> 
> Minor: "Reverted the patch introducing them." should be imperative,
> "Revert the patch that introduced them."