[nft PATCH 0/5] Fixes (and fallout) from running tests/monitor in JSON mode

[nft PATCH 0/5] Fixes (and fallout) from running tests/monitor in JSON mode

[Phil Sutter]

The reported problem of object deletion notifications with bogus data is
resolved in patch four. It seems only object notifications were
affected. Patch 6 extends test suite coverage for tables and chains a
bit.

Patch one is an unrelated trivial fix, patches two and three cover for
oddities noticed while working on the actual problem.

Phil Sutter (5):
  tools: gitignore nftables.service file
  monitor: Quote device names in chain declarations, too
  mnl: Allow for updating devices on existing inet ingress hook chains
  monitor: Inform JSON printer when reporting an object delete event
  tests: monitor: Extend testcases a bit

 include/json.h                             |  5 +--
 src/evaluate.c                             |  6 ++--
 src/json.c                                 | 18 ++++++----
 src/mnl.c                                  |  2 ++
 src/monitor.c                              |  2 +-
 src/rule.c                                 |  2 +-
 tests/monitor/testcases/chain.t            | 41 ++++++++++++++++++++++
 tests/monitor/testcases/flowtable-simple.t | 12 +++++++
 tests/monitor/testcases/object.t           | 10 +++---
 tests/monitor/testcases/table.t            | 15 ++++++++
 tools/.gitignore                           |  1 +
 11 files changed, 97 insertions(+), 17 deletions(-)
 create mode 100644 tests/monitor/testcases/chain.t
 create mode 100644 tests/monitor/testcases/table.t
 create mode 100644 tools/.gitignore

-- 
2.51.0

[nft PATCH 1/5] tools: gitignore nftables.service file

[Phil Sutter]

Fixes: c4b17cf830510 ("tools: add a systemd unit for static rulesets")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tools/.gitignore | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 tools/.gitignore

diff --git a/tools/.gitignore b/tools/.gitignore
new file mode 100644
index 0000000000000..2d06c49835b15
--- /dev/null
+++ b/tools/.gitignore
@@ -0,0 +1 @@
+nftables.service
-- 
2.51.0

[nft PATCH 2/5] monitor: Quote device names in chain declarations, too

[Phil Sutter]

Fixed commit missed the fact that there are two routines printing chain
declarations.

Fixes: eb30f236d91a8 ("rule: print chain and flowtable devices in quotes")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/rule.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/rule.c b/src/rule.c
index ceb56488d37f6..d0a62a3ee002d 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1131,7 +1131,7 @@ void chain_print_plain(const struct chain *chain, struct output_ctx *octx)
 
 			nft_print(octx, "devices = { ");
 			for (i = 0; i < chain->dev_array_len; i++) {
-				nft_print(octx, "%s", chain->dev_array[i]);
+				nft_print(octx, "\"%s\"", chain->dev_array[i]);
 				if (i + 1 != chain->dev_array_len)
 					nft_print(octx, ", ");
 			}
-- 
2.51.0

[nft PATCH 3/5] mnl: Allow for updating devices on existing inet ingress hook chains

[Phil Sutter]

Complete commit a66b5ad9540dd ("src: allow for updating devices on
existing netdev chain") in supporting inet family ingress hook chains as
well. The kernel does already but nft has to add a proper hooknum
attribute to pass the checks.

The hook.num field has to be initialized from hook.name using
str2hooknum(), which is part of chain evaluation. Calling
chain_evaluate() just for that purpose is a bit over the top, but the
hook name lookup may fail and performing chain evaluation for delete
command as well fits more into the code layout than duplicating parts of
it in mnl_nft_chain_del() or elsewhere. Just avoid the
chain_cache_find() call as its assert() triggers when deleting by
handle and also don't add to be deleted chains to cache.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/evaluate.c | 6 ++++--
 src/mnl.c      | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index b7e4f71fdfbc9..db4ac18f1dc9f 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -5758,7 +5758,9 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
 		return table_not_found(ctx);
 
 	if (chain == NULL) {
-		if (!chain_cache_find(table, ctx->cmd->handle.chain.name)) {
+		if (ctx->cmd->op != CMD_DELETE &&
+		    ctx->cmd->op != CMD_DESTROY &&
+		    !chain_cache_find(table, ctx->cmd->handle.chain.name)) {
 			chain = chain_alloc();
 			handle_merge(&chain->handle, &ctx->cmd->handle);
 			chain_cache_add(chain, table);
@@ -6070,7 +6072,7 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
 		return 0;
 	case CMD_OBJ_CHAIN:
 		chain_del_cache(ctx, cmd);
-		return 0;
+		return chain_evaluate(ctx, cmd->chain);
 	case CMD_OBJ_TABLE:
 		table_del_cache(ctx, cmd);
 		return 0;
diff --git a/src/mnl.c b/src/mnl.c
index 984dcac27b1cf..d1402c0fcb9f4 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -994,6 +994,8 @@ int mnl_nft_chain_del(struct netlink_ctx *ctx, struct cmd *cmd)
 		struct nlattr *nest;
 
 		nest = mnl_attr_nest_start(nlh, NFTA_CHAIN_HOOK);
+		mnl_attr_put_u32(nlh, NFTA_HOOK_HOOKNUM,
+				 htonl(cmd->chain->hook.num));
 		mnl_nft_chain_devs_build(nlh, cmd);
 		mnl_attr_nest_end(nlh, nest);
 	}
-- 
2.51.0

[nft PATCH 4/5] monitor: Inform JSON printer when reporting an object delete event

[Phil Sutter]

Since kernel commit a1050dd07168 ("netfilter: nf_tables: Reintroduce
shortened deletion notifications"), type-specific data is no longer
dumped when notifying for a deleted object. JSON output was not aware of
this and tried to print bogus data.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/json.h                   |  5 +++--
 src/json.c                       | 18 ++++++++++++------
 src/monitor.c                    |  2 +-
 tests/monitor/testcases/object.t | 10 +++++-----
 4 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/include/json.h b/include/json.h
index 42e1c8616c975..3b8d045f87bbc 100644
--- a/include/json.h
+++ b/include/json.h
@@ -113,7 +113,7 @@ void monitor_print_set_json(struct netlink_mon_handler *monh,
 void monitor_print_element_json(struct netlink_mon_handler *monh,
 				const char *cmd, struct set *s);
 void monitor_print_obj_json(struct netlink_mon_handler *monh,
-			    const char *cmd, struct obj *o);
+			    const char *cmd, struct obj *o, bool delete);
 void monitor_print_flowtable_json(struct netlink_mon_handler *monh,
 				  const char *cmd, struct flowtable *ft);
 void monitor_print_rule_json(struct netlink_mon_handler *monh,
@@ -252,7 +252,8 @@ static inline void monitor_print_element_json(struct netlink_mon_handler *monh,
 }
 
 static inline void monitor_print_obj_json(struct netlink_mon_handler *monh,
-					  const char *cmd, struct obj *o)
+					  const char *cmd, struct obj *o,
+					  bool delete)
 {
 	/* empty */
 }
diff --git a/src/json.c b/src/json.c
index d06fd04027140..0afce5415f541 100644
--- a/src/json.c
+++ b/src/json.c
@@ -397,7 +397,8 @@ static json_t *tunnel_erspan_print_json(const struct obj *obj)
 	return tunnel;
 }
 
-static json_t *obj_print_json(struct output_ctx *octx, const struct obj *obj)
+static json_t *obj_print_json(struct output_ctx *octx, const struct obj *obj,
+			      bool delete)
 {
 	const char *rate_unit = NULL, *burst_unit = NULL;
 	const char *type = obj_type_name(obj->type);
@@ -410,6 +411,9 @@ static json_t *obj_print_json(struct output_ctx *octx, const struct obj *obj)
 			"table", obj->handle.table.name,
 			"handle", obj->handle.handle.id);
 
+	if (delete)
+		goto out;
+
 	if (obj->comment) {
 		tmp = nft_json_pack("{s:s}", "comment", obj->comment);
 		json_object_update(root, tmp);
@@ -570,6 +574,7 @@ static json_t *obj_print_json(struct output_ctx *octx, const struct obj *obj)
 		break;
 	}
 
+out:
 	return nft_json_pack("{s:o}", type, root);
 }
 
@@ -1815,7 +1820,7 @@ static json_t *table_print_json_full(struct netlink_ctx *ctx,
 		json_array_append_new(root, tmp);
 	}
 	list_for_each_entry(obj, &table->obj_cache.list, cache.list) {
-		tmp = obj_print_json(&ctx->nft->output, obj);
+		tmp = obj_print_json(&ctx->nft->output, obj, false);
 		json_array_append_new(root, tmp);
 	}
 	list_for_each_entry(set, &table->set_cache.list, cache.list) {
@@ -1971,7 +1976,7 @@ static json_t *do_list_sets_json(struct netlink_ctx *ctx, struct cmd *cmd)
 static json_t *do_list_obj_json(struct netlink_ctx *ctx,
 				struct cmd *cmd, uint32_t type)
 {
-	json_t *root = json_array();
+	json_t *root = json_array(), *tmp;
 	struct table *table;
 	struct obj *obj;
 
@@ -1990,7 +1995,8 @@ static json_t *do_list_obj_json(struct netlink_ctx *ctx,
 			     strcmp(cmd->handle.obj.name, obj->handle.obj.name)))
 				continue;
 
-			json_array_append_new(root, obj_print_json(&ctx->nft->output, obj));
+			tmp = obj_print_json(&ctx->nft->output, obj, false);
+			json_array_append_new(root, tmp);
 		}
 	}
 
@@ -2207,11 +2213,11 @@ void monitor_print_element_json(struct netlink_mon_handler *monh,
 }
 
 void monitor_print_obj_json(struct netlink_mon_handler *monh,
-			    const char *cmd, struct obj *o)
+			    const char *cmd, struct obj *o, bool delete)
 {
 	struct output_ctx *octx = &monh->ctx->nft->output;
 
-	monitor_print_json(monh, cmd, obj_print_json(octx, o));
+	monitor_print_json(monh, cmd, obj_print_json(octx, o, delete));
 }
 
 void monitor_print_flowtable_json(struct netlink_mon_handler *monh,
diff --git a/src/monitor.c b/src/monitor.c
index e58f62252ca2d..fafeeebe914b8 100644
--- a/src/monitor.c
+++ b/src/monitor.c
@@ -549,7 +549,7 @@ static int netlink_events_obj_cb(const struct nlmsghdr *nlh, int type,
 		nft_mon_print(monh, "\n");
 		break;
 	case NFTNL_OUTPUT_JSON:
-		monitor_print_obj_json(monh, cmd, obj);
+		monitor_print_obj_json(monh, cmd, obj, type == NFT_MSG_DELOBJ);
 		if (!nft_output_echo(&monh->ctx->nft->output))
 			nft_mon_print(monh, "\n");
 		break;
diff --git a/tests/monitor/testcases/object.t b/tests/monitor/testcases/object.t
index 53a9f8c59836b..b60dc9899ab2a 100644
--- a/tests/monitor/testcases/object.t
+++ b/tests/monitor/testcases/object.t
@@ -9,7 +9,7 @@ J {"add": {"counter": {"family": "ip", "name": "c", "table": "t", "handle": 0, "
 
 I delete counter ip t c
 O -
-J {"delete": {"counter": {"family": "ip", "name": "c", "table": "t", "handle": 0, "packets": 0, "bytes": 0}}}
+J {"delete": {"counter": {"family": "ip", "name": "c", "table": "t", "handle": 0}}}
 
 # FIXME: input/output shouldn't be asynchronous here
 I add quota ip t q 25 mbytes
@@ -18,7 +18,7 @@ J {"add": {"quota": {"family": "ip", "name": "q", "table": "t", "handle": 0, "by
 
 I delete quota ip t q
 O -
-J {"delete": {"quota": {"family": "ip", "name": "q", "table": "t", "handle": 0, "bytes": 26214400, "used": 0, "inv": false}}}
+J {"delete": {"quota": {"family": "ip", "name": "q", "table": "t", "handle": 0}}}
 
 # FIXME: input/output shouldn't be asynchronous here
 I add limit ip t l rate 1/second
@@ -27,7 +27,7 @@ J {"add": {"limit": {"family": "ip", "name": "l", "table": "t", "handle": 0, "ra
 
 I delete limit ip t l
 O -
-J {"delete": {"limit": {"family": "ip", "name": "l", "table": "t", "handle": 0, "rate": 1, "per": "second", "burst": 5}}}
+J {"delete": {"limit": {"family": "ip", "name": "l", "table": "t", "handle": 0}}}
 
 I add ct helper ip t cth { type "sip" protocol tcp; l3proto ip; }
 O -
@@ -35,7 +35,7 @@ J {"add": {"ct helper": {"family": "ip", "name": "cth", "table": "t", "handle":
 
 I delete ct helper ip t cth
 O -
-J {"delete": {"ct helper": {"family": "ip", "name": "cth", "table": "t", "handle": 0, "type": "sip", "protocol": "tcp", "l3proto": "ip"}}}
+J {"delete": {"ct helper": {"family": "ip", "name": "cth", "table": "t", "handle": 0}}}
 
 I add ct timeout ip t ctt { protocol udp; l3proto ip; policy = { unreplied : 15s, replied : 12s }; }
 O -
@@ -43,4 +43,4 @@ J {"add": {"ct timeout": {"family": "ip", "name": "ctt", "table": "t", "handle":
 
 I delete ct timeout ip t ctt
 O -
-J {"delete": {"ct timeout": {"family": "ip", "name": "ctt", "table": "t", "handle": 0, "protocol": "udp", "l3proto": "ip", "policy": {"unreplied": 15, "replied": 12}}}}
+J {"delete": {"ct timeout": {"family": "ip", "name": "ctt", "table": "t", "handle": 0}}}
-- 
2.51.0

[nft PATCH 5/5] tests: monitor: Extend testcases a bit

[Phil Sutter]

Try to cover for reduced table and chain deletion notifications by
creating them with data which is omitted by the kernel during deletion.

Also try to expose the difference in reported flowtable hook deletion
vs. flowtable deletion.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tests/monitor/testcases/chain.t            | 41 ++++++++++++++++++++++
 tests/monitor/testcases/flowtable-simple.t | 12 +++++++
 tests/monitor/testcases/table.t            | 15 ++++++++
 3 files changed, 68 insertions(+)
 create mode 100644 tests/monitor/testcases/chain.t
 create mode 100644 tests/monitor/testcases/table.t

diff --git a/tests/monitor/testcases/chain.t b/tests/monitor/testcases/chain.t
new file mode 100644
index 0000000000000..975ccf1d33919
--- /dev/null
+++ b/tests/monitor/testcases/chain.t
@@ -0,0 +1,41 @@
+I add table inet t
+O -
+J {"add": {"table": {"family": "inet", "name": "t", "handle": 0}}}
+
+I add chain inet t c
+O -
+J {"add": {"chain": {"family": "inet", "table": "t", "name": "c", "handle": 0}}}
+
+I delete chain inet t c
+O -
+J {"delete": {"chain": {"family": "inet", "table": "t", "name": "c", "handle": 0}}}
+
+I add chain inet t c { type filter hook input priority filter; }
+O add chain inet t c { type filter hook input priority 0; policy accept; }
+J {"add": {"chain": {"family": "inet", "table": "t", "name": "c", "handle": 0, "type": "filter", "hook": "input", "prio": 0, "policy": "accept"}}}
+
+I delete chain inet t c
+O -
+J {"delete": {"chain": {"family": "inet", "table": "t", "name": "c", "handle": 0}}}
+
+I add chain inet t c { type filter hook ingress priority filter; devices = { "lo" }; }
+O add chain inet t c { type filter hook ingress devices = { "lo" } priority 0; policy accept; }
+J {"add": {"chain": {"family": "inet", "table": "t", "name": "c", "handle": 0, "dev": "lo", "type": "filter", "hook": "ingress", "prio": 0, "policy": "accept"}}}
+
+I delete chain inet t c
+O -
+J {"delete": {"chain": {"family": "inet", "table": "t", "name": "c", "handle": 0}}}
+
+I add chain inet t c { type filter hook ingress priority filter; devices = { "eth1", "lo" }; }
+O add chain inet t c { type filter hook ingress devices = { "eth1", "lo" } priority 0; policy accept; }
+J {"add": {"chain": {"family": "inet", "table": "t", "name": "c", "handle": 0, "dev": ["eth1", "lo"], "type": "filter", "hook": "ingress", "prio": 0, "policy": "accept"}}}
+
+I delete chain inet t c { type filter hook ingress priority filter; devices = { "eth1" }; }
+O delete chain inet t c { type filter hook ingress devices = { "eth1" } priority 0; policy accept; }
+J {"delete": {"chain": {"family": "inet", "table": "t", "name": "c", "handle": 0, "dev": "eth1", "type": "filter", "hook": "ingress", "prio": 0, "policy": "accept"}}}
+
+I delete chain inet t c
+O -
+J {"delete": {"chain": {"family": "inet", "table": "t", "name": "c", "handle": 0}}}
+
+
diff --git a/tests/monitor/testcases/flowtable-simple.t b/tests/monitor/testcases/flowtable-simple.t
index 11254c51fcab7..e1889ae5d0076 100644
--- a/tests/monitor/testcases/flowtable-simple.t
+++ b/tests/monitor/testcases/flowtable-simple.t
@@ -8,3 +8,15 @@ J {"add": {"flowtable": {"family": "ip", "name": "ft", "table": "t", "handle": 0
 I delete flowtable ip t ft
 O -
 J {"delete": {"flowtable": {"family": "ip", "name": "ft", "table": "t", "handle": 0}}}
+
+I add flowtable ip t ft { hook ingress priority 0; devices = { "eth1", "lo" }; }
+O -
+J {"add": {"flowtable": {"family": "ip", "name": "ft", "table": "t", "handle": 0, "hook": "ingress", "prio": 0, "dev": ["eth1", "lo"]}}}
+
+I delete flowtable ip t ft { hook ingress priority 0; devices = { "eth1" }; }
+O -
+J {"delete": {"flowtable": {"family": "ip", "name": "ft", "table": "t", "handle": 0, "hook": "ingress", "prio": 0, "dev": "eth1"}}}
+
+I delete flowtable ip t ft
+O -
+J {"delete": {"flowtable": {"family": "ip", "name": "ft", "table": "t", "handle": 0}}}
diff --git a/tests/monitor/testcases/table.t b/tests/monitor/testcases/table.t
new file mode 100644
index 0000000000000..35a0f510436dc
--- /dev/null
+++ b/tests/monitor/testcases/table.t
@@ -0,0 +1,15 @@
+I add table ip t
+O -
+J {"add": {"table": {"family": "ip", "name": "t", "handle": 0}}}
+
+I delete table ip t
+O -
+J {"delete": {"table": {"family": "ip", "name": "t", "handle": 0}}}
+
+I add table ip t { comment "foo bar"; flags dormant; }
+O add table ip t { flags dormant; }
+J {"add": {"table": {"family": "ip", "name": "t", "handle": 0, "flags": ["dormant"], "comment": "foo bar"}}}
+
+I delete table ip t
+O -
+J {"delete": {"table": {"family": "ip", "name": "t", "handle": 0}}}
-- 
2.51.0

Re: [nft PATCH 0/5] Fixes (and fallout) from running tests/monitor in JSON mode

[Phil Sutter]

On Fri, Aug 29, 2025 at 04:25:08PM +0200, Phil Sutter wrote:
> The reported problem of object deletion notifications with bogus data is
> resolved in patch four. It seems only object notifications were
> affected. Patch 6 extends test suite coverage for tables and chains a
> bit.
> 
> Patch one is an unrelated trivial fix, patches two and three cover for
> oddities noticed while working on the actual problem.
> 
> Phil Sutter (5):
>   tools: gitignore nftables.service file
>   monitor: Quote device names in chain declarations, too

Applied these two unrelated trivial fixes.

>   mnl: Allow for updating devices on existing inet ingress hook chains
>   monitor: Inform JSON printer when reporting an object delete event
>   tests: monitor: Extend testcases a bit
> 
>  include/json.h                             |  5 +--
>  src/evaluate.c                             |  6 ++--
>  src/json.c                                 | 18 ++++++----
>  src/mnl.c                                  |  2 ++
>  src/monitor.c                              |  2 +-
>  src/rule.c                                 |  2 +-
>  tests/monitor/testcases/chain.t            | 41 ++++++++++++++++++++++
>  tests/monitor/testcases/flowtable-simple.t | 12 +++++++
>  tests/monitor/testcases/object.t           | 10 +++---
>  tests/monitor/testcases/table.t            | 15 ++++++++
>  tools/.gitignore                           |  1 +
>  11 files changed, 97 insertions(+), 17 deletions(-)
>  create mode 100644 tests/monitor/testcases/chain.t
>  create mode 100644 tests/monitor/testcases/table.t
>  create mode 100644 tools/.gitignore
> 
> -- 
> 2.51.0
> 
> 
> 

Re: [nft PATCH 3/5] mnl: Allow for updating devices on existing inet ingress hook chains

[Pablo Neira Ayuso]

On Fri, Aug 29, 2025 at 04:25:11PM +0200, Phil Sutter wrote:
> Complete commit a66b5ad9540dd ("src: allow for updating devices on
> existing netdev chain") in supporting inet family ingress hook chains as
> well. The kernel does already but nft has to add a proper hooknum
> attribute to pass the checks.
> 
> The hook.num field has to be initialized from hook.name using
> str2hooknum(), which is part of chain evaluation. Calling
> chain_evaluate() just for that purpose is a bit over the top, but the
> hook name lookup may fail and performing chain evaluation for delete
> command as well fits more into the code layout than duplicating parts of
> it in mnl_nft_chain_del() or elsewhere. Just avoid the
> chain_cache_find() call as its assert() triggers when deleting by
> handle and also don't add to be deleted chains to cache.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/evaluate.c | 6 ++++--
>  src/mnl.c      | 2 ++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index b7e4f71fdfbc9..db4ac18f1dc9f 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -5758,7 +5758,9 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
>  		return table_not_found(ctx);
>  
>  	if (chain == NULL) {
> -		if (!chain_cache_find(table, ctx->cmd->handle.chain.name)) {
> +		if (ctx->cmd->op != CMD_DELETE &&
> +		    ctx->cmd->op != CMD_DESTROY &&
> +		    !chain_cache_find(table, ctx->cmd->handle.chain.name)) {
>  			chain = chain_alloc();
>  			handle_merge(&chain->handle, &ctx->cmd->handle);
>  			chain_cache_add(chain, table);
> @@ -6070,7 +6072,7 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
>  		return 0;
>  	case CMD_OBJ_CHAIN:
>  		chain_del_cache(ctx, cmd);
> -		return 0;
> +		return chain_evaluate(ctx, cmd->chain);

Maybe fix this to perform chain_del_cache() after chain_evaluate()?
ie.

                if (chain_evaluate(ctx, cmd->chain) < 0)
                        return -1;

                chain_del_cache(ctx, cmd);
                return 0;

>  	case CMD_OBJ_TABLE:
>  		table_del_cache(ctx, cmd);
>  		return 0;
> diff --git a/src/mnl.c b/src/mnl.c
> index 984dcac27b1cf..d1402c0fcb9f4 100644
> --- a/src/mnl.c
> +++ b/src/mnl.c
> @@ -994,6 +994,8 @@ int mnl_nft_chain_del(struct netlink_ctx *ctx, struct cmd *cmd)
>  		struct nlattr *nest;
>  
>  		nest = mnl_attr_nest_start(nlh, NFTA_CHAIN_HOOK);
> +		mnl_attr_put_u32(nlh, NFTA_HOOK_HOOKNUM,
> +				 htonl(cmd->chain->hook.num));
>  		mnl_nft_chain_devs_build(nlh, cmd);
>  		mnl_attr_nest_end(nlh, nest);
>  	}
> -- 
> 2.51.0
> 

Re: [nft PATCH 4/5] monitor: Inform JSON printer when reporting an object delete event

[Pablo Neira Ayuso]

On Fri, Aug 29, 2025 at 04:25:12PM +0200, Phil Sutter wrote:
> Since kernel commit a1050dd07168 ("netfilter: nf_tables: Reintroduce
> shortened deletion notifications"), type-specific data is no longer
> dumped when notifying for a deleted object. JSON output was not aware of
> this and tried to print bogus data.

Fixes: e70354f53e9f ("libnftables: Implement JSON output support")

> Signed-off-by: Phil Sutter <phil@nwl.cc>

Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>

Re: [nft PATCH 5/5] tests: monitor: Extend testcases a bit

[Pablo Neira Ayuso]

On Fri, Aug 29, 2025 at 04:25:13PM +0200, Phil Sutter wrote:
> Try to cover for reduced table and chain deletion notifications by
> creating them with data which is omitted by the kernel during deletion.
> 
> Also try to expose the difference in reported flowtable hook deletion
> vs. flowtable deletion.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Thanks.

Re: [nft PATCH 3/5] mnl: Allow for updating devices on existing inet ingress hook chains

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 2322 bytes --]

On Mon, Sep 08, 2025 at 12:29:03PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Aug 29, 2025 at 04:25:11PM +0200, Phil Sutter wrote:
> > Complete commit a66b5ad9540dd ("src: allow for updating devices on
> > existing netdev chain") in supporting inet family ingress hook chains as
> > well. The kernel does already but nft has to add a proper hooknum
> > attribute to pass the checks.
> > 
> > The hook.num field has to be initialized from hook.name using
> > str2hooknum(), which is part of chain evaluation. Calling
> > chain_evaluate() just for that purpose is a bit over the top, but the
> > hook name lookup may fail and performing chain evaluation for delete
> > command as well fits more into the code layout than duplicating parts of
> > it in mnl_nft_chain_del() or elsewhere. Just avoid the
> > chain_cache_find() call as its assert() triggers when deleting by
> > handle and also don't add to be deleted chains to cache.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  src/evaluate.c | 6 ++++--
> >  src/mnl.c      | 2 ++
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/evaluate.c b/src/evaluate.c
> > index b7e4f71fdfbc9..db4ac18f1dc9f 100644
> > --- a/src/evaluate.c
> > +++ b/src/evaluate.c
> > @@ -5758,7 +5758,9 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
> >  		return table_not_found(ctx);
> >  
> >  	if (chain == NULL) {
> > -		if (!chain_cache_find(table, ctx->cmd->handle.chain.name)) {
> > +		if (ctx->cmd->op != CMD_DELETE &&
> > +		    ctx->cmd->op != CMD_DESTROY &&
> > +		    !chain_cache_find(table, ctx->cmd->handle.chain.name)) {
> >  			chain = chain_alloc();
> >  			handle_merge(&chain->handle, &ctx->cmd->handle);
> >  			chain_cache_add(chain, table);
> > @@ -6070,7 +6072,7 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
> >  		return 0;
> >  	case CMD_OBJ_CHAIN:
> >  		chain_del_cache(ctx, cmd);
> > -		return 0;
> > +		return chain_evaluate(ctx, cmd->chain);
> 
> Maybe fix this to perform chain_del_cache() after chain_evaluate()?
> ie.
> 
>                 if (chain_evaluate(ctx, cmd->chain) < 0)
>                         return -1;
> 
>                 chain_del_cache(ctx, cmd);

My suggestion won't work.

Maybe add a specific chain_del_evaluate(), see untested patch attached.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 1191 bytes --]

diff --git a/src/evaluate.c b/src/evaluate.c
index d4971228b61f..7c72a453d0f3 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -5722,6 +5722,37 @@ static uint32_t str2hooknum(uint32_t family, const char *hook)
 	return NF_INET_NUMHOOKS;
 }
 
+static int chain_del_evaluate(struct eval_ctx *ctx, struct chain *chain)
+{
+	struct table *table;
+
+	table = table_cache_find(&ctx->nft->cache.table_cache,
+				 ctx->cmd->handle.table.name,
+				 ctx->cmd->handle.family);
+	if (table == NULL)
+		return table_not_found(ctx);
+
+	if (chain->dev_expr) {
+		if (!(chain->flags & CHAIN_F_BASECHAIN))
+			chain->flags |= CHAIN_F_BASECHAIN;
+
+		if (chain->handle.family == NFPROTO_NETDEV ||
+		    (chain->handle.family == NFPROTO_INET &&
+		     chain->hook.num == NF_INET_INGRESS)) {
+			if (chain->dev_expr &&
+			    !evaluate_device_expr(ctx, &chain->dev_expr))
+				return -1;
+		} else if (chain->dev_expr) {
+			return __stmt_binary_error(ctx, &chain->dev_expr->location, NULL,
+						   "This chain type cannot be bound to device");
+		}
+	}
+
+	chain_del_cache(ctx, cmd);
+
+	return 0;
+}
+
 static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
 {
 	struct table *table;

Re: [nft PATCH 3/5] mnl: Allow for updating devices on existing inet ingress hook chains

[Phil Sutter]

On Mon, Sep 08, 2025 at 01:01:00PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Sep 08, 2025 at 12:29:03PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Aug 29, 2025 at 04:25:11PM +0200, Phil Sutter wrote:
> > > Complete commit a66b5ad9540dd ("src: allow for updating devices on
> > > existing netdev chain") in supporting inet family ingress hook chains as
> > > well. The kernel does already but nft has to add a proper hooknum
> > > attribute to pass the checks.
> > > 
> > > The hook.num field has to be initialized from hook.name using
> > > str2hooknum(), which is part of chain evaluation. Calling
> > > chain_evaluate() just for that purpose is a bit over the top, but the
> > > hook name lookup may fail and performing chain evaluation for delete
> > > command as well fits more into the code layout than duplicating parts of
> > > it in mnl_nft_chain_del() or elsewhere. Just avoid the
> > > chain_cache_find() call as its assert() triggers when deleting by
> > > handle and also don't add to be deleted chains to cache.
> > > 
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > >  src/evaluate.c | 6 ++++--
> > >  src/mnl.c      | 2 ++
> > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/evaluate.c b/src/evaluate.c
> > > index b7e4f71fdfbc9..db4ac18f1dc9f 100644
> > > --- a/src/evaluate.c
> > > +++ b/src/evaluate.c
> > > @@ -5758,7 +5758,9 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
> > >  		return table_not_found(ctx);
> > >  
> > >  	if (chain == NULL) {
> > > -		if (!chain_cache_find(table, ctx->cmd->handle.chain.name)) {
> > > +		if (ctx->cmd->op != CMD_DELETE &&
> > > +		    ctx->cmd->op != CMD_DESTROY &&
> > > +		    !chain_cache_find(table, ctx->cmd->handle.chain.name)) {
> > >  			chain = chain_alloc();
> > >  			handle_merge(&chain->handle, &ctx->cmd->handle);
> > >  			chain_cache_add(chain, table);
> > > @@ -6070,7 +6072,7 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
> > >  		return 0;
> > >  	case CMD_OBJ_CHAIN:
> > >  		chain_del_cache(ctx, cmd);
> > > -		return 0;
> > > +		return chain_evaluate(ctx, cmd->chain);
> > 
> > Maybe fix this to perform chain_del_cache() after chain_evaluate()?

I agree, side-effects of reusing chain_evaluate() for deletion are not
worth it.

> > ie.
> > 
> >                 if (chain_evaluate(ctx, cmd->chain) < 0)
> >                         return -1;
> > 
> >                 chain_del_cache(ctx, cmd);
> 
> My suggestion won't work.
> 
> Maybe add a specific chain_del_evaluate(), see untested patch attached.

Since we only need a proper value in chain->hook.num, a more minimal
version is fine:

diff --git a/src/evaluate.c b/src/evaluate.c
index b7e4f71fdfbc9..8cecbe09de01c 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -5992,6 +5992,22 @@ static void chain_del_cache(struct eval_ctx *ctx, struct cmd *cmd)
        chain_free(chain);
 }
 
+static int chain_del_evaluate(struct eval_ctx *ctx, struct cmd *cmd)
+{
+       struct chain *chain = cmd->chain;
+
+       if (chain && chain->flags & CHAIN_F_BASECHAIN && chain->hook.name) {
+               chain->hook.num = str2hooknum(chain->handle.family,
+                                             chain->hook.name);
+               if (chain->hook.num == NF_INET_NUMHOOKS)
+                       return __stmt_binary_error(ctx, &chain->hook.loc, NULL,
+                                                  "The %s family does not support this hook",
+                                                  family2str(chain->handle.family));
+       }
+       chain_del_cache(ctx, cmd);
+       return 0;
+}
+
 static void set_del_cache(struct eval_ctx *ctx, struct cmd *cmd)
 {
        struct table *table;
@@ -6069,8 +6085,7 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
        case CMD_OBJ_RULE:
                return 0;
        case CMD_OBJ_CHAIN:
-               chain_del_cache(ctx, cmd);
-               return 0;
+               return chain_del_evaluate(ctx, cmd);
        case CMD_OBJ_TABLE:
                table_del_cache(ctx, cmd);
                return 0;

Fine with you?

Cheers, Phil

Re: [nft PATCH 3/5] mnl: Allow for updating devices on existing inet ingress hook chains

[Pablo Neira Ayuso]

On Tue, Sep 09, 2025 at 01:48:44AM +0200, Phil Sutter wrote:
> On Mon, Sep 08, 2025 at 01:01:00PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Sep 08, 2025 at 12:29:03PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Aug 29, 2025 at 04:25:11PM +0200, Phil Sutter wrote:
> > > > Complete commit a66b5ad9540dd ("src: allow for updating devices on
> > > > existing netdev chain") in supporting inet family ingress hook chains as
> > > > well. The kernel does already but nft has to add a proper hooknum
> > > > attribute to pass the checks.
> > > > 
> > > > The hook.num field has to be initialized from hook.name using
> > > > str2hooknum(), which is part of chain evaluation. Calling
> > > > chain_evaluate() just for that purpose is a bit over the top, but the
> > > > hook name lookup may fail and performing chain evaluation for delete
> > > > command as well fits more into the code layout than duplicating parts of
> > > > it in mnl_nft_chain_del() or elsewhere. Just avoid the
> > > > chain_cache_find() call as its assert() triggers when deleting by
> > > > handle and also don't add to be deleted chains to cache.
> > > > 
> > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > ---
> > > >  src/evaluate.c | 6 ++++--
> > > >  src/mnl.c      | 2 ++
> > > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/src/evaluate.c b/src/evaluate.c
> > > > index b7e4f71fdfbc9..db4ac18f1dc9f 100644
> > > > --- a/src/evaluate.c
> > > > +++ b/src/evaluate.c
> > > > @@ -5758,7 +5758,9 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
> > > >  		return table_not_found(ctx);
> > > >  
> > > >  	if (chain == NULL) {
> > > > -		if (!chain_cache_find(table, ctx->cmd->handle.chain.name)) {
> > > > +		if (ctx->cmd->op != CMD_DELETE &&
> > > > +		    ctx->cmd->op != CMD_DESTROY &&
> > > > +		    !chain_cache_find(table, ctx->cmd->handle.chain.name)) {
> > > >  			chain = chain_alloc();
> > > >  			handle_merge(&chain->handle, &ctx->cmd->handle);
> > > >  			chain_cache_add(chain, table);
> > > > @@ -6070,7 +6072,7 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
> > > >  		return 0;
> > > >  	case CMD_OBJ_CHAIN:
> > > >  		chain_del_cache(ctx, cmd);
> > > > -		return 0;
> > > > +		return chain_evaluate(ctx, cmd->chain);
> > > 
> > > Maybe fix this to perform chain_del_cache() after chain_evaluate()?
> 
> I agree, side-effects of reusing chain_evaluate() for deletion are not
> worth it.
> 
> > > ie.
> > > 
> > >                 if (chain_evaluate(ctx, cmd->chain) < 0)
> > >                         return -1;
> > > 
> > >                 chain_del_cache(ctx, cmd);
> > 
> > My suggestion won't work.
> > 
> > Maybe add a specific chain_del_evaluate(), see untested patch attached.
> 
> Since we only need a proper value in chain->hook.num, a more minimal
> version is fine:
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index b7e4f71fdfbc9..8cecbe09de01c 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -5992,6 +5992,22 @@ static void chain_del_cache(struct eval_ctx *ctx, struct cmd *cmd)
>         chain_free(chain);
>  }
>  
> +static int chain_del_evaluate(struct eval_ctx *ctx, struct cmd *cmd)
> +{
> +       struct chain *chain = cmd->chain;
> +
> +       if (chain && chain->flags & CHAIN_F_BASECHAIN && chain->hook.name) {
> +               chain->hook.num = str2hooknum(chain->handle.family,
> +                                             chain->hook.name);
> +               if (chain->hook.num == NF_INET_NUMHOOKS)
> +                       return __stmt_binary_error(ctx, &chain->hook.loc, NULL,
> +                                                  "The %s family does not support this hook",
> +                                                  family2str(chain->handle.family));
> +       }
> +       chain_del_cache(ctx, cmd);
> +       return 0;
> +}
> +
>  static void set_del_cache(struct eval_ctx *ctx, struct cmd *cmd)
>  {
>         struct table *table;
> @@ -6069,8 +6085,7 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
>         case CMD_OBJ_RULE:
>                 return 0;
>         case CMD_OBJ_CHAIN:
> -               chain_del_cache(ctx, cmd);
> -               return 0;
> +               return chain_del_evaluate(ctx, cmd);
>         case CMD_OBJ_TABLE:
>                 table_del_cache(ctx, cmd);
>                 return 0;
> 
> Fine with you?

Yes, thanks!

Re: [nft PATCH 4/5] monitor: Inform JSON printer when reporting an object delete event

[Phil Sutter]

On Mon, Sep 08, 2025 at 12:30:05PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Aug 29, 2025 at 04:25:12PM +0200, Phil Sutter wrote:
> > Since kernel commit a1050dd07168 ("netfilter: nf_tables: Reintroduce
> > shortened deletion notifications"), type-specific data is no longer
> > dumped when notifying for a deleted object. JSON output was not aware of
> > this and tried to print bogus data.
> 
> Fixes: e70354f53e9f ("libnftables: Implement JSON output support")

A Fixes: tag is fine with me, although one could argue that output was
fine before said kernel change. I'd rather refer to 9e88aae28e9f4
("monitor: Use libnftables JSON output") instead as this introduced the
relevant code in src/monitor.c.

> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> 
> Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>

Thanks, Phil

Re: [nft PATCH 0/5] Fixes (and fallout) from running tests/monitor in JSON mode

[Phil Sutter]

On Tue, Sep 02, 2025 at 12:57:55PM +0200, Phil Sutter wrote:
> On Fri, Aug 29, 2025 at 04:25:08PM +0200, Phil Sutter wrote:
> > The reported problem of object deletion notifications with bogus data is
> > resolved in patch four. It seems only object notifications were
> > affected. Patch 6 extends test suite coverage for tables and chains a
> > bit.
> > 
> > Patch one is an unrelated trivial fix, patches two and three cover for
> > oddities noticed while working on the actual problem.
> > 
> > Phil Sutter (5):
> >   tools: gitignore nftables.service file
> >   monitor: Quote device names in chain declarations, too
> 
> Applied these two unrelated trivial fixes.
> 
> >   mnl: Allow for updating devices on existing inet ingress hook chains
> >   monitor: Inform JSON printer when reporting an object delete event
> >   tests: monitor: Extend testcases a bit

Applied the remaining three patches as well after:

* Adding the proposed minimal chain_del_evaluate() to the first one
* Adding a Fixes: tag to the second one