On Tue, 22 Sep 2026 16:36:40 +0000
Morten Brørup <[email protected]> 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: [email protected]
>
> Reported-by: Maxime Leroy <[email protected]>
> Signed-off-by: Morten Brørup <[email protected]>
> ---
Reviewed-by: Stephen Hemminger <[email protected]>
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."
