netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* nftables monitor json mode is broken
@ 2025-08-27 17:12 Florian Westphal
  2025-08-27 18:19 ` Pablo Neira Ayuso
  2025-08-27 20:55 ` Phil Sutter
  0 siblings, 2 replies; 8+ messages in thread
From: Florian Westphal @ 2025-08-27 17:12 UTC (permalink / raw)
  To: netfilter-devel

Hi,

as subject says, 'nft monitor -j' is broken.
Example:

./run-tests.sh -j testcases/object.t
monitor: running tests from file object.t
monitor output differs!
--- /tmp/tmp.emU4zIN8UT/tmp.C4TeyO6xYk  2025-08-27 19:05:08.039619097 +0200
+++ /tmp/tmp.emU4zIN8UT/tmp.jBOL3aIrp5  2025-08-27 19:05:09.062551248 +0200
@@ -1 +1 @@
-{"delete": {"quota": {"family": "ip", "name": "q", "table": "t", "handle": 0, "bytes": 26214400, "used": 0, "inv": false}}}
+{"delete": {"quota": {"family": "ip", "name": "q", "table": "t", "handle": 0, "bytes": 0, "used": 0, "inv": false}}}
monitor output differs!
--- /tmp/tmp.emU4zIN8UT/tmp.C4TeyO6xYk  2025-08-27 19:05:10.095619097 +0200
+++ /tmp/tmp.emU4zIN8UT/tmp.Guz55knY19  2025-08-27 19:05:11.117393075 +0200
@@ -1 +1 @@
-{"delete": {"limit": {"family": "ip", "name": "l", "table": "t", "handle": 0, "rate": 1, "per": "second", "burst": 5}}}
+{"delete": {"limit": {"family": "ip", "name": "l", "table": "t", "handle": 0, "rate": 0, "per": "error"}}}

I did notice this weeks ago but thought it was a problem on my end
and then didn't have time to investigate closer.

But its in fact broken on kernel side, since

netfilter: nf_tables: Reintroduce shortened deletion notifications

In short, unlike the normal output, json output wants to dump
everything, but the notifications no longer include the extra data, just
the bare minimum to identify the object being deleted.

As noone has complained so far I am inclinded to delete the
tests and rip out json support from monitor mode, it seems noone
uses it or even runs the tests for it.

Alternatives i see are:
1. implement a cache and query it
2. rework the json mode to be forgiving as to what is set
   and what isn't in the object.

Object here also means any object reported in any delete kind,
not just NFT_MSG_DELOBJ.  This applies to set elements etc. too,
json expects the full info, but the kernel notifications no longer
provide this.

Alternative options?

Thanks.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: nftables monitor json mode is broken
  2025-08-27 17:12 nftables monitor json mode is broken Florian Westphal
@ 2025-08-27 18:19 ` Pablo Neira Ayuso
  2025-08-27 18:37   ` Pablo Neira Ayuso
  2025-08-27 20:55 ` Phil Sutter
  1 sibling, 1 reply; 8+ messages in thread
From: Pablo Neira Ayuso @ 2025-08-27 18:19 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

On Wed, Aug 27, 2025 at 07:12:36PM +0200, Florian Westphal wrote:
> Hi,
> 
> as subject says, 'nft monitor -j' is broken.
> Example:
> 
> ./run-tests.sh -j testcases/object.t
> monitor: running tests from file object.t
> monitor output differs!
> --- /tmp/tmp.emU4zIN8UT/tmp.C4TeyO6xYk  2025-08-27 19:05:08.039619097 +0200
> +++ /tmp/tmp.emU4zIN8UT/tmp.jBOL3aIrp5  2025-08-27 19:05:09.062551248 +0200
> @@ -1 +1 @@
> -{"delete": {"quota": {"family": "ip", "name": "q", "table": "t", "handle": 0, "bytes": 26214400, "used": 0, "inv": false}}}
> +{"delete": {"quota": {"family": "ip", "name": "q", "table": "t", "handle": 0, "bytes": 0, "used": 0, "inv": false}}}
> monitor output differs!
> --- /tmp/tmp.emU4zIN8UT/tmp.C4TeyO6xYk  2025-08-27 19:05:10.095619097 +0200
> +++ /tmp/tmp.emU4zIN8UT/tmp.Guz55knY19  2025-08-27 19:05:11.117393075 +0200
> @@ -1 +1 @@
> -{"delete": {"limit": {"family": "ip", "name": "l", "table": "t", "handle": 0, "rate": 1, "per": "second", "burst": 5}}}
> +{"delete": {"limit": {"family": "ip", "name": "l", "table": "t", "handle": 0, "rate": 0, "per": "error"}}}
> 
> I did notice this weeks ago but thought it was a problem on my end
> and then didn't have time to investigate closer.
> 
> But its in fact broken on kernel side, since
> 
> netfilter: nf_tables: Reintroduce shortened deletion notifications
> 
> In short, unlike the normal output, json output wants to dump
> everything, but the notifications no longer include the extra data, just
> the bare minimum to identify the object being deleted.
> 
> As noone has complained so far I am inclinded to delete the
> tests and rip out json support from monitor mode, it seems noone
> uses it or even runs the tests for it.

Why? Is unfixable to consider this?

> Alternatives i see are:
> 1. implement a cache and query it

There is a cache infrastructure, monitor only need to use it.

> 2. rework the json mode to be forgiving as to what is set
>    and what isn't in the object.
>
> Object here also means any object reported in any delete kind,
> not just NFT_MSG_DELOBJ.  This applies to set elements etc. too,
> json expects the full info, but the kernel notifications no longer
> provide this.

But it does not make sense to provide the full information, delete
object should just provide the handle to identify, to remain in parity
with the native syntax.

> Alternative options?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: nftables monitor json mode is broken
  2025-08-27 18:19 ` Pablo Neira Ayuso
@ 2025-08-27 18:37   ` Pablo Neira Ayuso
  2025-08-28  9:29     ` Florian Westphal
  0 siblings, 1 reply; 8+ messages in thread
From: Pablo Neira Ayuso @ 2025-08-27 18:37 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

On Wed, Aug 27, 2025 at 08:19:51PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Aug 27, 2025 at 07:12:36PM +0200, Florian Westphal wrote:
> > Hi,
> > 
> > as subject says, 'nft monitor -j' is broken.
> > Example:
> > 
> > ./run-tests.sh -j testcases/object.t
> > monitor: running tests from file object.t
> > monitor output differs!
> > --- /tmp/tmp.emU4zIN8UT/tmp.C4TeyO6xYk  2025-08-27 19:05:08.039619097 +0200
> > +++ /tmp/tmp.emU4zIN8UT/tmp.jBOL3aIrp5  2025-08-27 19:05:09.062551248 +0200
> > @@ -1 +1 @@
> > -{"delete": {"quota": {"family": "ip", "name": "q", "table": "t", "handle": 0, "bytes": 26214400, "used": 0, "inv": false}}}
> > +{"delete": {"quota": {"family": "ip", "name": "q", "table": "t", "handle": 0, "bytes": 0, "used": 0, "inv": false}}}
> > monitor output differs!
> > --- /tmp/tmp.emU4zIN8UT/tmp.C4TeyO6xYk  2025-08-27 19:05:10.095619097 +0200
> > +++ /tmp/tmp.emU4zIN8UT/tmp.Guz55knY19  2025-08-27 19:05:11.117393075 +0200
> > @@ -1 +1 @@
> > -{"delete": {"limit": {"family": "ip", "name": "l", "table": "t", "handle": 0, "rate": 1, "per": "second", "burst": 5}}}
> > +{"delete": {"limit": {"family": "ip", "name": "l", "table": "t", "handle": 0, "rate": 0, "per": "error"}}}
> > 
> > I did notice this weeks ago but thought it was a problem on my end
> > and then didn't have time to investigate closer.
> > 
> > But its in fact broken on kernel side, since
> > 
> > netfilter: nf_tables: Reintroduce shortened deletion notifications
> > 
> > In short, unlike the normal output, json output wants to dump
> > everything, but the notifications no longer include the extra data, just
> > the bare minimum to identify the object being deleted.
> > 
> > As noone has complained so far I am inclinded to delete the
> > tests and rip out json support from monitor mode, it seems noone
> > uses it or even runs the tests for it.
> 
> Why? Is unfixable to consider this?
> 
> > Alternatives i see are:
> > 1. implement a cache and query it
> 
> There is a cache infrastructure, monitor only need to use it.

"monitor only use it"

Addendum:

this is a relatively large rework, I started some code but is
incomplete, including rule caching to deal with runtime incremental
updates.

I think it should be better to fix what we have then look pick back on
the rework at some point.

> > 2. rework the json mode to be forgiving as to what is set
> >    and what isn't in the object.
> >
> > Object here also means any object reported in any delete kind,
> > not just NFT_MSG_DELOBJ.  This applies to set elements etc. too,
> > json expects the full info, but the kernel notifications no longer
> > provide this.
> 
> But it does not make sense to provide the full information, delete
> object should just provide the handle to identify, to remain in parity
> with the native syntax.
> 
> > Alternative options?
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: nftables monitor json mode is broken
  2025-08-27 17:12 nftables monitor json mode is broken Florian Westphal
  2025-08-27 18:19 ` Pablo Neira Ayuso
@ 2025-08-27 20:55 ` Phil Sutter
  2025-08-27 21:10   ` Phil Sutter
  1 sibling, 1 reply; 8+ messages in thread
From: Phil Sutter @ 2025-08-27 20:55 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

Hi,

On Wed, Aug 27, 2025 at 07:12:36PM +0200, Florian Westphal wrote:
> as subject says, 'nft monitor -j' is broken.
> Example:
> 
> ./run-tests.sh -j testcases/object.t
> monitor: running tests from file object.t
> monitor output differs!
> --- /tmp/tmp.emU4zIN8UT/tmp.C4TeyO6xYk  2025-08-27 19:05:08.039619097 +0200
> +++ /tmp/tmp.emU4zIN8UT/tmp.jBOL3aIrp5  2025-08-27 19:05:09.062551248 +0200
> @@ -1 +1 @@
> -{"delete": {"quota": {"family": "ip", "name": "q", "table": "t", "handle": 0, "bytes": 26214400, "used": 0, "inv": false}}}
> +{"delete": {"quota": {"family": "ip", "name": "q", "table": "t", "handle": 0, "bytes": 0, "used": 0, "inv": false}}}
> monitor output differs!
> --- /tmp/tmp.emU4zIN8UT/tmp.C4TeyO6xYk  2025-08-27 19:05:10.095619097 +0200
> +++ /tmp/tmp.emU4zIN8UT/tmp.Guz55knY19  2025-08-27 19:05:11.117393075 +0200
> @@ -1 +1 @@
> -{"delete": {"limit": {"family": "ip", "name": "l", "table": "t", "handle": 0, "rate": 1, "per": "second", "burst": 5}}}
> +{"delete": {"limit": {"family": "ip", "name": "l", "table": "t", "handle": 0, "rate": 0, "per": "error"}}}
> 
> I did notice this weeks ago but thought it was a problem on my end
> and then didn't have time to investigate closer.
> 
> But its in fact broken on kernel side, since
> 
> netfilter: nf_tables: Reintroduce shortened deletion notifications
> 
> In short, unlike the normal output, json output wants to dump
> everything, but the notifications no longer include the extra data, just
> the bare minimum to identify the object being deleted.

Oh crap, I missed this entirely.

> As noone has complained so far I am inclinded to delete the
> tests and rip out json support from monitor mode, it seems noone
> uses it or even runs the tests for it.
> 
> Alternatives i see are:
> 1. implement a cache and query it
> 2. rework the json mode to be forgiving as to what is set
>    and what isn't in the object.

My suggestion:
- Add special casing for NFT_MSG_DEL* with NFTNL_OUTPUT_JSON as well in
  monitor.c
- Pass 'bool delete' to monitor_print_*_json()
- And on to *_print_json() (or implement *_print_json_bare() to
  conditionally call from the above

> Object here also means any object reported in any delete kind,
> not just NFT_MSG_DELOBJ.  This applies to set elements etc. too,
> json expects the full info, but the kernel notifications no longer
> provide this.

Yes, and there is no proper way to detect these incomplete objects. So
the only way to fix it is to pass the message type into print routines.

Funny detail: JSON echo mode is unaffected because it ignores anything
that's not an add command. %)

> Alternative options?

Ignore the problem for now, I'll implement the proposed solution above
tomorrow. I hope with the patch at hand, we can decide whether this goes
into the release or not.

Sorry for the mess!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: nftables monitor json mode is broken
  2025-08-27 20:55 ` Phil Sutter
@ 2025-08-27 21:10   ` Phil Sutter
  2025-08-28  9:31     ` Florian Westphal
  0 siblings, 1 reply; 8+ messages in thread
From: Phil Sutter @ 2025-08-27 21:10 UTC (permalink / raw)
  To: Florian Westphal, netfilter-devel; +Cc: Pablo Neira Ayuso

On Wed, Aug 27, 2025 at 10:55:49PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Wed, Aug 27, 2025 at 07:12:36PM +0200, Florian Westphal wrote:
> > as subject says, 'nft monitor -j' is broken.
> > Example:
> > 
> > ./run-tests.sh -j testcases/object.t
> > monitor: running tests from file object.t
> > monitor output differs!
> > --- /tmp/tmp.emU4zIN8UT/tmp.C4TeyO6xYk  2025-08-27 19:05:08.039619097 +0200
> > +++ /tmp/tmp.emU4zIN8UT/tmp.jBOL3aIrp5  2025-08-27 19:05:09.062551248 +0200
> > @@ -1 +1 @@
> > -{"delete": {"quota": {"family": "ip", "name": "q", "table": "t", "handle": 0, "bytes": 26214400, "used": 0, "inv": false}}}
> > +{"delete": {"quota": {"family": "ip", "name": "q", "table": "t", "handle": 0, "bytes": 0, "used": 0, "inv": false}}}
> > monitor output differs!
> > --- /tmp/tmp.emU4zIN8UT/tmp.C4TeyO6xYk  2025-08-27 19:05:10.095619097 +0200
> > +++ /tmp/tmp.emU4zIN8UT/tmp.Guz55knY19  2025-08-27 19:05:11.117393075 +0200
> > @@ -1 +1 @@
> > -{"delete": {"limit": {"family": "ip", "name": "l", "table": "t", "handle": 0, "rate": 1, "per": "second", "burst": 5}}}
> > +{"delete": {"limit": {"family": "ip", "name": "l", "table": "t", "handle": 0, "rate": 0, "per": "error"}}}
> > 
> > I did notice this weeks ago but thought it was a problem on my end
> > and then didn't have time to investigate closer.
> > 
> > But its in fact broken on kernel side, since
> > 
> > netfilter: nf_tables: Reintroduce shortened deletion notifications
> > 
> > In short, unlike the normal output, json output wants to dump
> > everything, but the notifications no longer include the extra data, just
> > the bare minimum to identify the object being deleted.
> 
> Oh crap, I missed this entirely.
> 
> > As noone has complained so far I am inclinded to delete the
> > tests and rip out json support from monitor mode, it seems noone
> > uses it or even runs the tests for it.
> > 
> > Alternatives i see are:
> > 1. implement a cache and query it
> > 2. rework the json mode to be forgiving as to what is set
> >    and what isn't in the object.
> 
> My suggestion:
> - Add special casing for NFT_MSG_DEL* with NFTNL_OUTPUT_JSON as well in
>   monitor.c
> - Pass 'bool delete' to monitor_print_*_json()
> - And on to *_print_json() (or implement *_print_json_bare() to
>   conditionally call from the above
> 
> > Object here also means any object reported in any delete kind,
> > not just NFT_MSG_DELOBJ.  This applies to set elements etc. too,
> > json expects the full info, but the kernel notifications no longer
> > provide this.
> 
> Yes, and there is no proper way to detect these incomplete objects. So
> the only way to fix it is to pass the message type into print routines.
> 
> Funny detail: JSON echo mode is unaffected because it ignores anything
> that's not an add command. %)

Oh, and there is also the same issue regarding flowtable hook deletion
vs. flowtable deletion as fixed by commit 18ddac660dfa1 ("monitor:
Correctly print flowtable updates"). :(

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: nftables monitor json mode is broken
  2025-08-27 18:37   ` Pablo Neira Ayuso
@ 2025-08-28  9:29     ` Florian Westphal
  2025-08-28 12:27       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 8+ messages in thread
From: Florian Westphal @ 2025-08-28  9:29 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Why? Is unfixable to consider this?

I'm not sure.

It depends on several factors:
1. Do we have users of the json monitor mode?
2. Can they cope with *partial* info?
   For non-json, the user will be a human and they
   can the delete messages will have enough info to
   correlate it with the corresponding add messages.

   But for automated robots consuming json? Dunno.
3. Is the burden of correlating the delete info
   with the full information about the deleted object
   on the nft monitor -j side or the consumer of the
   (Then incomplete) json info?

> this is a relatively large rework, I started some code but is
> incomplete, including rule caching to deal with runtime incremental
> updates.

Thanks Pablo.

> I think it should be better to fix what we have then look pick back on
> the rework at some point.

I also prefer repair to "nuke it".
But I dislike the idea of spending time on something that is not
used in practice.

I refuse to believe there are people that prefer to
stare at "nft monitor -j"...

And if there a scripts that consume it, I don't understand
the use case.

Sorry if I was too terse in my initial complaint.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: nftables monitor json mode is broken
  2025-08-27 21:10   ` Phil Sutter
@ 2025-08-28  9:31     ` Florian Westphal
  0 siblings, 0 replies; 8+ messages in thread
From: Florian Westphal @ 2025-08-28  9:31 UTC (permalink / raw)
  To: Phil Sutter, netfilter-devel, Pablo Neira Ayuso

Phil Sutter <phil@nwl.cc> wrote:
> Oh, and there is also the same issue regarding flowtable hook deletion
> vs. flowtable deletion as fixed by commit 18ddac660dfa1 ("monitor:
> Correctly print flowtable updates"). :(

Yes, the json print functions can't handle the partial object states.
I also saw at least one segfault but did not investigate if it was
related to this issue or not.

Thanks for looking at this.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: nftables monitor json mode is broken
  2025-08-28  9:29     ` Florian Westphal
@ 2025-08-28 12:27       ` Pablo Neira Ayuso
  0 siblings, 0 replies; 8+ messages in thread
From: Pablo Neira Ayuso @ 2025-08-28 12:27 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

On Thu, Aug 28, 2025 at 11:29:14AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Why? Is unfixable to consider this?
> 
> I'm not sure.
> 
> It depends on several factors:
> 1. Do we have users of the json monitor mode?
> 2. Can they cope with *partial* info?
>    For non-json, the user will be a human and they
>    can the delete messages will have enough info to
>    correlate it with the corresponding add messages.
>    But for automated robots consuming json? Dunno.
> 3. Is the burden of correlating the delete info
>    with the full information about the deleted object
>    on the nft monitor -j side or the consumer of the
>    (Then incomplete) json info?

I don't think json output should diverge from the native monitor mode,
which only displays the partial information.

Then, for stateful objects such as counters, maybe there is a usecase
to display this in the delete object events, but then native nftables
monitor should display the same behaviour.

> > this is a relatively large rework, I started some code but is
> > incomplete, including rule caching to deal with runtime incremental
> > updates.
> 
> Thanks Pablo.
> 
> > I think it should be better to fix what we have then look pick back on
> > the rework at some point.
> 
> I also prefer repair to "nuke it".
> But I dislike the idea of spending time on something that is not
> used in practice.

I don't find a good reason to cripple json to make it less capable
than the native representation.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-08-28 12:27 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-27 17:12 nftables monitor json mode is broken Florian Westphal
2025-08-27 18:19 ` Pablo Neira Ayuso
2025-08-27 18:37   ` Pablo Neira Ayuso
2025-08-28  9:29     ` Florian Westphal
2025-08-28 12:27       ` Pablo Neira Ayuso
2025-08-27 20:55 ` Phil Sutter
2025-08-27 21:10   ` Phil Sutter
2025-08-28  9:31     ` Florian Westphal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).