[PATCH nft 0/3] typeof fixes

[PATCH nft 0/3] typeof fixes

[Florian Westphal]

First patch removes unused code fropm ipopt.
Second patch adds missing udata support for tcp/ip options and sctp
chunks.
Third patch fixes a crash in nft describe.

 include/ipopt.h                                    |  2 +-
 src/expression.c                                   |  8 ++--
 src/exthdr.c                                       | 47 ++++++++++++++++++----
 src/ipopt.c                                        | 26 +++---------
 src/parser_bison.y                                 |  8 +++-
 src/parser_json.c                                  |  4 +-
 tests/shell/testcases/parsing/describe             |  7 ++++
 tests/shell/testcases/sets/dumps/typeof_sets_0.nft | 27 +++++++++++++
 tests/shell/testcases/sets/typeof_sets_0           | 27 +++++++++++++
 9 files changed, 119 insertions(+), 37 deletions(-)

[PATCH nft 1/3] ipopt: drop unused 'ptr' argument

[Florian Westphal]

Its always 0, so remove it.
Looks like this was intended to support variable options that have
array-like members, but so far this isn't implemented, better remove
dead code and implement it properly when such support is needed.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/ipopt.h    |  2 +-
 src/ipopt.c        | 23 ++---------------------
 src/parser_bison.y |  4 ++--
 src/parser_json.c  |  4 ++--
 4 files changed, 7 insertions(+), 26 deletions(-)

diff --git a/include/ipopt.h b/include/ipopt.h
index d8d48066ae50..03420dc6221d 100644
--- a/include/ipopt.h
+++ b/include/ipopt.h
@@ -6,7 +6,7 @@
 #include <statement.h>
 
 extern struct expr *ipopt_expr_alloc(const struct location *loc,
-				      uint8_t type, uint8_t field, uint8_t ptr);
+				      uint8_t type, uint8_t field);
 
 extern void ipopt_init_raw(struct expr *expr, uint8_t type,
 			    unsigned int offset, unsigned int len,
diff --git a/src/ipopt.c b/src/ipopt.c
index 5f9f908c0b34..42ea41cd705b 100644
--- a/src/ipopt.c
+++ b/src/ipopt.c
@@ -66,27 +66,8 @@ const struct exthdr_desc *ipopt_protocols[UINT8_MAX] = {
 	[IPOPT_RA]		= &ipopt_ra,
 };
 
-static unsigned int calc_offset(const struct exthdr_desc *desc,
-				const struct proto_hdr_template *tmpl,
-				unsigned int arg)
-{
-	if (!desc || tmpl == &ipopt_unknown_template)
-		return 0;
-
-	switch (desc->type) {
-	case IPOPT_RR:
-	case IPOPT_LSRR:
-	case IPOPT_SSRR:
-		if (tmpl == &desc->templates[IPOPT_FIELD_ADDR_0])
-			return (tmpl->offset < 24) ? 0 : arg;
-		return 0;
-	default:
-		return 0;
-	}
-}
-
 struct expr *ipopt_expr_alloc(const struct location *loc, uint8_t type,
-			       uint8_t field, uint8_t ptr)
+			       uint8_t field)
 {
 	const struct proto_hdr_template *tmpl;
 	const struct exthdr_desc *desc;
@@ -102,7 +83,7 @@ struct expr *ipopt_expr_alloc(const struct location *loc, uint8_t type,
 	expr->exthdr.desc   = desc;
 	expr->exthdr.tmpl   = tmpl;
 	expr->exthdr.op     = NFT_EXTHDR_OP_IPV4;
-	expr->exthdr.offset = tmpl->offset + calc_offset(desc, tmpl, ptr);
+	expr->exthdr.offset = tmpl->offset;
 	expr->exthdr.raw_type = desc->type;
 
 	return expr;
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 355758e1befb..357850dececc 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -5331,11 +5331,11 @@ ip_hdr_expr		:	IP	ip_hdr_field	close_scope_ip
 			}
 			|	IP	OPTION	ip_option_type ip_option_field	close_scope_ip
 			{
-				$$ = ipopt_expr_alloc(&@$, $3, $4, 0);
+				$$ = ipopt_expr_alloc(&@$, $3, $4);
 			}
 			|	IP	OPTION	ip_option_type close_scope_ip
 			{
-				$$ = ipopt_expr_alloc(&@$, $3, IPOPT_FIELD_TYPE, 0);
+				$$ = ipopt_expr_alloc(&@$, $3, IPOPT_FIELD_TYPE);
 				$$->exthdr.flags = NFT_EXTHDR_F_PRESENT;
 			}
 			;
diff --git a/src/parser_json.c b/src/parser_json.c
index 7a2d30ff665c..2fad308f7783 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -689,7 +689,7 @@ static struct expr *json_parse_ip_option_expr(struct json_ctx *ctx,
 
 	if (json_unpack(root, "{s:s}", "field", &field)) {
 		expr = ipopt_expr_alloc(int_loc, descval,
-					 IPOPT_FIELD_TYPE, 0);
+					 IPOPT_FIELD_TYPE);
 		expr->exthdr.flags = NFT_EXTHDR_F_PRESENT;
 
 		return expr;
@@ -698,7 +698,7 @@ static struct expr *json_parse_ip_option_expr(struct json_ctx *ctx,
 		json_error(ctx, "Unknown ip option field '%s'.", field);
 		return NULL;
 	}
-	return ipopt_expr_alloc(int_loc, descval, fieldval, 0);
+	return ipopt_expr_alloc(int_loc, descval, fieldval);
 }
 
 static int json_parse_sctp_chunk_field(const struct exthdr_desc *desc,
-- 
2.32.0

[PATCH nft 2/3] exthdr: support ip/tcp options and sctp chunks in typeof expressions

[Florian Westphal]

This did not store the 'op' member and listing always treated this as ipv6
extension header.

Add test cases for this.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/exthdr.c                                  | 47 ++++++++++++++++---
 .../testcases/sets/dumps/typeof_sets_0.nft    | 27 +++++++++++
 tests/shell/testcases/sets/typeof_sets_0      | 27 +++++++++++
 3 files changed, 94 insertions(+), 7 deletions(-)

diff --git a/src/exthdr.c b/src/exthdr.c
index 2357ab60648d..3e5f5cd8b73e 100644
--- a/src/exthdr.c
+++ b/src/exthdr.c
@@ -115,7 +115,8 @@ static void exthdr_expr_clone(struct expr *new, const struct expr *expr)
 
 #define NFTNL_UDATA_EXTHDR_DESC 0
 #define NFTNL_UDATA_EXTHDR_TYPE 1
-#define NFTNL_UDATA_EXTHDR_MAX 2
+#define NFTNL_UDATA_EXTHDR_OP	2
+#define NFTNL_UDATA_EXTHDR_MAX 3
 
 static int exthdr_parse_udata(const struct nftnl_udata *attr, void *data)
 {
@@ -126,6 +127,7 @@ static int exthdr_parse_udata(const struct nftnl_udata *attr, void *data)
 	switch (type) {
 	case NFTNL_UDATA_EXTHDR_DESC:
 	case NFTNL_UDATA_EXTHDR_TYPE:
+	case NFTNL_UDATA_EXTHDR_OP:
 		if (len != sizeof(uint32_t))
 			return -1;
 		break;
@@ -140,6 +142,7 @@ static int exthdr_parse_udata(const struct nftnl_udata *attr, void *data)
 static struct expr *exthdr_expr_parse_udata(const struct nftnl_udata *attr)
 {
 	const struct nftnl_udata *ud[NFTNL_UDATA_EXTHDR_MAX + 1] = {};
+	enum nft_exthdr_op op = NFT_EXTHDR_OP_IPV6;
 	const struct exthdr_desc *desc;
 	unsigned int type;
 	uint32_t desc_id;
@@ -154,14 +157,31 @@ static struct expr *exthdr_expr_parse_udata(const struct nftnl_udata *attr)
 	    !ud[NFTNL_UDATA_EXTHDR_TYPE])
 		return NULL;
 
-	desc_id = nftnl_udata_get_u32(ud[NFTNL_UDATA_EXTHDR_DESC]);
-	desc = exthdr_find_desc(desc_id);
-	if (!desc)
-		return NULL;
+	if (ud[NFTNL_UDATA_EXTHDR_OP])
+		op = nftnl_udata_get_u32(ud[NFTNL_UDATA_EXTHDR_OP]);
 
+	desc_id = nftnl_udata_get_u32(ud[NFTNL_UDATA_EXTHDR_DESC]);
 	type = nftnl_udata_get_u32(ud[NFTNL_UDATA_EXTHDR_TYPE]);
 
-	return exthdr_expr_alloc(&internal_location, desc, type);
+	switch (op) {
+	case NFT_EXTHDR_OP_IPV6:
+		desc = exthdr_find_desc(desc_id);
+
+		return exthdr_expr_alloc(&internal_location, desc, type);
+	case NFT_EXTHDR_OP_TCPOPT:
+		return tcpopt_expr_alloc(&internal_location,
+					 desc_id, type);
+	case NFT_EXTHDR_OP_IPV4:
+		return ipopt_expr_alloc(&internal_location,
+					 desc_id, type);
+	case NFT_EXTHDR_OP_SCTP:
+		return sctp_chunk_expr_alloc(&internal_location,
+					     desc_id, type);
+	case __NFT_EXTHDR_OP_MAX:
+		return NULL;
+	}
+
+	return NULL;
 }
 
 static unsigned int expr_exthdr_type(const struct exthdr_desc *desc,
@@ -176,9 +196,22 @@ static int exthdr_expr_build_udata(struct nftnl_udata_buf *udbuf,
 	const struct proto_hdr_template *tmpl = expr->exthdr.tmpl;
 	const struct exthdr_desc *desc = expr->exthdr.desc;
 	unsigned int type = expr_exthdr_type(desc, tmpl);
+	enum nft_exthdr_op op = expr->exthdr.op;
 
-	nftnl_udata_put_u32(udbuf, NFTNL_UDATA_EXTHDR_DESC, desc->id);
 	nftnl_udata_put_u32(udbuf, NFTNL_UDATA_EXTHDR_TYPE, type);
+	switch (op) {
+	case NFT_EXTHDR_OP_IPV6:
+		nftnl_udata_put_u32(udbuf, NFTNL_UDATA_EXTHDR_DESC, desc->id);
+		break;
+	case NFT_EXTHDR_OP_TCPOPT:
+	case NFT_EXTHDR_OP_IPV4:
+	case NFT_EXTHDR_OP_SCTP:
+		nftnl_udata_put_u32(udbuf, NFTNL_UDATA_EXTHDR_OP, op);
+		nftnl_udata_put_u32(udbuf, NFTNL_UDATA_EXTHDR_DESC, expr->exthdr.raw_type);
+		break;
+	default:
+		return -1;
+	}
 
 	return 0;
 }
diff --git a/tests/shell/testcases/sets/dumps/typeof_sets_0.nft b/tests/shell/testcases/sets/dumps/typeof_sets_0.nft
index 06d891e682b7..8f11b110552c 100644
--- a/tests/shell/testcases/sets/dumps/typeof_sets_0.nft
+++ b/tests/shell/testcases/sets/dumps/typeof_sets_0.nft
@@ -19,6 +19,21 @@ table inet t {
 		elements = { 1, 1024 }
 	}
 
+	set s5 {
+		typeof ip option ra value
+		elements = { 1, 1024 }
+	}
+
+	set s6 {
+		typeof tcp option maxseg size
+		elements = { 1, 1024 }
+	}
+
+	set s7 {
+		typeof sctp chunk init num-inbound-streams
+		elements = { 1, 4 }
+	}
+
 	chain c1 {
 		osf name @s1 accept
 	}
@@ -26,4 +41,16 @@ table inet t {
 	chain c2 {
 		vlan id @s2 accept
 	}
+
+	chain c5 {
+		ip option ra value @s5 accept
+	}
+
+	chain c6 {
+		tcp option maxseg size @s6 accept
+	}
+
+	chain c7 {
+		sctp chunk init num-inbound-streams @s7 accept
+	}
 }
diff --git a/tests/shell/testcases/sets/typeof_sets_0 b/tests/shell/testcases/sets/typeof_sets_0
index a6ff8ca772e2..1e99e2987733 100755
--- a/tests/shell/testcases/sets/typeof_sets_0
+++ b/tests/shell/testcases/sets/typeof_sets_0
@@ -25,6 +25,21 @@ EXPECTED="table inet t {
 		elements = { 1, 1024 }
 	}
 
+	set s5 {
+		typeof ip option ra value
+		elements = { 1, 1024 }
+	}
+
+	set s6 {
+		typeof tcp option maxseg size
+		elements = { 1, 1024 }
+	}
+
+	set s7 {
+		typeof sctp chunk init num-inbound-streams
+		elements = { 1, 4 }
+	}
+
 	chain c1 {
 		osf name @s1 accept
 	}
@@ -32,6 +47,18 @@ EXPECTED="table inet t {
 	chain c2 {
 		ether type vlan vlan id @s2 accept
 	}
+
+	chain c5 {
+		ip option ra value @s5 accept
+	}
+
+	chain c6 {
+		tcp option maxseg size @s6 accept
+	}
+
+	chain c7 {
+		sctp chunk init num-inbound-streams @s7 accept
+	}
 }"
 
 set -e
-- 
2.32.0

[PATCH nft 3/3] iptopt: fix crash with invalid field/type combo

[Florian Westphal]

% nft describe ip option rr value
segmentation fault

after this fix, this exits with 'Error: unknown ip option type/field'.

Problem is that 'rr' doesn't have a value template, so the template struct is
all-zeroes, so we crash when trying to use tmpl->dtype (its NULL).

Furthermore, expr_describe tries to print expr->identifier but expr is
exthdr, not symbol: ->identifier contains garbage.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/expression.c                       | 8 ++++----
 src/ipopt.c                            | 3 +++
 src/parser_bison.y                     | 4 ++++
 tests/shell/testcases/parsing/describe | 7 +++++++
 4 files changed, 18 insertions(+), 4 deletions(-)
 create mode 100755 tests/shell/testcases/parsing/describe

diff --git a/src/expression.c b/src/expression.c
index 4c0874fe9950..f1cca8845376 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -135,12 +135,12 @@ void expr_describe(const struct expr *expr, struct output_ctx *octx)
 		nft_print(octx, "datatype %s (%s)",
 			  dtype->name, dtype->desc);
 		len = dtype->size;
-	} else if (dtype != &invalid_type) {
+	} else {
 		nft_print(octx, "%s expression, datatype %s (%s)",
 			  expr_name(expr), dtype->name, dtype->desc);
-	} else {
-		nft_print(octx, "datatype %s is invalid\n", expr->identifier);
-		return;
+
+		if (dtype == &invalid_type)
+			return;
 	}
 
 	if (dtype->basetype != NULL) {
diff --git a/src/ipopt.c b/src/ipopt.c
index 42ea41cd705b..67e904ff3d88 100644
--- a/src/ipopt.c
+++ b/src/ipopt.c
@@ -78,6 +78,9 @@ struct expr *ipopt_expr_alloc(const struct location *loc, uint8_t type,
 	if (!tmpl)
 		return NULL;
 
+	if (!tmpl->len)
+		return NULL;
+
 	expr = expr_alloc(loc, EXPR_EXTHDR, tmpl->dtype,
 			  BYTEORDER_BIG_ENDIAN, tmpl->len);
 	expr->exthdr.desc   = desc;
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 357850dececc..16607bb79bdd 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -5332,6 +5332,10 @@ ip_hdr_expr		:	IP	ip_hdr_field	close_scope_ip
 			|	IP	OPTION	ip_option_type ip_option_field	close_scope_ip
 			{
 				$$ = ipopt_expr_alloc(&@$, $3, $4);
+				if (!$$) {
+					erec_queue(error(&@1, "unknown ip option type/field"), state->msgs);
+					YYERROR;
+				}
 			}
 			|	IP	OPTION	ip_option_type close_scope_ip
 			{
diff --git a/tests/shell/testcases/parsing/describe b/tests/shell/testcases/parsing/describe
new file mode 100755
index 000000000000..2ee072e820fd
--- /dev/null
+++ b/tests/shell/testcases/parsing/describe
@@ -0,0 +1,7 @@
+#!/bin/bash
+
+errmsg='Error: unknown ip option type/field'
+
+str=$($NFT describe ip option rr value 2>&1 | head -n 1)
+
+[ "$str" = "$errmsg" ] && exit 0
-- 
2.32.0

Re: [PATCH nft 0/3] typeof fixes

[Pablo Neira Ayuso]

On Fri, Dec 03, 2021 at 05:07:52PM +0100, Florian Westphal wrote:
> First patch removes unused code fropm ipopt.
> Second patch adds missing udata support for tcp/ip options and sctp
> chunks.
> Third patch fixes a crash in nft describe.

LGTM, thanks