[PATCH v2 nftables 0/3] un-break nftables on big-endian arches

[PATCH v2 nftables 0/3] un-break nftables on big-endian arches

[Florian Westphal]

nftables 0.9.1 fails on s390x.  Breakage includes:

- "jump foo" is rejected
- chain policies are treated as drop
- chain priority is always set to 0 (same bug though).

Also, last test case reveals problems with receive buffer sizes,
these have been fixed as well.

After these changes, all test cases work, except one.
The failure in that test case is caused by timeout rounding
errors due to CONFIG_HZ=100, I'll leave that for the time being
given HZ=100 is rare noways and the "fix" is to adjust the test case.

Florian Westphal (3):
      src: fix jumps on bigendian arches
      src: parser: fix parsing of chain priority and policy on bigendian
      src: mnl: retry when we hit -ENOBUFS

 datatype.c     |   27 ++++++++++++++++++---------
 mnl.c          |   12 ++++++++++--
 netlink.c      |   16 +++++++++++++---
 parser_bison.y |    6 ++++--
 4 files changed, 45 insertions(+), 16 deletions(-)

[PATCH nftables 1/3] src: fix jumps on bigendian arches

[Florian Westphal]

table bla {
  chain foo { }
  chain bar { jump foo }
 }
}

Fails to restore on big-endian platforms:
jump.nft:5:2-9: Error: Could not process rule: No such file or directory
 jump foo

nft passes a 0-length name to the kernel.

This is because when we export the value (the string), we provide
the size of the destination buffer.

In earlier versions, the parser allocated the name with the same
fixed size and all was fine.

After the fix, the export places the name in the wrong location
in the destination buffer.

This makes tests/shell/testcases/chains/0001jumps_0 work on s390x.

v2: convert one error check to a BUG(), it should not happen unless
    kernel abi is broken.

Fixes: 142350f154c78 ("src: invalid read when importing chain name")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/datatype.c | 27 ++++++++++++++++++---------
 src/netlink.c  | 16 +++++++++++++---
 2 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/src/datatype.c b/src/datatype.c
index 28f726f4e84c..c5a013463eb9 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -244,10 +244,25 @@ const struct datatype invalid_type = {
 	.print		= invalid_type_print,
 };
 
-static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
+static void verdict_jump_chain_print(const char *what, const struct expr *e,
+				     struct output_ctx *octx)
 {
 	char chain[NFT_CHAIN_MAXNAMELEN];
+	unsigned int len;
+
+	memset(chain, 0, sizeof(chain));
 
+	len = e->len / BITS_PER_BYTE;
+	if (len >= sizeof(chain))
+		BUG("verdict expression length %u is too large (%lu bits max)",
+		    e->len, (unsigned long)sizeof(chain) * BITS_PER_BYTE);
+
+	mpz_export_data(chain, e->value, BYTEORDER_HOST_ENDIAN, len);
+	nft_print(octx, "%s %s", what, chain);
+}
+
+static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
+{
 	switch (expr->verdict) {
 	case NFT_CONTINUE:
 		nft_print(octx, "continue");
@@ -257,10 +272,7 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
 		break;
 	case NFT_JUMP:
 		if (expr->chain->etype == EXPR_VALUE) {
-			mpz_export_data(chain, expr->chain->value,
-					BYTEORDER_HOST_ENDIAN,
-					NFT_CHAIN_MAXNAMELEN);
-			nft_print(octx, "jump %s", chain);
+			verdict_jump_chain_print("jump", expr->chain, octx);
 		} else {
 			nft_print(octx, "jump ");
 			expr_print(expr->chain, octx);
@@ -268,10 +280,7 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
 		break;
 	case NFT_GOTO:
 		if (expr->chain->etype == EXPR_VALUE) {
-			mpz_export_data(chain, expr->chain->value,
-					BYTEORDER_HOST_ENDIAN,
-					NFT_CHAIN_MAXNAMELEN);
-			nft_print(octx, "goto %s", chain);
+			verdict_jump_chain_print("goto", expr->chain, octx);
 		} else {
 			nft_print(octx, "goto ");
 			expr_print(expr->chain, octx);
diff --git a/src/netlink.c b/src/netlink.c
index aeeb12eaca93..f8e1120447d9 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -222,17 +222,27 @@ static void netlink_gen_verdict(const struct expr *expr,
 				struct nft_data_linearize *data)
 {
 	char chain[NFT_CHAIN_MAXNAMELEN];
+	unsigned int len;
 
 	data->verdict = expr->verdict;
 
 	switch (expr->verdict) {
 	case NFT_JUMP:
 	case NFT_GOTO:
+		len = expr->chain->len / BITS_PER_BYTE;
+
+		if (!len)
+			BUG("chain length is 0");
+
+		if (len > sizeof(chain))
+			BUG("chain is too large (%u, %u max)",
+			    len, (unsigned int)sizeof(chain));
+
+		memset(chain, 0, sizeof(chain));
+
 		mpz_export_data(chain, expr->chain->value,
-				BYTEORDER_HOST_ENDIAN,
-				NFT_CHAIN_MAXNAMELEN);
+				BYTEORDER_HOST_ENDIAN, len);
 		snprintf(data->chain, NFT_CHAIN_MAXNAMELEN, "%s", chain);
-		data->chain[NFT_CHAIN_MAXNAMELEN-1] = '\0';
 		break;
 	}
 }
-- 
2.21.0

[PATCH nftables 2/3] src: parser: fix parsing of chain priority and policy on bigendian

[Florian Westphal]

tests/shell/testcases/flowtable/0001flowtable_0
tests/shell/testcases/nft-f/0008split_tables_0
fail the 'dump compare' on s390x.
The priority (10) turns to 0, and accept turned to drop.

Problem is that '$1' is a 64bit value -- then we pass the address
and import 'int' -- we then get the upper all zero bits.

Add a 32bit interger type and use that.

v2: add uint32_t type to union, v1 used temporary value instead.

Fixes: 627c451b2351 ("src: allow variables in the chain priority specification")
Fixes: dba4a9b4b5fe ("src: allow variable in chain policy")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/parser_bison.y | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index 939b9a8db6d7..bff5e274c787 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -134,6 +134,7 @@ int nft_lex(void *, void *, void *);
 
 %union {
 	uint64_t		val;
+	uint32_t		val32;
 	const char *		string;
 
 	struct list_head	*list;
@@ -561,7 +562,8 @@ int nft_lex(void *, void *, void *);
 %destructor { handle_free(&$$); } table_spec tableid_spec chain_spec chainid_spec flowtable_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec
 %type <handle>			set_spec setid_spec set_identifier flowtable_identifier obj_spec objid_spec obj_identifier
 %destructor { handle_free(&$$); } set_spec setid_spec set_identifier obj_spec objid_spec obj_identifier
-%type <val>			family_spec family_spec_explicit chain_policy int_num
+%type <val>			family_spec family_spec_explicit
+%type <val32>			int_num	chain_policy
 %type <prio_spec>		extended_prio_spec prio_spec
 %type <string>			extended_prio_name
 %destructor { xfree($$); }	extended_prio_name
@@ -2025,7 +2027,7 @@ extended_prio_spec	:	int_num
 			}
 			;
 
-int_num		:	NUM			{ $$ = $1; }
+int_num			:	NUM			{ $$ = $1; }
 			|	DASH	NUM		{ $$ = -$2; }
 			;
 
-- 
2.21.0

[PATCH nftables 3/3] src: mnl: retry when we hit -ENOBUFS

[Florian Westphal]

tests/shell/testcases/transactions/0049huge_0

still fails with ENOBUFS error after endian fix done in
previous patch.  Its enough to increase the scale factor (4)
on s390x, but rather than continue with these "guess the proper
size" game, just increase the buffer size and retry up to 3 times.

This makes above test work on s390x.

So, implement what Pablo suggested in the earlier commit:
    We could also explore increasing the buffer and retry if
    mnl_nft_socket_sendmsg() hits ENOBUFS if we ever hit this problem again.

v2: call setsockopt unconditionally, then increase on error.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/mnl.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/mnl.c b/src/mnl.c
index 97a2e0765189..9c1f5356c9b9 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -311,6 +311,7 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list,
 	int ret, fd = mnl_socket_get_fd(nl), portid = mnl_socket_get_portid(nl);
 	uint32_t iov_len = nftnl_batch_iovec_len(ctx->batch);
 	char rcv_buf[MNL_SOCKET_BUFFER_SIZE];
+	unsigned int enobuf_restarts = 0;
 	size_t avg_msg_size, batch_size;
 	const struct sockaddr_nl snl = {
 		.nl_family = AF_NETLINK
@@ -320,6 +321,7 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list,
 		.tv_usec	= 0
 	};
 	struct iovec iov[iov_len];
+	unsigned int scale = 4;
 	struct msghdr msg = {};
 	fd_set readfds;
 
@@ -328,7 +330,8 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list,
 	batch_size = mnl_nft_batch_to_msg(ctx, &msg, &snl, iov, iov_len);
 	avg_msg_size = div_round_up(batch_size, num_cmds);
 
-	mnl_set_rcvbuffer(ctx->nft->nf_sock, num_cmds * avg_msg_size * 4);
+restart:
+	mnl_set_rcvbuffer(ctx->nft->nf_sock, num_cmds * avg_msg_size * scale);
 
 	ret = mnl_nft_socket_sendmsg(ctx, &msg);
 	if (ret == -1)
@@ -347,8 +350,13 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list,
 			break;
 
 		ret = mnl_socket_recvfrom(nl, rcv_buf, sizeof(rcv_buf));
-		if (ret == -1)
+		if (ret == -1) {
+			if (errno == ENOBUFS && enobuf_restarts++ < 3) {
+				scale *= 2;
+				goto restart;
+			}
 			return -1;
+		}
 
 		ret = mnl_cb_run(rcv_buf, ret, 0, portid, &netlink_echo_callback, ctx);
 		/* Continue on error, make sure we get all acknowledgments */
-- 
2.21.0

Re: [PATCH nftables 1/3] src: fix jumps on bigendian arches

[Pablo Neira Ayuso]

On Tue, Aug 13, 2019 at 10:12:44PM +0200, Florian Westphal wrote:
> table bla {
>   chain foo { }
>   chain bar { jump foo }
>  }
> }
> 
> Fails to restore on big-endian platforms:
> jump.nft:5:2-9: Error: Could not process rule: No such file or directory
>  jump foo
> 
> nft passes a 0-length name to the kernel.
> 
> This is because when we export the value (the string), we provide
> the size of the destination buffer.
> 
> In earlier versions, the parser allocated the name with the same
> fixed size and all was fine.
> 
> After the fix, the export places the name in the wrong location
> in the destination buffer.
> 
> This makes tests/shell/testcases/chains/0001jumps_0 work on s390x.
> 
> v2: convert one error check to a BUG(), it should not happen unless
>     kernel abi is broken.
> 
> Fixes: 142350f154c78 ("src: invalid read when importing chain name")
> Signed-off-by: Florian Westphal <fw@strlen.de>

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

Re: [PATCH nftables 2/3] src: parser: fix parsing of chain priority and policy on bigendian

[Pablo Neira Ayuso]

On Tue, Aug 13, 2019 at 10:12:45PM +0200, Florian Westphal wrote:
> tests/shell/testcases/flowtable/0001flowtable_0
> tests/shell/testcases/nft-f/0008split_tables_0
> fail the 'dump compare' on s390x.
> The priority (10) turns to 0, and accept turned to drop.
> 
> Problem is that '$1' is a 64bit value -- then we pass the address
> and import 'int' -- we then get the upper all zero bits.
> 
> Add a 32bit interger type and use that.
> 
> v2: add uint32_t type to union, v1 used temporary value instead.
> 
> Fixes: 627c451b2351 ("src: allow variables in the chain priority specification")
> Fixes: dba4a9b4b5fe ("src: allow variable in chain policy")
> Signed-off-by: Florian Westphal <fw@strlen.de>

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

Re: [PATCH nftables 3/3] src: mnl: retry when we hit -ENOBUFS

[Pablo Neira Ayuso]

On Tue, Aug 13, 2019 at 10:12:46PM +0200, Florian Westphal wrote:
> tests/shell/testcases/transactions/0049huge_0
> 
> still fails with ENOBUFS error after endian fix done in
> previous patch.  Its enough to increase the scale factor (4)
> on s390x, but rather than continue with these "guess the proper
> size" game, just increase the buffer size and retry up to 3 times.
> 
> This makes above test work on s390x.
> 
> So, implement what Pablo suggested in the earlier commit:
>     We could also explore increasing the buffer and retry if
>     mnl_nft_socket_sendmsg() hits ENOBUFS if we ever hit this problem again.
> 
> v2: call setsockopt unconditionally, then increase on error.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

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

Re: [PATCH nftables 3/3] src: mnl: retry when we hit -ENOBUFS

[Eric Garver]

Hi Florian,

On Tue, Aug 13, 2019 at 10:12:46PM +0200, Florian Westphal wrote:
> tests/shell/testcases/transactions/0049huge_0
> 
> still fails with ENOBUFS error after endian fix done in
> previous patch.  Its enough to increase the scale factor (4)
> on s390x, but rather than continue with these "guess the proper
> size" game, just increase the buffer size and retry up to 3 times.
> 
> This makes above test work on s390x.
> 
> So, implement what Pablo suggested in the earlier commit:
>     We could also explore increasing the buffer and retry if
>     mnl_nft_socket_sendmsg() hits ENOBUFS if we ever hit this problem again.
> 
> v2: call setsockopt unconditionally, then increase on error.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  src/mnl.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mnl.c b/src/mnl.c
> index 97a2e0765189..9c1f5356c9b9 100644
> --- a/src/mnl.c
> +++ b/src/mnl.c
> @@ -311,6 +311,7 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list,
>  	int ret, fd = mnl_socket_get_fd(nl), portid = mnl_socket_get_portid(nl);
>  	uint32_t iov_len = nftnl_batch_iovec_len(ctx->batch);
>  	char rcv_buf[MNL_SOCKET_BUFFER_SIZE];
> +	unsigned int enobuf_restarts = 0;
>  	size_t avg_msg_size, batch_size;
>  	const struct sockaddr_nl snl = {
>  		.nl_family = AF_NETLINK
> @@ -320,6 +321,7 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list,
>  		.tv_usec	= 0
>  	};
>  	struct iovec iov[iov_len];
> +	unsigned int scale = 4;
>  	struct msghdr msg = {};
>  	fd_set readfds;
>  
> @@ -328,7 +330,8 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list,
>  	batch_size = mnl_nft_batch_to_msg(ctx, &msg, &snl, iov, iov_len);
>  	avg_msg_size = div_round_up(batch_size, num_cmds);
>  
> -	mnl_set_rcvbuffer(ctx->nft->nf_sock, num_cmds * avg_msg_size * 4);
> +restart:
> +	mnl_set_rcvbuffer(ctx->nft->nf_sock, num_cmds * avg_msg_size * scale);
>  
>  	ret = mnl_nft_socket_sendmsg(ctx, &msg);
>  	if (ret == -1)
> @@ -347,8 +350,13 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list,
>  			break;
>  
>  		ret = mnl_socket_recvfrom(nl, rcv_buf, sizeof(rcv_buf));
> -		if (ret == -1)
> +		if (ret == -1) {
> +			if (errno == ENOBUFS && enobuf_restarts++ < 3) {
> +				scale *= 2;
> +				goto restart;
> +			}

If this restart is triggered it causes rules to be duplicated. We send
the same batch again.

I'm hitting this on x86_64. Maybe we need find a better way to estimate
the rcvbuffer in the case of --echo. By the time we see ENOBUFS we're
already in a bad way - events have already be lost.

>  			return -1;
> +		}
>  
>  		ret = mnl_cb_run(rcv_buf, ret, 0, portid, &netlink_echo_callback, ctx);
>  		/* Continue on error, make sure we get all acknowledgments */
> -- 
> 2.21.0
> 

Re: [PATCH nftables 3/3] src: mnl: retry when we hit -ENOBUFS

[Florian Westphal]

Eric Garver <eric@garver.life> wrote:
> > +			if (errno == ENOBUFS && enobuf_restarts++ < 3) {
> > +				scale *= 2;
> > +				goto restart;
> > +			}
> 
> If this restart is triggered it causes rules to be duplicated. We send
> the same batch again.

Right :-(

> I'm hitting this on x86_64. Maybe we need find a better way to estimate
> the rcvbuffer in the case of --echo. By the time we see ENOBUFS we're
> already in a bad way - events have already be lost.

Thats correct.  Pablo, I don't see a way for dynamic probing, as Eric
explains the error occurs after the fact so we cannot recover.

I will send a best-effort fix in a bit.