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 <[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."
