[nft PATCH 0/4] Continue upon netlink deserialization failures

[nft PATCH 0/4] Continue upon netlink deserialization failures

[Phil Sutter]

Faced with unexpected values or missing attributes, most of the netlink
deserialization code would complain, drop the nftables object being
constructed and carry on. Some error paths though instead call BUG() or
assert(0) instead. This series eliminates at least the most prominent
ones among those (patches 1 and 3).

Patch 4 prevents object deserialization from aborting upon the first one
with unexpected data. If netlink_delinearize_obj() returns NULL, an
error message has been emitted already so just return 0 to the foreach
loop so it continues with the next object.

Patch 2 is just preparation work for patch 3.

Phil Sutter (4):
  netlink_delinearize: Replace some BUG()s by error messages
  netlink: Pass netlink_ctx to netlink_delinearize_setelem()
  netlink: Keep going after set element parsing failures
  cache: Tolerate object deserialization failures

 include/netlink.h         |  6 +++---
 src/cache.c               | 11 +++++------
 src/monitor.c             |  7 +++----
 src/netlink.c             | 15 +++++++++------
 src/netlink_delinearize.c | 17 +++++++++++------
 5 files changed, 31 insertions(+), 25 deletions(-)

-- 
2.49.0

[nft PATCH 1/4] netlink_delinearize: Replace some BUG()s by error messages

[Phil Sutter]

Netlink parser tries to keep going despite errors. Faced with an
incompatible ruleset, this is much more user-friendly than exiting the
program upon the first obstacle. This patch fixes three more spots to
support this.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/netlink_delinearize.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index f7c10fb5af316..79bef0bb299ab 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -605,7 +605,8 @@ static void netlink_parse_bitwise(struct netlink_parse_ctx *ctx,
 						   sreg, left);
 		break;
 	default:
-		BUG("invalid bitwise operation %u\n", op);
+		return netlink_error(ctx, loc,
+				     "Invalid bitwise operation %u", op);
 	}
 
 	dreg = netlink_parse_register(nle, NFTNL_EXPR_BITWISE_DREG);
@@ -616,6 +617,7 @@ static void netlink_parse_byteorder(struct netlink_parse_ctx *ctx,
 				    const struct location *loc,
 				    const struct nftnl_expr *nle)
 {
+	uint32_t opval = nftnl_expr_get_u32(nle, NFTNL_EXPR_BYTEORDER_OP);
 	enum nft_registers sreg, dreg;
 	struct expr *expr, *arg;
 	enum ops op;
@@ -627,7 +629,7 @@ static void netlink_parse_byteorder(struct netlink_parse_ctx *ctx,
 				     "Byteorder expression has no left "
 				     "hand side");
 
-	switch (nftnl_expr_get_u32(nle, NFTNL_EXPR_BYTEORDER_OP)) {
+	switch (opval) {
 	case NFT_BYTEORDER_NTOH:
 		op = OP_NTOH;
 		break;
@@ -635,8 +637,9 @@ static void netlink_parse_byteorder(struct netlink_parse_ctx *ctx,
 		op = OP_HTON;
 		break;
 	default:
-		BUG("invalid byteorder operation %u\n",
-		    nftnl_expr_get_u32(nle, NFTNL_EXPR_BYTEORDER_OP));
+		expr_free(arg);
+		return netlink_error(ctx, loc,
+				     "Invalid byteorder operation %u\n", opval);
 	}
 
 	expr = unary_expr_alloc(loc, op, arg);
@@ -733,8 +736,10 @@ static void netlink_parse_inner(struct netlink_parse_ctx *ctx,
 		expr->meta.inner_desc = inner_desc;
 		break;
 	default:
-		assert(0);
-		break;
+		netlink_error(ctx, loc, "Unsupported inner expression type %s",
+			      expr_ops(expr)->name);
+		expr_free(expr);
+		return;
 	}
 
 	netlink_set_register(ctx, ctx->inner_reg, expr);
-- 
2.49.0

[nft PATCH 2/4] netlink: Pass netlink_ctx to netlink_delinearize_setelem()

[Phil Sutter]

Prepare for calling netlink_io_error() which needs the context pointer.
Trade this in for the cache pointer since no caller uses a special one.
No functional change intended.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/netlink.h |  6 +++---
 src/monitor.c     |  7 +++----
 src/netlink.c     | 11 ++++++-----
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/netlink.h b/include/netlink.h
index e9667a24b0d11..c7da6f9e3bcbb 100644
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -172,9 +172,9 @@ extern int netlink_list_setelems(struct netlink_ctx *ctx,
 extern int netlink_get_setelem(struct netlink_ctx *ctx, const struct handle *h,
 			       const struct location *loc, struct set *cache_set,
 			       struct set *set, struct expr *init, bool reset);
-extern int netlink_delinearize_setelem(struct nftnl_set_elem *nlse,
-				       struct set *set,
-				       struct nft_cache *cache);
+extern int netlink_delinearize_setelem(struct netlink_ctx *ctx,
+				       struct nftnl_set_elem *nlse,
+				       struct set *set);
 
 extern int netlink_list_objs(struct netlink_ctx *ctx, const struct handle *h);
 extern struct obj *netlink_delinearize_obj(struct netlink_ctx *ctx,
diff --git a/src/monitor.c b/src/monitor.c
index 0db40895babf9..7556dfd350e4e 100644
--- a/src/monitor.c
+++ b/src/monitor.c
@@ -472,8 +472,8 @@ static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
 			nftnl_set_elems_iter_destroy(nlsei);
 			goto out;
 		}
-		if (netlink_delinearize_setelem(nlse, dummyset,
-						&monh->ctx->nft->cache) < 0) {
+		if (netlink_delinearize_setelem(monh->ctx,
+						nlse, dummyset) < 0) {
 			set_free(dummyset);
 			nftnl_set_elems_iter_destroy(nlsei);
 			goto out;
@@ -829,8 +829,7 @@ static void netlink_events_cache_addsetelem(struct netlink_mon_handler *monh,
 
 	nlse = nftnl_set_elems_iter_next(nlsei);
 	while (nlse != NULL) {
-		if (netlink_delinearize_setelem(nlse, set,
-						&monh->ctx->nft->cache) < 0) {
+		if (netlink_delinearize_setelem(monh->ctx, nlse, set) < 0) {
 			fprintf(stderr,
 				"W: Unable to cache set_elem. "
 				"Delinearize failed.\n");
diff --git a/src/netlink.c b/src/netlink.c
index fe02120def035..1222919458bae 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -1435,12 +1435,13 @@ static void set_elem_parse_udata(struct nftnl_set_elem *nlse,
 	}
 }
 
-int netlink_delinearize_setelem(struct nftnl_set_elem *nlse,
-				struct set *set, struct nft_cache *cache)
+int netlink_delinearize_setelem(struct netlink_ctx *ctx,
+				struct nftnl_set_elem *nlse,
+				struct set *set)
 {
 	struct setelem_parse_ctx setelem_parse_ctx = {
 		.set	= set,
-		.cache	= cache,
+		.cache	= &ctx->nft->cache,
 	};
 	struct nft_data_delinearize nld;
 	struct expr *expr, *key, *data;
@@ -1498,7 +1499,7 @@ int netlink_delinearize_setelem(struct nftnl_set_elem *nlse,
 		struct stmt *stmt;
 
 		nle = nftnl_set_elem_get(nlse, NFTNL_SET_ELEM_EXPR, NULL);
-		stmt = netlink_parse_set_expr(set, cache, nle);
+		stmt = netlink_parse_set_expr(set, &ctx->nft->cache, nle);
 		list_add_tail(&stmt->list, &setelem_parse_ctx.stmt_list);
 	} else if (nftnl_set_elem_is_set(nlse, NFTNL_SET_ELEM_EXPRESSIONS)) {
 		nftnl_set_elem_expr_foreach(nlse, set_elem_parse_expressions,
@@ -1575,7 +1576,7 @@ int netlink_delinearize_setelem(struct nftnl_set_elem *nlse,
 static int list_setelem_cb(struct nftnl_set_elem *nlse, void *arg)
 {
 	struct netlink_ctx *ctx = arg;
-	return netlink_delinearize_setelem(nlse, ctx->set, &ctx->nft->cache);
+	return netlink_delinearize_setelem(ctx, nlse, ctx->set);
 }
 
 static int list_setelem_debug_cb(struct nftnl_set_elem *nlse, void *arg)
-- 
2.49.0

[nft PATCH 3/4] netlink: Keep going after set element parsing failures

[Phil Sutter]

Print an error message and try to deserialize the remaining elements
instead of calling BUG().

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/netlink.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/netlink.c b/src/netlink.c
index 1222919458bae..3221d9f8ffc93 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -1475,7 +1475,9 @@ int netlink_delinearize_setelem(struct netlink_ctx *ctx,
 		key->byteorder = set->key->byteorder;
 		key->len = set->key->len;
 	} else {
-		BUG("Unexpected set element with no key\n");
+		netlink_io_error(ctx, NULL,
+			         "Unexpected set element with no key\n");
+		return 0;
 	}
 
 	expr = set_elem_expr_alloc(&netlink_location, key);
-- 
2.49.0

Re: [nft PATCH 3/4] netlink: Keep going after set element parsing failures

[Pablo Neira Ayuso]

On Wed, May 21, 2025 at 03:12:41PM +0200, Phil Sutter wrote:
> Print an error message and try to deserialize the remaining elements
> instead of calling BUG().
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/netlink.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/netlink.c b/src/netlink.c
> index 1222919458bae..3221d9f8ffc93 100644
> --- a/src/netlink.c
> +++ b/src/netlink.c
> @@ -1475,7 +1475,9 @@ int netlink_delinearize_setelem(struct netlink_ctx *ctx,
>  		key->byteorder = set->key->byteorder;
>  		key->len = set->key->len;
>  	} else {
> -		BUG("Unexpected set element with no key\n");
> +		netlink_io_error(ctx, NULL,
> +			         "Unexpected set element with no key\n");
> +		return 0;

If set element has no key, then something is very wrong. There is
already one exception that is the catch-all element (which has no
key).

This is enqueuing an error record, but 0 is returned, I am not sure if
this is ever going to be printed.

I am not sure this patch works.

>  	}
>  
>  	expr = set_elem_expr_alloc(&netlink_location, key);
> -- 
> 2.49.0
> 

Re: [nft PATCH 3/4] netlink: Keep going after set element parsing failures

[Phil Sutter]

On Wed, May 21, 2025 at 04:17:49PM +0200, Pablo Neira Ayuso wrote:
> On Wed, May 21, 2025 at 03:12:41PM +0200, Phil Sutter wrote:
> > Print an error message and try to deserialize the remaining elements
> > instead of calling BUG().
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  src/netlink.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/netlink.c b/src/netlink.c
> > index 1222919458bae..3221d9f8ffc93 100644
> > --- a/src/netlink.c
> > +++ b/src/netlink.c
> > @@ -1475,7 +1475,9 @@ int netlink_delinearize_setelem(struct netlink_ctx *ctx,
> >  		key->byteorder = set->key->byteorder;
> >  		key->len = set->key->len;
> >  	} else {
> > -		BUG("Unexpected set element with no key\n");
> > +		netlink_io_error(ctx, NULL,
> > +			         "Unexpected set element with no key\n");
> > +		return 0;
> 
> If set element has no key, then something is very wrong. There is
> already one exception that is the catch-all element (which has no
> key).

Yes, in these cases the ruleset parser fails and the output is very
likely broken or at least incomplete. This series merely aligns error
handling: Take netlink_parse_cmp() for instance: If NFTNL_EXPR_CMP_SREG
attribute is missing or bogus, netlink_error() is called and the
function returns (void). No error status propagation happens (which we
could change easily), but most importantly the parser continues to
deserialize as much as possible.

> This is enqueuing an error record, but 0 is returned, I am not sure if
> this is ever going to be printed.

It does: Forcing the code to enter that third branch, listing a set with three
elements looks like this:

% sudo ./src/nft list ruleset
table ip t {
	set s {
		type inet_service
	}
}
netlink: Error: Unexpected set element with no key

netlink: Error: Unexpected set element with no key

netlink: Error: Unexpected set element with no key

> I am not sure this patch works.

Well, that extra newline is indeed a bug. :)

Cheers, Phil

Re: [nft PATCH 3/4] netlink: Keep going after set element parsing failures

[Pablo Neira Ayuso]

On Wed, May 21, 2025 at 07:19:19PM +0200, Phil Sutter wrote:
> On Wed, May 21, 2025 at 04:17:49PM +0200, Pablo Neira Ayuso wrote:
> > On Wed, May 21, 2025 at 03:12:41PM +0200, Phil Sutter wrote:
> > > Print an error message and try to deserialize the remaining elements
> > > instead of calling BUG().
> > > 
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > >  src/netlink.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/netlink.c b/src/netlink.c
> > > index 1222919458bae..3221d9f8ffc93 100644
> > > --- a/src/netlink.c
> > > +++ b/src/netlink.c
> > > @@ -1475,7 +1475,9 @@ int netlink_delinearize_setelem(struct netlink_ctx *ctx,
> > >  		key->byteorder = set->key->byteorder;
> > >  		key->len = set->key->len;
> > >  	} else {
> > > -		BUG("Unexpected set element with no key\n");
> > > +		netlink_io_error(ctx, NULL,
> > > +			         "Unexpected set element with no key\n");
> > > +		return 0;
> > 
> > If set element has no key, then something is very wrong. There is
> > already one exception that is the catch-all element (which has no
> > key).
> 
> Yes, in these cases the ruleset parser fails and the output is very
> likely broken or at least incomplete. This series merely aligns error
> handling: Take netlink_parse_cmp() for instance: If NFTNL_EXPR_CMP_SREG
> attribute is missing or bogus, netlink_error() is called and the
> function returns (void). No error status propagation happens (which we
> could change easily), but most importantly the parser continues to
> deserialize as much as possible.
> 
> > This is enqueuing an error record, but 0 is returned, I am not sure if
> > this is ever going to be printed.
> 
> It does: Forcing the code to enter that third branch, listing a set with three
> elements looks like this:
> 
> % sudo ./src/nft list ruleset
> table ip t {
> 	set s {
> 		type inet_service
> 	}
> }
> netlink: Error: Unexpected set element with no key
> 
> netlink: Error: Unexpected set element with no key
> 
> netlink: Error: Unexpected set element with no key
> 
> > I am not sure this patch works.
> 
> Well, that extra newline is indeed a bug. :)

Go ahead push it out then.

[nft PATCH 4/4] cache: Tolerate object deserialization failures

[Phil Sutter]

If netlink_delinearize_obj() fails, it will print an error message. Skip
this object and keep going.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/cache.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/cache.c b/src/cache.c
index e89acdf55a9b0..3ac819cf68fb7 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -871,12 +871,11 @@ static int obj_cache_cb(struct nftnl_obj *nlo, void *arg)
 		return 0;
 
 	obj = netlink_delinearize_obj(ctx->nlctx, nlo);
-	if (!obj)
-		return -1;
-
-	obj_name = nftnl_obj_get_str(nlo, NFTNL_OBJ_NAME);
-	hash = djb_hash(obj_name) % NFT_CACHE_HSIZE;
-	cache_add(&obj->cache, &ctx->table->obj_cache, hash);
+	if (obj) {
+		obj_name = nftnl_obj_get_str(nlo, NFTNL_OBJ_NAME);
+		hash = djb_hash(obj_name) % NFT_CACHE_HSIZE;
+		cache_add(&obj->cache, &ctx->table->obj_cache, hash);
+	}
 
 	nftnl_obj_list_del(nlo);
 	nftnl_obj_free(nlo);
-- 
2.49.0

Re: [nft PATCH 0/4] Continue upon netlink deserialization failures

[Pablo Neira Ayuso]

On Wed, May 21, 2025 at 03:12:38PM +0200, Phil Sutter wrote:
> Faced with unexpected values or missing attributes, most of the netlink
> deserialization code would complain, drop the nftables object being
> constructed and carry on. Some error paths though instead call BUG() or
> assert(0) instead. This series eliminates at least the most prominent
> ones among those (patches 1 and 3).
> 
> Patch 4 prevents object deserialization from aborting upon the first one
> with unexpected data. If netlink_delinearize_obj() returns NULL, an
> error message has been emitted already so just return 0 to the foreach
> loop so it continues with the next object.
> 
> Patch 2 is just preparation work for patch 3.

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

Re: [nft PATCH 0/4] Continue upon netlink deserialization failures

[Phil Sutter]

On Thu, May 22, 2025 at 07:18:14PM +0200, Pablo Neira Ayuso wrote:
> On Wed, May 21, 2025 at 03:12:38PM +0200, Phil Sutter wrote:
> > Faced with unexpected values or missing attributes, most of the netlink
> > deserialization code would complain, drop the nftables object being
> > constructed and carry on. Some error paths though instead call BUG() or
> > assert(0) instead. This series eliminates at least the most prominent
> > ones among those (patches 1 and 3).
> > 
> > Patch 4 prevents object deserialization from aborting upon the first one
> > with unexpected data. If netlink_delinearize_obj() returns NULL, an
> > error message has been emitted already so just return 0 to the foreach
> > loop so it continues with the next object.
> > 
> > Patch 2 is just preparation work for patch 3.
> 
> Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>

Applied after dropping two explicit newline characters leftover from
BUG() to netlink_error() conversion.

Thanks, Phil