[PATCH nft 1/2] tests/py: prepare for set debug change

[PATCH nft 1/2] tests/py: prepare for set debug change

[Florian Westphal]

Next patch will make initial set dump from kernel emit set debug
information, so the obtained netlink debug file won't match what is
recorded in tests/py.

Furthermore, as the python add rules for each of the family the test is
for, subsequent dump will include debug information of the other/previous
families.

Change the script to skip all unrelated information to only compare the
relevant set element information and the generated expressions.

This change still finds changes in [ expr ... ] and set elem debug output.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/py/nft-test.py | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tests/py/nft-test.py b/tests/py/nft-test.py
index 7acdb77f2d0a..ea70f19b196d 100755
--- a/tests/py/nft-test.py
+++ b/tests/py/nft-test.py
@@ -717,17 +717,20 @@ def payload_check(payload_buffer, file, cmd):
         return False
 
     for lineno, want_line in enumerate(payload_buffer):
+        # skip irreleant parts, such as "ip test-ipv4 output"
+        if want_line.find("[") < 0 or want_line.find("]") < 0:
+             continue
+
         line = file.readline()
+        while line.find("[") < 0 or line.find("]") < 0:
+            line = file.readline()
+            if line == "":
+                break
 
         if want_line == line:
             i += 1
             continue
 
-        if want_line.find('[') < 0 and line.find('[') < 0:
-            continue
-        if want_line.find(']') < 0 and line.find(']') < 0:
-            continue
-
         if payload_check_set_elems(want_line, line):
             continue
 
@@ -877,6 +880,8 @@ def rule_add(rule, filename, lineno, force_all_family_option, filename_path):
                     gotf.write("# %s\n" % rule[0])
                     while True:
                         line = payload_log.readline()
+                        if line.startswith("family "):
+                            continue
                         if line == "":
                             break
                         gotf.write(line)
-- 
2.47.0

[PATCH nft 2/2] debug: include kernel set information on cache fill

[Florian Westphal]

Honor --debug=netlink flag also when doing initial set dump
from the kernel.

With recent libnftnl update this will include the chosen
set backend name that is used by the kernel.

Because set names are scoped by table and protocol family,
also include the family protocol number.

Dumping this information breaks tests/py as the recorded
debug output no longer matches, this is fixed in previous
change.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/mnl.c     | 15 +++++++++++++--
 src/netlink.c |  3 +++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/mnl.c b/src/mnl.c
index 828006c4d6bf..24a7487a5b5b 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -1386,9 +1386,15 @@ int mnl_nft_set_del(struct netlink_ctx *ctx, struct cmd *cmd)
 	return 0;
 }
 
+struct set_cb_args {
+	struct netlink_ctx *ctx;
+	struct nftnl_set_list *list;
+};
+
 static int set_cb(const struct nlmsghdr *nlh, void *data)
 {
-	struct nftnl_set_list *nls_list = data;
+	struct set_cb_args *args = data;
+	struct nftnl_set_list *nls_list = args->list;
 	struct nftnl_set *s;
 
 	if (check_genid(nlh) < 0)
@@ -1401,6 +1407,8 @@ static int set_cb(const struct nlmsghdr *nlh, void *data)
 	if (nftnl_set_nlmsg_parse(nlh, s) < 0)
 		goto err_free;
 
+	netlink_dump_set(s, args->ctx);
+
 	nftnl_set_list_add_tail(s, nls_list);
 	return MNL_CB_OK;
 
@@ -1419,6 +1427,7 @@ mnl_nft_set_dump(struct netlink_ctx *ctx, int family,
 	struct nlmsghdr *nlh;
 	struct nftnl_set *s;
 	int ret;
+	struct set_cb_args args;
 
 	s = nftnl_set_alloc();
 	if (s == NULL)
@@ -1440,7 +1449,9 @@ mnl_nft_set_dump(struct netlink_ctx *ctx, int family,
 	if (nls_list == NULL)
 		memory_allocation_error();
 
-	ret = nft_mnl_talk(ctx, nlh, nlh->nlmsg_len, set_cb, nls_list);
+	args.list = nls_list;
+	args.ctx  = ctx;
+	ret = nft_mnl_talk(ctx, nlh, nlh->nlmsg_len, set_cb, &args);
 	if (ret < 0 && errno != ENOENT)
 		goto err;
 
diff --git a/src/netlink.c b/src/netlink.c
index 36140fb63d6f..f3a5fa2e4309 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -832,10 +832,13 @@ static const struct datatype *dtype_map_from_kernel(enum nft_data_types type)
 void netlink_dump_set(const struct nftnl_set *nls, struct netlink_ctx *ctx)
 {
 	FILE *fp = ctx->nft->output.output_fp;
+	uint32_t family;
 
 	if (!(ctx->nft->debug_mask & NFT_DEBUG_NETLINK) || !fp)
 		return;
 
+	family = nftnl_set_get_u32(nls, NFTNL_SET_FAMILY);
+	fprintf(fp, "family %d ", family);
 	nftnl_set_fprintf(fp, nls, 0, 0);
 	fprintf(fp, "\n");
 }
-- 
2.47.0

Re: [PATCH nft 2/2] debug: include kernel set information on cache fill

[Pablo Neira Ayuso]

Hi Florian,

On Wed, Nov 20, 2024 at 11:02:16AM +0100, Florian Westphal wrote:
> Honor --debug=netlink flag also when doing initial set dump
> from the kernel.
> 
> With recent libnftnl update this will include the chosen
> set backend name that is used by the kernel.
> 
> Because set names are scoped by table and protocol family,
> also include the family protocol number.
> 
> Dumping this information breaks tests/py as the recorded
> debug output no longer matches, this is fixed in previous
> change.

table ip x {
        set y {
                type ipv4_addr
                size 256        # count 128
                ...

We have to exposed the number of elements counter. I think this can be
exposed if set declaration provides size (or default size is used).

And update nftables manpage:

"When listing the set, the element count is larger than the listed
number of elements for sets: the number of elements in the set is
updated when elements added/deleted to the set and periodically when
the garbage collector evicts the timed out elements."

P.S: Yes, I changed my mind on this :)

Re: [PATCH nft 2/2] debug: include kernel set information on cache fill

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Hi Florian,
> 
> On Wed, Nov 20, 2024 at 11:02:16AM +0100, Florian Westphal wrote:
> > Honor --debug=netlink flag also when doing initial set dump
> > from the kernel.
> > 
> > With recent libnftnl update this will include the chosen
> > set backend name that is used by the kernel.
> > 
> > Because set names are scoped by table and protocol family,
> > also include the family protocol number.
> > 
> > Dumping this information breaks tests/py as the recorded
> > debug output no longer matches, this is fixed in previous
> > change.
> 
> table ip x {
>         set y {
>                 type ipv4_addr
>                 size 256        # count 128
>                 ...
> 
> We have to exposed the number of elements counter. I think this can be
> exposed if set declaration provides size (or default size is used).

OK,  I will update libnftl then because this means it will need
proper getter for nft sake.

Re: [PATCH nft 2/2] debug: include kernel set information on cache fill

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> >         set y {
> >                 type ipv4_addr
> >                 size 256        # count 128
> >                 ...
> > 
> > We have to exposed the number of elements counter. I think this can be
> > exposed if set declaration provides size (or default size is used).
> 
> OK,  I will update libnftl then because this means it will need
> proper getter for nft sake.

There is a problem with this, shell tests break:

W: [DUMP FAIL]    9/430 tests/shell/testcases/sets/0057set_create_fails_0

cat /tmp/nft-test.latest.root/test-tests-shell-testcases-sets-0057set_create_fails_0.11/ruleset-diff
--- tests/shell/testcases/sets/dumps/0057set_create_fails_0.nft 2024-11-21 09:46:16.888431831 +0100
+++ /tmp/nft-test.20241121-101956.182.zWvUOZ/test-tests-shell-testcases-sets-0057set_create_fails_0.11/ruleset-after    2024-11-21 10:20:00.046431831 +0100
@@ -1,7 +1,7 @@
 table inet filter {
        set test {
                type ipv4_addr
-               size 65535
+               size 65535      # count 1
                elements = { 1.1.1.1 }
        }

As shell tests coud run on old kernel, regen dump file won't work.

Only options I see is to add a feature test file for this support,
and then either disabling dump validation if it failed or adding
additonal/alternative dump file.

Re: [PATCH nft 2/2] debug: include kernel set information on cache fill

[Pablo Neira Ayuso]

On Thu, Nov 21, 2024 at 10:24:27AM +0100, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > >         set y {
> > >                 type ipv4_addr
> > >                 size 256        # count 128
> > >                 ...
> > > 
> > > We have to exposed the number of elements counter. I think this can be
> > > exposed if set declaration provides size (or default size is used).
> > 
> > OK,  I will update libnftl then because this means it will need
> > proper getter for nft sake.
> 
> There is a problem with this, shell tests break:
> 
> W: [DUMP FAIL]    9/430 tests/shell/testcases/sets/0057set_create_fails_0
> 
> cat /tmp/nft-test.latest.root/test-tests-shell-testcases-sets-0057set_create_fails_0.11/ruleset-diff
> --- tests/shell/testcases/sets/dumps/0057set_create_fails_0.nft 2024-11-21 09:46:16.888431831 +0100
> +++ /tmp/nft-test.20241121-101956.182.zWvUOZ/test-tests-shell-testcases-sets-0057set_create_fails_0.11/ruleset-after    2024-11-21 10:20:00.046431831 +0100
> @@ -1,7 +1,7 @@
>  table inet filter {
>         set test {
>                 type ipv4_addr
> -               size 65535
> +               size 65535      # count 1
>                 elements = { 1.1.1.1 }
>         }
> 
> As shell tests coud run on old kernel, regen dump file won't work.
> 
> Only options I see is to add a feature test file for this support,
> and then either disabling dump validation if it failed or adding
> additonal/alternative dump file.

Oh right, tests!

Probably tests/shell can be workaround to remove # count X before
comparing output.

It won't look nice, but I think tests/shell can carry on this burden.
This means # count N will not be checked in old and new kernels.

To validate # count N, we can still rely on tests/py and the debug
output as you propose.

Not great, but does this sound sensible to you?

Thanks.

Re: [PATCH nft 2/2] debug: include kernel set information on cache fill

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Only options I see is to add a feature test file for this support,
> > and then either disabling dump validation if it failed or adding
> > additonal/alternative dump file.
> 
> Oh right, tests!
> 
> Probably tests/shell can be workaround to remove # count X before
> comparing output.
> 
> It won't look nice, but I think tests/shell can carry on this burden.
> This means # count N will not be checked in old and new kernels.
> 
> To validate # count N, we can still rely on tests/py and the debug
> output as you propose.
> 
> Not great, but does this sound sensible to you?

1. Add new feature test
2. Update dump files to include "# count xxx"
3. when diff -u fails, do postprocess on recorded
   dump file, i.e. sed s/# count.*//g 
4. repeat diff with postprocessed recorded dump
   if ok -> ok, else dump failure

Does that sound ok?
AFAICS we only need to update < 10 dump files,
so churn is not too bad.

Alternative is to always store postprocessed
dumps and then always run sed before diff, but I think
its better to do the extra mile.

Re: [PATCH nft 2/2] debug: include kernel set information on cache fill

[Pablo Neira Ayuso]

On Thu, Nov 21, 2024 at 01:02:42PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Only options I see is to add a feature test file for this support,
> > > and then either disabling dump validation if it failed or adding
> > > additonal/alternative dump file.
> > 
> > Oh right, tests!
> > 
> > Probably tests/shell can be workaround to remove # count X before
> > comparing output.
> > 
> > It won't look nice, but I think tests/shell can carry on this burden.
> > This means # count N will not be checked in old and new kernels.
> > 
> > To validate # count N, we can still rely on tests/py and the debug
> > output as you propose.
> > 
> > Not great, but does this sound sensible to you?
> 
> 1. Add new feature test
> 2. Update dump files to include "# count xxx"
> 3. when diff -u fails, do postprocess on recorded
>    dump file, i.e. sed s/# count.*//g 
> 4. repeat diff with postprocessed recorded dump
>    if ok -> ok, else dump failure
> 
> Does that sound ok?

OK, still one more aspect I'd like to discuss.

> AFAICS we only need to update < 10 dump files,
> so churn is not too bad.
>
> Alternative is to always store postprocessed
> dumps and then always run sed before diff, but I think
> its better to do the extra mile.

rbtree going leaks a raw count of independent interval values which is
going to be awkward to the user.

Re: [PATCH nft 2/2] debug: include kernel set information on cache fill

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > AFAICS we only need to update < 10 dump files,
> > so churn is not too bad.
> >
> > Alternative is to always store postprocessed
> > dumps and then always run sed before diff, but I think
> > its better to do the extra mile.
> 
> rbtree going leaks a raw count of independent interval values which is
> going to be awkward to the user.

Sure, wasn't that the reason why you iniitially wanted to restrict this to
--netlink=debug?  What made you change your mind?

Maybe apply the simpler, existing v1 patches only, i.e. no exposure?

I can just send a v2 with the new attribute names and no getter for
libnftnl.

Re: [PATCH nft 2/2] debug: include kernel set information on cache fill

[Pablo Neira Ayuso]

Hi Florian,

On Thu, Nov 21, 2024 at 06:19:57PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > AFAICS we only need to update < 10 dump files,
> > > so churn is not too bad.
> > >
> > > Alternative is to always store postprocessed
> > > dumps and then always run sed before diff, but I think
> > > its better to do the extra mile.
> > 
> > rbtree going leaks a raw count of independent interval values which is
> > going to be awkward to the user.
> 
> Sure, wasn't that the reason why you iniitially wanted to restrict this to
> --netlink=debug?  What made you change your mind?

With large garbage collection cycle, this counter provides a hint to
the user to understand that slots are still being consumed by expired
elements.

> Maybe apply the simpler, existing v1 patches only, i.e. no exposure?

My concern is that this is exposing this implementation detail of the
rbtree, forever. Can we agree to do heuristics to hide this detail:

Assuming initial 0.0.0.0 dummy element is in place (this can be
subtracted), then, division by two gives us the number of ranges.

> I can just send a v2 with the new attribute names and no getter for
> libnftnl.

Thanks.

Re: [PATCH nft 2/2] debug: include kernel set information on cache fill

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Sure, wasn't that the reason why you iniitially wanted to restrict this to
> > --netlink=debug?  What made you change your mind?
> 
> With large garbage collection cycle, this counter provides a hint to
> the user to understand that slots are still being consumed by expired
> elements.

But how / where is that relevant?

rbtree does gc at insert time.  We could extend rbtree to force gc
even if interval is huge in case we have many expired elements.

We could do this by making __nft_rbtree_insert() count the number
of expired nodes that it saw during traversal, then force gc at commit
time even if time_after_eq() isn't met.

> > Maybe apply the simpler, existing v1 patches only, i.e. no exposure?
> 
> My concern is that this is exposing this implementation detail of the
> rbtree, forever. Can we agree to do heuristics to hide this detail:
> 
> Assuming initial 0.0.0.0 dummy element is in place (this can be
> subtracted), then, division by two gives us the number of ranges.

Ouch.  This either means more kernel complexity and lie to userspace,
or leak rbtree details into nft, basically strcmp on the new
SET_TYPE nlattr string and then display something else on frontend side.

I'd prefer to avoid this mess.

Re: [PATCH nft 2/2] debug: include kernel set information on cache fill

[Pablo Neira Ayuso]

On Fri, Nov 22, 2024 at 02:43:27PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Sure, wasn't that the reason why you iniitially wanted to restrict this to
> > > --netlink=debug?  What made you change your mind?
> > 
> > With large garbage collection cycle, this counter provides a hint to
> > the user to understand that slots are still being consumed by expired
> > elements.
> 
> But how / where is that relevant?
> 
> rbtree does gc at insert time.  We could extend rbtree to force gc
> even if interval is huge in case we have many expired elements.
> 
> We could do this by making __nft_rbtree_insert() count the number
> of expired nodes that it saw during traversal, then force gc at commit
> time even if time_after_eq() isn't met.

IIRC, rbtree insert path already performs gc on-demand.

> > > Maybe apply the simpler, existing v1 patches only, i.e. no exposure?
> > 
> > My concern is that this is exposing this implementation detail of the
> > rbtree, forever. Can we agree to do heuristics to hide this detail:
> > 
> > Assuming initial 0.0.0.0 dummy element is in place (this can be
> > subtracted), then, division by two gives us the number of ranges.
> 
> Ouch.  This either means more kernel complexity and lie to userspace,
> or leak rbtree details into nft, basically strcmp on the new
> SET_TYPE nlattr string and then display something else on frontend side.

Yes, this is lying to userspace to hide the implementation details.

I would really like to provide an alternative interface for the rbtree
to allow for the same netlink representation as pipapo. I expected
pipapo can replace rbtree by pipapo, but you mentioned in the past
this could be an issue.

> I'd prefer to avoid this mess.

OK, then we assume this will be forever used for debugging only,
unless rbtree is fully replaced.

Please, let me have a look, if I fail or it is too ugly you can still
ditch it and we can follow up with your approach.

Thanks.

Re: [PATCH nft 2/2] debug: include kernel set information on cache fill

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Nov 22, 2024 at 02:43:27PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > Sure, wasn't that the reason why you iniitially wanted to restrict this to
> > > > --netlink=debug?  What made you change your mind?
> > > 
> > > With large garbage collection cycle, this counter provides a hint to
> > > the user to understand that slots are still being consumed by expired
> > > elements.
> > 
> > But how / where is that relevant?
> > 
> > rbtree does gc at insert time.  We could extend rbtree to force gc
> > even if interval is huge in case we have many expired elements.
> > 
> > We could do this by making __nft_rbtree_insert() count the number
> > of expired nodes that it saw during traversal, then force gc at commit
> > time even if time_after_eq() isn't met.
> 
> IIRC, rbtree insert path already performs gc on-demand.

It doesn't do a full scan though.

Maybe lets take two steps back.  What is the actual issue that
needs to be resolved?

Even if nelems/count is dumped while concealing the
rbtree details, then its still confusing, you get
nelems 42 but no (or fewer) elements = { ... dumped
due to the timeout thing.

So in case we have to document that nelems/count isn't
the number of active elements but stored elements, including
the inactive ones, then we might as well not export this
and instead document consequence of large gc interval.

We could also do something even simpler: when we hit
size limit on dataplane insertion for TIMEOUT element,
expedite next gc scan if gc interval is > 10s (or some
other value -- don't want constant scans when set is full
with no timed out elements).

> I would really like to provide an alternative interface for the rbtree
> to allow for the same netlink representation as pipapo. I expected
> pipapo can replace rbtree by pipapo, but you mentioned in the past
> this could be an issue.

pipapo has other issues, just compare insert and delete times
of pipapo or hash or rbtree.

Even if thats not a concern, ATM userspace cannot force pipapo even if
it wanted to, so this is moot anyway.

> > I'd prefer to avoid this mess.
> 
> OK, then we assume this will be forever used for debugging only,
> unless rbtree is fully replaced.

Only if this fixup stuff is done in the kernel, which sabotages
debug output (conceals actual elements by some strategy rather
than just expose set->nelems).

> Please, let me have a look, if I fail or it is too ugly you can still
> ditch it and we can follow up with your approach.

OK.