[libnftnl RFC] data_reg: Improve data reg value printing

[libnftnl RFC] data_reg: Improve data reg value printing

[Phil Sutter]

The old code printing each field with data as u32 value is problematic
in two ways:

A) Field values are printed in host byte order which may not be correct
   and output for identical data will divert between machines of
   different Endianness.

B) The actual data length is not clearly readable from given output.

This patch won't entirely fix for (A) given that data may be in host
byte order but it solves for the common case of matching against packet
data.

Fixing for (B) is crucial to see what's happening beneath the bonnet.
The new output will show exactly what is used e.g. by a cmp expression.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
This change will affect practically all stored payload dumps in nftables
test suite. I have an alternative version which prints "full" reg fields
as before and uses the byte-by-byte printing only for the remainder (if
any). This would largely reduce the churn in stored payload dumps, but
also make this less useful.
---
 src/expr/data_reg.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/expr/data_reg.c b/src/expr/data_reg.c
index fd5e0d6e749e1..d7531a76af68f 100644
--- a/src/expr/data_reg.c
+++ b/src/expr/data_reg.c
@@ -26,13 +26,22 @@ nftnl_data_reg_value_snprintf_default(char *buf, size_t remain,
 				      uint32_t flags)
 {
 	const char *pfx = flags & DATA_F_NOPFX ? "" : "0x";
-	int offset = 0, ret, i;
-
+	int num32 = reg->len / sizeof(uint32_t);
+	int rem32 = reg->len % sizeof(uint32_t);
+	int offset = 0, ret, i, j;
 
+	for (i = 0; i < num32 + !!rem32; i++) {
+		ret = snprintf(buf + offset, remain, "%s", pfx);
+		SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 
-	for (i = 0; i < div_round_up(reg->len, sizeof(uint32_t)); i++) {
-		ret = snprintf(buf + offset, remain,
-			       "%s%.8x ", pfx, reg->val[i]);
+		for (j = 0; j < sizeof(uint32_t); j++) {
+			if (i == num32 && j == rem32)
+				break;
+			ret = snprintf(buf + offset, remain, "%.2x",
+				       ((unsigned char *)&reg->val[i])[j]);
+			SNPRINTF_BUFFER_SIZE(ret, remain, offset);
+		}
+		ret = snprintf(buf + offset, remain, " ");
 		SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 	}
 
-- 
2.51.0

Re: [libnftnl RFC] data_reg: Improve data reg value printing

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> The old code printing each field with data as u32 value is problematic
> in two ways:
> 
> A) Field values are printed in host byte order which may not be correct
>    and output for identical data will divert between machines of
>    different Endianness.
> 
> B) The actual data length is not clearly readable from given output.
> 
> This patch won't entirely fix for (A) given that data may be in host
> byte order but it solves for the common case of matching against packet
> data.

Can you provide an example diff and a diffstat for the expected fallout in
nftables?

> Fixing for (B) is crucial to see what's happening beneath the bonnet.
> The new output will show exactly what is used e.g. by a cmp expression.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> This change will affect practically all stored payload dumps in nftables
> test suite. I have an alternative version which prints "full" reg fields
> as before and uses the byte-by-byte printing only for the remainder (if
> any). This would largely reduce the churn in stored payload dumps, but
> also make this less useful.

I think that if we want it then one big code-churn commit would be
better than multiple smaller ones.

The inability to see the width of the compare operation is bad
for debugging so I would prefer to change it.

Re: [libnftnl RFC] data_reg: Improve data reg value printing

[Phil Sutter]

On Thu, Sep 11, 2025 at 04:33:58PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > The old code printing each field with data as u32 value is problematic
> > in two ways:
> > 
> > A) Field values are printed in host byte order which may not be correct
> >    and output for identical data will divert between machines of
> >    different Endianness.
> > 
> > B) The actual data length is not clearly readable from given output.
> > 
> > This patch won't entirely fix for (A) given that data may be in host
> > byte order but it solves for the common case of matching against packet
> > data.
> 
> Can you provide an example diff and a diffstat for the expected fallout in
> nftables?

Getting a diffstat is tedious: tests/py will create .got files, but
copying them to their name with that suffix dropped will remove
unproblematic payloads. OTOH dropping all stored payload files upfront
will cause a mess due to needed per-family payload records.

Here's some stats though after running tests/py in standard syntax only:

|% find . -name \*.got | wc -l
|123
|% find . -name \*.got | \
|	while read f; do diff "$f" "${f%.got}"; done | \
|	grep '^>' | \
|	wc -l
|7821

Here are some sample diffs:

| # meta length 1000
| ip test-ip4 input
|   [ meta load len => reg 1 ]
|-  [ cmp eq reg 1 0x000003e8 ]
|+  [ cmp eq reg 1 0xe8030000 ]
| 
| # meta length 22
| ip test-ip4 input
|   [ meta load len => reg 1 ]
|-  [ cmp eq reg 1 0x00000016 ]
|+  [ cmp eq reg 1 0x16000000 ]

The above is noise, data is in host byte order. Ranges are not, though:

| # meta length 33-45
| ip test-ip4 input
|   [ meta load len => reg 1 ]
|   [ byteorder reg 1 = hton(reg 1, 4, 4) ]
|-  [ range eq reg 1 0x21000000 0x2d000000 ]
|+  [ range eq reg 1 0x00000021 0x0000002d ]

Sets are "special", also due to another patch which cleans up output for
them:

| # meta length { 33, 55, 67, 88}
|-__set%d test-ip4 3
|-__set%d test-ip4 0
|-       element 00000021  : 0 [end]     element 00000037  : 0 [end]     element 00000043  : 0 [end]     element 00000058  : 0 [end]
|+       element 21000000        element 37000000        element 43000000        element 58000000 
| ip test-ip4 input
|   [ meta load len => reg 1 ]
|   [ lookup reg 1 set __set%d ]
 
Said patch drops that pointless [end] marker and drops the colon for
non-maps as well as flags if zero (otherwise prints "flags NN" to
separate them from element data). Set definitions are lost in the test
suite runner filter since d477eada4f271 ("tests/py: clean up set backend
support fallout").

But now for some better examples:

| # meta protocol ip
| ip test-ip4 input
|   [ meta load protocol => reg 1 ]
|-  [ cmp eq reg 1 0x00000008 ]
|+  [ cmp eq reg 1 0x0800 ]
|
| # meta l4proto 22
| ip test-ip4 input
|   [ meta load l4proto => reg 1 ]
|-  [ cmp eq reg 1 0x00000016 ]
|+  [ cmp eq reg 1 0x16 ]


Obviously, 'meta protocol' is two bytes and 'meta l4proto' just one.

Strings are much nicer:

| # meta iifname "dummy0"
| ip test-ip4 input
|   [ meta load iifname => reg 1 ]
|-  [ cmp eq reg 1 0x6d6d7564 0x00003079 0x00000000 0x00000000 ]
|+  [ cmp eq reg 1 0x64756d6d 0x79300000 0x00000000 0x00000000 ]

Packet payload is the actual sales pitch for this patch:

| # ip6 saddr 1234:1234:1234::
| inet test-inet input
|   [ meta load nfproto => reg 1 ]
|-  [ cmp eq reg 1 0x0000000a ]
|+  [ cmp eq reg 1 0x0a ]
|   [ payload load 16b @ network header + 8 => reg 1 ]
|-  [ cmp eq reg 1 0x34123412 0x00003412 0x00000000 0x00000000 ]
|+  [ cmp eq reg 1 0x12341234 0x12340000 0x00000000 0x00000000 ]
|
| # ip6 nexthdr 33-44
| inet test-inet input
|   [ meta load nfproto => reg 1 ]
|-  [ cmp eq reg 1 0x0000000a ]
|+  [ cmp eq reg 1 0x0a ]
|   [ payload load 1b @ network header + 6 => reg 1 ]
|-  [ range eq reg 1 0x00000021 0x0000002c ]
|+  [ range eq reg 1 0x21 0x2c ]
|
| # ip6 length != 233
| inet test-inet input
|   [ meta load nfproto => reg 1 ]
|-  [ cmp eq reg 1 0x0000000a ]
|+  [ cmp eq reg 1 0x0a ]
|   [ payload load 2b @ network header + 4 => reg 1 ]
|-  [ cmp neq reg 1 0x0000e900 ]
|+  [ cmp neq reg 1 0x00e9 ]

The combination of network byte order and correct lengths fits perfectly
and even works on Big Endian hosts. Ethernet addresses are 6B:

| # ether saddr 00:0f:54:0c:11:04 ip daddr 1.2.3.4 accept
| ip test-ip input
|   [ meta load iiftype => reg 1 ]
|-  [ cmp eq reg 1 0x00000001 ]
|+  [ cmp eq reg 1 0x0100 ]
|   [ payload load 6b @ link header + 6 => reg 1 ]
|-  [ cmp eq reg 1 0x0c540f00 0x00000411 ]
|+  [ cmp eq reg 1 0x000f540c 0x1104 ]
|   [ payload load 4b @ network header + 16 => reg 1 ]
|-  [ cmp eq reg 1 0x04030201 ]
|+  [ cmp eq reg 1 0x01020304 ]
|   [ immediate reg 0 accept ]

The value separating into 4B chunks and the 0x prefix is rather
disturbing here, maybe we should drop at least the prefix entirely (set
elements don't have it, either).


> > Fixing for (B) is crucial to see what's happening beneath the bonnet.
> > The new output will show exactly what is used e.g. by a cmp expression.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> > This change will affect practically all stored payload dumps in nftables
> > test suite. I have an alternative version which prints "full" reg fields
> > as before and uses the byte-by-byte printing only for the remainder (if
> > any). This would largely reduce the churn in stored payload dumps, but
> > also make this less useful.
> 
> I think that if we want it then one big code-churn commit would be
> better than multiple smaller ones.

ACK. Hence why I also decided to do some cleanup of set element debug
output - the imminent bulk change is a good occasion.

> The inability to see the width of the compare operation is bad
> for debugging so I would prefer to change it.

As mentioned, we could decide to leave full data fields or at least data
regs of size N*4 alone to reduce churn. But interpreting values becomes
even harder due to the mix of host byte order and "Big Endian".

Cheers, Phil

Re: [libnftnl RFC] data_reg: Improve data reg value printing

[Pablo Neira Ayuso]

Hi Phil,

On Thu, Sep 11, 2025 at 04:11:45PM +0200, Phil Sutter wrote:
> The old code printing each field with data as u32 value is problematic
> in two ways:
> 
> A) Field values are printed in host byte order which may not be correct
>    and output for identical data will divert between machines of
>    different Endianness.
> 
> B) The actual data length is not clearly readable from given output.
> 
> This patch won't entirely fix for (A) given that data may be in host
> byte order but it solves for the common case of matching against packet
> data.
> 
> Fixing for (B) is crucial to see what's happening beneath the bonnet.
> The new output will show exactly what is used e.g. by a cmp expression.

Could you fix this from libnftables? ie. add print functions that have
access to the byteorder, so print can do accordingly.

Re: [libnftnl RFC] data_reg: Improve data reg value printing

[Phil Sutter]

Hi Pablo,

On Mon, Sep 15, 2025 at 11:19:27PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Sep 11, 2025 at 04:11:45PM +0200, Phil Sutter wrote:
> > The old code printing each field with data as u32 value is problematic
> > in two ways:
> > 
> > A) Field values are printed in host byte order which may not be correct
> >    and output for identical data will divert between machines of
> >    different Endianness.
> > 
> > B) The actual data length is not clearly readable from given output.
> > 
> > This patch won't entirely fix for (A) given that data may be in host
> > byte order but it solves for the common case of matching against packet
> > data.
> > 
> > Fixing for (B) is crucial to see what's happening beneath the bonnet.
> > The new output will show exactly what is used e.g. by a cmp expression.
> 
> Could you fix this from libnftables? ie. add print functions that have
> access to the byteorder, so print can do accordingly.

You mean replacing nftnl_expr_fprintf() and all per-expr callbacks
entirely? I guess this exposes lots of libnftnl internals to
libnftables.

IIRC, the last approach from 2020 was to communicate LHS byteorder to
RHS in libnftnl internally and in addition to that annotate sets with
element byteorder info (which is not entirely trivial due to
concatenations of data with varying byteorder.

I don't get the code in my old branches anymore, though. Maybe if I read
up on our past mails. Or maybe we just retry from scratch with a fresh
mind. :)

Cheers, Phil

Re: [libnftnl RFC] data_reg: Improve data reg value printing

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Hi Phil,
> 
> On Thu, Sep 11, 2025 at 04:11:45PM +0200, Phil Sutter wrote:
> > The old code printing each field with data as u32 value is problematic
> > in two ways:
> > 
> > A) Field values are printed in host byte order which may not be correct
> >    and output for identical data will divert between machines of
> >    different Endianness.
> > 
> > B) The actual data length is not clearly readable from given output.
> > 
> > This patch won't entirely fix for (A) given that data may be in host
> > byte order but it solves for the common case of matching against packet
> > data.
> > 
> > Fixing for (B) is crucial to see what's happening beneath the bonnet.
> > The new output will show exactly what is used e.g. by a cmp expression.
> 
> Could you fix this from libnftables? ie. add print functions that have
> access to the byteorder, so print can do accordingly.

FWIW I prefer this patch.

While it would be possible to move all printing to libnftables (and
as you point out it has more information available wrt. to the data
types involved, esp. byteorder), I think that printing the data
byte-wise rather than per u32 word is sufficient for debugging a wide
range of bugs.  In particular it will expose the true size of the
immediate/rhs value.

Re: [libnftnl RFC] data_reg: Improve data reg value printing

[Pablo Neira Ayuso]

Hi Phil,

On Tue, Sep 16, 2025 at 12:16:42AM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Mon, Sep 15, 2025 at 11:19:27PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Sep 11, 2025 at 04:11:45PM +0200, Phil Sutter wrote:
> > > The old code printing each field with data as u32 value is problematic
> > > in two ways:
> > > 
> > > A) Field values are printed in host byte order which may not be correct
> > >    and output for identical data will divert between machines of
> > >    different Endianness.
> > > 
> > > B) The actual data length is not clearly readable from given output.
> > > 
> > > This patch won't entirely fix for (A) given that data may be in host
> > > byte order but it solves for the common case of matching against packet
> > > data.
> > > 
> > > Fixing for (B) is crucial to see what's happening beneath the bonnet.
> > > The new output will show exactly what is used e.g. by a cmp expression.
> > 
> > Could you fix this from libnftables? ie. add print functions that have
> > access to the byteorder, so print can do accordingly.
> 
> You mean replacing nftnl_expr_fprintf() and all per-expr callbacks
> entirely? I guess this exposes lots of libnftnl internals to
> libnftables.
>
> IIRC, the last approach from 2020 was to communicate LHS byteorder to
> RHS in libnftnl internally and in addition to that annotate sets with
> element byteorder info (which is not entirely trivial due to
> concatenations of data with varying byteorder.
>
> I don't get the code in my old branches anymore, though. Maybe if I read
> up on our past mails. Or maybe we just retry from scratch with a fresh
> mind. :)

Maybe this approach?

For immediates, a new libnftnl function could help us deal with this:

-                nftnl_expr_set(nle, NFTNL_EXPR_CMP_DATA, nld2.value, nld2.len);
+                nftnl_expr_set_imm(nle, NFTNL_EXPR_CMP_DATA, nld2.value, nld2.len, nld2.byteorder);

Then, for set elements, add a new field here:

struct nft_data_linearize {
        uint32_t        len;
        uint32_t        value[NFT_REG32_COUNT];
+       uint32_t        byteorder[NFT_REG32_COUNT];

where byteorder stores a bitmask that specifies byteorder, 0 can be
network endian and 1 host endian, the length of the bitmask length
tells us the size of this field.

Then, add nftnl_set_elem_set_imm() that takes nld.byteorder.

From libnftnl, use this bitmask from _snprintf to swap bits
accordingly.

union nftnl_data_reg would need to be modified to store this
byteorder[] array including the bitmask. This will not increase memory
consumption because only one intermediate libnftnl object is used to
generate the netlink bytecode blob at a time.

This should not be too hard to make it work for userspace -> kernel.

Would this be good enough to make tests/py happy?

Re: [libnftnl RFC] data_reg: Improve data reg value printing

[Phil Sutter]

On Tue, Sep 16, 2025 at 02:08:03PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Hi Phil,
> > 
> > On Thu, Sep 11, 2025 at 04:11:45PM +0200, Phil Sutter wrote:
> > > The old code printing each field with data as u32 value is problematic
> > > in two ways:
> > > 
> > > A) Field values are printed in host byte order which may not be correct
> > >    and output for identical data will divert between machines of
> > >    different Endianness.
> > > 
> > > B) The actual data length is not clearly readable from given output.
> > > 
> > > This patch won't entirely fix for (A) given that data may be in host
> > > byte order but it solves for the common case of matching against packet
> > > data.
> > > 
> > > Fixing for (B) is crucial to see what's happening beneath the bonnet.
> > > The new output will show exactly what is used e.g. by a cmp expression.
> > 
> > Could you fix this from libnftables? ie. add print functions that have
> > access to the byteorder, so print can do accordingly.
> 
> FWIW I prefer this patch.
> 
> While it would be possible to move all printing to libnftables (and
> as you point out it has more information available wrt. to the data
> types involved, esp. byteorder), I think that printing the data
> byte-wise rather than per u32 word is sufficient for debugging a wide
> range of bugs.  In particular it will expose the true size of the
> immediate/rhs value.

Can we treat the register length fix separate from the Endian issue? I
am aware we want to reduce the amount of payload record churn, but with
this patch applied, a follow-up addressing the Endian issue should touch
only the data in host Byteorder.

Guess I'll play a bit with the suggested Byteorder fix approach to see
how straightforward it is and how much extra churn it causes.

Thanks, Phil