On Thu, Sep 11, 2025 at 04:33:58PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@xxxxxx> 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@xxxxxx>
> > ---
> > 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
