[nft PATCH 0/3] Boolean comparison and exthdr existence match support

[nft PATCH 0/3] Boolean comparison and exthdr existence match support

[Phil Sutter]

The following series adds two distinct features to nftables, though
since the second one depends on presence of the first one this is
submitted as a series.

Patch 1 adds support for a boolean variant of relational expression.
It's OP is strictly implicit and determined by RHS being a boolean
expression. It depends on a related kernel patch adding support for
NFT_CMP_BOOL to nft_cmp.c.

Patch 2 extends exthdr expression by a private flags field which will be
used in patch 3. It depends on a related patch for libnftnl to handle
the new field.

Patch 3 then adds support for checking extension header presence to
exthdr expression by making use of the previously introduced exthdr flag
NFT_EXTHDR_F_PRESENT. It's ideally used together with a boolean
relational expression for a syntax of e.g.:

| exthdr hbh exists

to match on hop-by-hop options presence or:

| exthdr frag missing

to match on packets without fragmentation header present.

Phil Sutter (3):
  Implement boolean comparison in relational expression
  exthdr: Add support for exthdr specific flags
  exthdr: Implement exthdr existence check

 include/expression.h                | 10 +++++++++
 include/exthdr.h                    |  4 ++++
 include/linux/netfilter/nf_tables.h |  1 +
 include/netlink.h                   |  2 ++
 src/evaluate.c                      | 13 ++++++++++++
 src/expression.c                    | 39 ++++++++++++++++++++++++++++++++++
 src/exthdr.c                        | 10 +++++++--
 src/netlink.c                       | 20 ++++++++++++++++++
 src/netlink_delinearize.c           | 12 +++++++++--
 src/netlink_linearize.c             |  4 ++++
 src/parser_bison.y                  | 42 +++++++++++++++++++++++++++++++++++++
 src/scanner.l                       |  7 +++++++
 12 files changed, 160 insertions(+), 4 deletions(-)

-- 
2.11.0

[nft PATCH 1/3] Implement boolean comparison in relational expression

[Phil Sutter]

This works by introducing OP_BOOL which allows to properly display the
boolean statement when listing rules. Apart from that, in kernel space
it is this way possible to optimize the comparison instead of having to
live with EQ/NEQ zero checks.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/expression.h                |  9 +++++++++
 include/linux/netfilter/nf_tables.h |  1 +
 include/netlink.h                   |  2 ++
 src/evaluate.c                      | 13 +++++++++++++
 src/expression.c                    | 38 +++++++++++++++++++++++++++++++++++++
 src/netlink.c                       | 20 +++++++++++++++++++
 src/netlink_delinearize.c           |  8 +++++++-
 src/netlink_linearize.c             |  3 +++
 src/parser_bison.y                  | 18 ++++++++++++++++++
 src/scanner.l                       |  5 +++++
 10 files changed, 116 insertions(+), 1 deletion(-)

diff --git a/include/expression.h b/include/expression.h
index ec90265b5f926..fa1587639621e 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -62,6 +62,7 @@ enum expr_types {
 	EXPR_HASH,
 	EXPR_RT,
 	EXPR_FIB,
+	EXPR_BOOLEAN,
 };
 
 enum ops {
@@ -89,6 +90,8 @@ enum ops {
 	OP_FLAGCMP,
 	/* Set lookup */
 	OP_LOOKUP,
+	/* boolean comparison */
+	OP_BOOL,
 	__OP_MAX
 };
 #define OP_MAX		(__OP_MAX - 1)
@@ -217,6 +220,10 @@ struct expr {
 	enum ops		op;
 	union {
 		struct {
+			/* EXPR_BOOLEAN */
+			bool			boolean;
+		};
+		struct {
 			/* EXPR_SYMBOL */
 			const struct scope	*scope;
 			const char		*identifier;
@@ -421,4 +428,6 @@ extern struct expr *set_elem_expr_alloc(const struct location *loc,
 extern void range_expr_value_low(mpz_t rop, const struct expr *expr);
 extern void range_expr_value_high(mpz_t rop, const struct expr *expr);
 
+extern struct expr *boolean_expr_alloc(const struct location *loc,
+				       bool val);
 #endif /* NFTABLES_EXPRESSION_H */
diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
index b00a05d1ee566..075894657aecd 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -543,6 +543,7 @@ enum nft_cmp_ops {
 	NFT_CMP_LTE,
 	NFT_CMP_GT,
 	NFT_CMP_GTE,
+	NFT_CMP_BOOL,
 };
 
 /**
diff --git a/include/netlink.h b/include/netlink.h
index 450aba571a979..6b0a72a846d59 100644
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -94,6 +94,8 @@ extern struct expr *netlink_alloc_value(const struct location *loc,
 extern struct expr *netlink_alloc_data(const struct location *loc,
 				       const struct nft_data_delinearize *nld,
 				       enum nft_registers dreg);
+extern struct expr *netlink_alloc_boolean(const struct location *loc,
+					  const struct nft_data_delinearize *nld);
 
 extern void netlink_linearize_rule(struct netlink_ctx *ctx,
 				   struct nftnl_rule *nlr,
diff --git a/src/evaluate.c b/src/evaluate.c
index bcbced1e3dfa6..6a65ec16c36f3 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1444,6 +1444,9 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 		case EXPR_LIST:
 			rel->op = OP_FLAGCMP;
 			break;
+		case EXPR_BOOLEAN:
+			rel->op = OP_BOOL;
+			break;
 		default:
 			if (right->dtype->basetype != NULL &&
 			    right->dtype->basetype->type == TYPE_BITMASK)
@@ -1605,6 +1608,8 @@ range:
 		if (byteorder_conversion(ctx, &right->right, BYTEORDER_BIG_ENDIAN) < 0)
 			return -1;
 		break;
+	case OP_BOOL:
+		break;
 	default:
 		BUG("invalid relational operation %u\n", rel->op);
 	}
@@ -1615,6 +1620,12 @@ range:
 	return 0;
 }
 
+static int expr_evaluate_boolean(struct eval_ctx *ctx, struct expr **expr)
+{
+	(*expr)->len = ctx->ectx.len;
+	return 0;
+}
+
 static int expr_evaluate(struct eval_ctx *ctx, struct expr **expr)
 {
 #ifdef DEBUG
@@ -1671,6 +1682,8 @@ static int expr_evaluate(struct eval_ctx *ctx, struct expr **expr)
 		return expr_evaluate_numgen(ctx, expr);
 	case EXPR_HASH:
 		return expr_evaluate_hash(ctx, expr);
+	case EXPR_BOOLEAN:
+		return expr_evaluate_boolean(ctx, expr);
 	default:
 		BUG("unknown expression type %s\n", (*expr)->ops->name);
 	}
diff --git a/src/expression.c b/src/expression.c
index 1567870c631b6..8842a7836fc07 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -456,6 +456,7 @@ const char *expr_op_symbols[] = {
 	[OP_GTE]	= ">=",
 	[OP_RANGE]	= "within range",
 	[OP_LOOKUP]	= NULL,
+	[OP_BOOL]	= NULL,
 };
 
 static void unary_expr_print(const struct expr *expr)
@@ -990,3 +991,40 @@ void range_expr_value_high(mpz_t rop, const struct expr *expr)
 		BUG("invalid range expression type %s\n", expr->ops->name);
 	}
 }
+
+static void boolean_expr_clone(struct expr *new, const struct expr *expr)
+{
+	new->boolean = expr->boolean;
+}
+
+static void boolean_expr_print(const struct expr *expr)
+{
+	switch (expr->dtype->type) {
+	case TYPE_FIB_ADDR:		/* fib expr */
+	case TYPE_IFINDEX:		/* fib expr */
+		printf(expr->boolean ? "exists" : "missing");
+		break;
+	default:
+		printf(expr->boolean ? "true" : "false");
+	}
+}
+
+static const struct expr_ops boolean_expr_ops = {
+	.type	= EXPR_BOOLEAN,
+	.name	= "boolean",
+	.clone	= boolean_expr_clone,
+	.print	= boolean_expr_print,
+};
+
+struct expr *boolean_expr_alloc(const struct location *loc,
+				bool val)
+{
+	struct expr *expr;
+
+	expr = expr_alloc(loc, &boolean_expr_ops,
+			  &invalid_type, BYTEORDER_INVALID, 0);
+	expr->boolean = val;
+	expr->flags = EXPR_F_CONSTANT | EXPR_F_SINGLETON;
+
+	return expr;
+}
diff --git a/src/netlink.c b/src/netlink.c
index 4135f2517a09b..7fed893d3af26 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -375,6 +375,14 @@ static void netlink_gen_verdict(const struct expr *expr,
 	}
 }
 
+static void netlink_gen_boolean(const struct expr *expr,
+				struct nft_data_linearize *data)
+{
+	data->len = div_round_up(expr->len, BITS_PER_BYTE);
+	memset(data->value, 0, sizeof(data->value));
+	data->value[0] = expr->boolean;
+}
+
 void netlink_gen_data(const struct expr *expr, struct nft_data_linearize *data)
 {
 	switch (expr->ops->type) {
@@ -384,6 +392,8 @@ void netlink_gen_data(const struct expr *expr, struct nft_data_linearize *data)
 		return netlink_gen_concat_data(expr, data);
 	case EXPR_VERDICT:
 		return netlink_gen_verdict(expr, data);
+	case EXPR_BOOLEAN:
+		return netlink_gen_boolean(expr, data);
 	default:
 		BUG("invalid data expression type %s\n", expr->ops->name);
 	}
@@ -426,6 +436,16 @@ struct expr *netlink_alloc_data(const struct location *loc,
 	}
 }
 
+struct expr *netlink_alloc_boolean(const struct location *loc,
+				   const struct nft_data_delinearize *nld)
+{
+	struct expr *expr;
+
+	expr = boolean_expr_alloc(loc, nld->value[0]);
+	expr->len = nld->len * BITS_PER_BYTE;
+	return expr;
+}
+
 int netlink_add_rule_batch(struct netlink_ctx *ctx,
 			   const struct handle *h,
 		           const struct rule *rule, uint32_t flags)
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 48968442d9bcc..06823de24d0e7 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -241,6 +241,8 @@ static enum ops netlink_parse_cmp_op(const struct nftnl_expr *nle)
 		return OP_GT;
 	case NFT_CMP_GTE:
 		return OP_GTE;
+	case NFT_CMP_BOOL:
+		return OP_BOOL;
 	default:
 		return OP_INVALID;
 	}
@@ -265,7 +267,10 @@ static void netlink_parse_cmp(struct netlink_parse_ctx *ctx,
 	op = netlink_parse_cmp_op(nle);
 
 	nld.value = nftnl_expr_get(nle, NFTNL_EXPR_CMP_DATA, &nld.len);
-	right = netlink_alloc_value(loc, &nld);
+	if (op == OP_BOOL)
+		right = netlink_alloc_boolean(loc, &nld);
+	else
+		right = netlink_alloc_value(loc, &nld);
 
 	if (left->len > right->len &&
 	    left->dtype != &string_type) {
@@ -1846,6 +1851,7 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
 	case EXPR_VERDICT:
 	case EXPR_NUMGEN:
 	case EXPR_FIB:
+	case EXPR_BOOLEAN:
 		break;
 	case EXPR_HASH:
 		expr_postprocess(ctx, &expr->hash.expr);
diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 5030135cd5d58..aff4f04a36db9 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -300,6 +300,8 @@ static enum nft_cmp_ops netlink_gen_cmp_op(enum ops op)
 		return NFT_CMP_LTE;
 	case OP_GTE:
 		return NFT_CMP_GTE;
+	case OP_BOOL:
+		return NFT_CMP_BOOL;
 	default:
 		BUG("invalid comparison operation %u\n", op);
 	}
@@ -489,6 +491,7 @@ static void netlink_gen_relational(struct netlink_linearize_ctx *ctx,
 	case OP_GT:
 	case OP_LTE:
 	case OP_GTE:
+	case OP_BOOL:
 		return netlink_gen_cmp(ctx, expr, dreg);
 	case OP_RANGE:
 		return netlink_gen_range(ctx, expr, dreg);
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 4749c9fa87308..3205cc407ffa8 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -428,6 +428,13 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 
 %token NOTRACK			"notrack"
 
+%token TRUE			"true"
+%token FALSE			"false"
+
+%type <val>			boolean_spec
+%type <expr>			boolean_expr
+%destructor { expr_free($$); }	boolean_expr
+
 %type <string>			identifier type_identifier string comment_spec
 %destructor { xfree($$); }	identifier type_identifier string comment_spec
 
@@ -2239,6 +2246,16 @@ integer_expr		:	NUM
 			}
 			;
 
+boolean_expr		:	boolean_spec
+			{
+				$$ = boolean_expr_alloc(&@$, $1);
+			}
+			;
+
+boolean_spec		:	TRUE	{ $$ = true; }
+			|	FALSE	{ $$ = false; }
+			;
+
 primary_expr		:	symbol_expr			{ $$ = $1; }
 			|	integer_expr			{ $$ = $1; }
 			|	payload_expr			{ $$ = $1; }
@@ -2616,6 +2633,7 @@ concat_rhs_expr		:	basic_rhs_expr
 
 primary_rhs_expr	:	symbol_expr		{ $$ = $1; }
 			|	integer_expr		{ $$ = $1; }
+			|	boolean_expr		{ $$ = $1; }
 			|	ETHER
 			{
 				$$ = symbol_expr_alloc(&@$, SYMBOL_VALUE,
diff --git a/src/scanner.l b/src/scanner.l
index d0d25ea946009..debc18fad37ef 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -482,6 +482,11 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "xml"			{ return XML; }
 "json"			{ return JSON; }
 
+"exists"		{ return TRUE; }
+"missing"		{ return FALSE; }
+"true"			{ return TRUE; }
+"false"			{ return FALSE; }
+
 {addrstring}		{
 				yylval->string = xstrdup(yytext);
 				return STRING;
-- 
2.11.0

[nft PATCH 2/3] exthdr: Add support for exthdr specific flags

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/expression.h      | 1 +
 src/exthdr.c              | 4 +++-
 src/netlink_delinearize.c | 4 +++-
 src/netlink_linearize.c   | 1 +
 4 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/expression.h b/include/expression.h
index fa1587639621e..67c046e23de1f 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -288,6 +288,7 @@ struct expr {
 			const struct exthdr_desc	*desc;
 			const struct proto_hdr_template	*tmpl;
 			unsigned int			offset;
+			unsigned int			flags;
 		} exthdr;
 		struct {
 			/* EXPR_META */
diff --git a/src/exthdr.c b/src/exthdr.c
index c641d4a398ad2..32bf3558115c5 100644
--- a/src/exthdr.c
+++ b/src/exthdr.c
@@ -30,7 +30,8 @@ static void exthdr_expr_print(const struct expr *expr)
 static bool exthdr_expr_cmp(const struct expr *e1, const struct expr *e2)
 {
 	return e1->exthdr.desc == e2->exthdr.desc &&
-	       e1->exthdr.tmpl == e2->exthdr.tmpl;
+	       e1->exthdr.tmpl == e2->exthdr.tmpl &&
+	       e1->exthdr.flags == e2->exthdr.flags;
 }
 
 static void exthdr_expr_clone(struct expr *new, const struct expr *expr)
@@ -38,6 +39,7 @@ static void exthdr_expr_clone(struct expr *new, const struct expr *expr)
 	new->exthdr.desc = expr->exthdr.desc;
 	new->exthdr.tmpl = expr->exthdr.tmpl;
 	new->exthdr.offset = expr->exthdr.offset;
+	new->exthdr.flags = expr->exthdr.flags;
 }
 
 static const struct expr_ops exthdr_expr_ops = {
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 06823de24d0e7..1d2a50abb5473 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -504,16 +504,18 @@ static void netlink_parse_exthdr(struct netlink_parse_ctx *ctx,
 				 const struct nftnl_expr *nle)
 {
 	enum nft_registers dreg;
-	uint32_t offset, len;
+	uint32_t offset, len, flags;
 	uint8_t type;
 	struct expr *expr;
 
 	type   = nftnl_expr_get_u8(nle, NFTNL_EXPR_EXTHDR_TYPE);
 	offset = nftnl_expr_get_u32(nle, NFTNL_EXPR_EXTHDR_OFFSET) * BITS_PER_BYTE;
 	len    = nftnl_expr_get_u32(nle, NFTNL_EXPR_EXTHDR_LEN) * BITS_PER_BYTE;
+	flags  = nftnl_expr_get_u32(nle, NFTNL_EXPR_EXTHDR_FLAGS);
 
 	expr = exthdr_expr_alloc(loc, NULL, 0);
 	exthdr_init_raw(expr, type, offset, len);
+	expr->exthdr.flags = flags;
 
 	dreg = netlink_parse_register(nle, NFTNL_EXPR_EXTHDR_DREG);
 	netlink_set_register(ctx, dreg, expr);
diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index aff4f04a36db9..f2560ece066a1 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -172,6 +172,7 @@ static void netlink_gen_exthdr(struct netlink_linearize_ctx *ctx,
 			   expr->exthdr.tmpl->offset / BITS_PER_BYTE);
 	nftnl_expr_set_u32(nle, NFTNL_EXPR_EXTHDR_LEN,
 			   div_round_up(expr->len, BITS_PER_BYTE));
+	nftnl_expr_set_u32(nle, NFTNL_EXPR_EXTHDR_FLAGS, expr->exthdr.flags);
 	nftnl_rule_add_expr(ctx->nlr, nle);
 }
 
-- 
2.11.0

[nft PATCH 3/3] exthdr: Implement exthdr existence check

[Phil Sutter]

This is meant to be used as LHS of a boolean relational expression, like
the following example matching on fragment header presence:

| exthdr frag exists

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/exthdr.h   |  4 ++++
 src/expression.c   |  1 +
 src/exthdr.c       |  6 +++++-
 src/parser_bison.y | 24 ++++++++++++++++++++++++
 src/scanner.l      |  2 ++
 5 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/include/exthdr.h b/include/exthdr.h
index d17841bc46159..f45cdf9b1886a 100644
--- a/include/exthdr.h
+++ b/include/exthdr.h
@@ -85,4 +85,8 @@ extern const struct exthdr_desc exthdr_frag;
 extern const struct exthdr_desc exthdr_dst;
 extern const struct exthdr_desc exthdr_mh;
 
+enum nft_exthdr_flags {
+	NFT_EXTHDR_F_PRESENT = (1 << 0),
+};
+
 #endif /* NFTABLES_EXTHDR_H */
diff --git a/src/expression.c b/src/expression.c
index 8842a7836fc07..9034420ae0db4 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -1002,6 +1002,7 @@ static void boolean_expr_print(const struct expr *expr)
 	switch (expr->dtype->type) {
 	case TYPE_FIB_ADDR:		/* fib expr */
 	case TYPE_IFINDEX:		/* fib expr */
+	case TYPE_INET_PROTOCOL:	/* exthdr exists expr */
 		printf(expr->boolean ? "exists" : "missing");
 		break;
 	default:
diff --git a/src/exthdr.c b/src/exthdr.c
index 32bf3558115c5..37d7700f328c8 100644
--- a/src/exthdr.c
+++ b/src/exthdr.c
@@ -24,7 +24,11 @@
 
 static void exthdr_expr_print(const struct expr *expr)
 {
-	printf("%s %s", expr->exthdr.desc->name, expr->exthdr.tmpl->token);
+	if (expr->exthdr.flags & NFT_EXTHDR_F_PRESENT)
+		printf("exthdr %s", expr->exthdr.desc->name);
+	else
+		printf("%s %s", expr->exthdr.desc->name,
+				expr->exthdr.tmpl->token);
 }
 
 static bool exthdr_expr_cmp(const struct expr *e1, const struct expr *e2)
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 3205cc407ffa8..e819affadbf05 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -139,6 +139,7 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 	const struct datatype	*datatype;
 	struct handle_spec	handle_spec;
 	struct position_spec	position_spec;
+	const struct exthdr_desc *exthdr_desc;
 }
 
 %token TOKEN_EOF 0		"end of file"
@@ -431,6 +432,8 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %token TRUE			"true"
 %token FALSE			"false"
 
+%token EXTHDR			"exthdr"
+
 %type <val>			boolean_spec
 %type <expr>			boolean_expr
 %destructor { expr_free($$); }	boolean_expr
@@ -633,6 +636,10 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %type <quota>			quota_config
 %destructor { xfree($$); }	quota_config
 
+%type <expr>			exthdr_exists_expr
+%destructor { expr_free($$); }	exthdr_exists_expr
+%type <exthdr_desc>		exthdr_spec
+
 %%
 
 input			:	/* empty */
@@ -2260,6 +2267,7 @@ primary_expr		:	symbol_expr			{ $$ = $1; }
 			|	integer_expr			{ $$ = $1; }
 			|	payload_expr			{ $$ = $1; }
 			|	exthdr_expr			{ $$ = $1; }
+			|	exthdr_exists_expr		{ $$ = $1; }
 			|	meta_expr			{ $$ = $1; }
 			|	rt_expr				{ $$ = $1; }
 			|	ct_expr				{ $$ = $1; }
@@ -3316,4 +3324,20 @@ mh_hdr_field		:	NEXTHDR		{ $$ = MHHDR_NEXTHDR; }
 			|	CHECKSUM	{ $$ = MHHDR_CHECKSUM; }
 			;
 
+exthdr_exists_expr	:	EXTHDR	exthdr_spec
+			{
+				/* Assume that NEXTHDR template is always
+				 * the fist one in list of templates.
+				 */
+				$$ = exthdr_expr_alloc(&@$, $2, 1);
+				$$->exthdr.flags = NFT_EXTHDR_F_PRESENT;
+			}
+			;
+
+exthdr_spec		:	HBH	{ $$ = &exthdr_hbh; }
+			|	RT	{ $$ = &exthdr_rt; }
+			|	FRAG	{ $$ = &exthdr_frag; }
+			|	DST	{ $$ = &exthdr_dst; }
+			|	MH	{ $$ = &exthdr_mh; }
+			;
 %%
diff --git a/src/scanner.l b/src/scanner.l
index debc18fad37ef..bd6edaac1224b 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -487,6 +487,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "true"			{ return TRUE; }
 "false"			{ return FALSE; }
 
+"exthdr"		{ return EXTHDR; }
+
 {addrstring}		{
 				yylval->string = xstrdup(yytext);
 				return STRING;
-- 
2.11.0

Re: [nft PATCH 0/3] Boolean comparison and exthdr existence match support

[Pablo Neira Ayuso]

Hi Phil,

On Tue, Jan 17, 2017 at 11:10:04PM +0100, Phil Sutter wrote:
> The following series adds two distinct features to nftables, though
> since the second one depends on presence of the first one this is
> submitted as a series.
> 
> Patch 1 adds support for a boolean variant of relational expression.
> It's OP is strictly implicit and determined by RHS being a boolean
> expression. It depends on a related kernel patch adding support for
> NFT_CMP_BOOL to nft_cmp.c.

If the problem is that we lack of context from the delinearize path,
then I would prefer if you scratch one bit from the fib flags to
indicate that we want a true (1)/false (0) return value. Just like we
plan to do with exthdr. This should require a small kernel patch for
nft_fib I think.

Thus, we can skip this ad hoc update for nft_cmp which seems to me
that it's only there to help us get the context that we lack from the
delinearize step.

Then, from the delinearize path, if this fib/exthdr flag is set, we
attach the corresponding datatype to the expression based on this new
flag.

Thanks.

Re: [nft PATCH 0/3] Boolean comparison and exthdr existence match support

[Phil Sutter]

Hi Pablo,

On Mon, Jan 23, 2017 at 01:57:47PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Jan 17, 2017 at 11:10:04PM +0100, Phil Sutter wrote:
> > The following series adds two distinct features to nftables, though
> > since the second one depends on presence of the first one this is
> > submitted as a series.
> > 
> > Patch 1 adds support for a boolean variant of relational expression.
> > It's OP is strictly implicit and determined by RHS being a boolean
> > expression. It depends on a related kernel patch adding support for
> > NFT_CMP_BOOL to nft_cmp.c.
> 
> If the problem is that we lack of context from the delinearize path,
> then I would prefer if you scratch one bit from the fib flags to
> indicate that we want a true (1)/false (0) return value. Just like we
> plan to do with exthdr. This should require a small kernel patch for
> nft_fib I think.
> 
> Thus, we can skip this ad hoc update for nft_cmp which seems to me
> that it's only there to help us get the context that we lack from the
> delinearize step.

This is not ad hoc updating nft_cmp but instead support for a new
operation. Did you maybe reply having the first approach from my RFC in
mind? Because I scratched that and went with the second one since it's
more complete.

> Then, from the delinearize path, if this fib/exthdr flag is set, we
> attach the corresponding datatype to the expression based on this new
> flag.

The point in NFT_CMP_BOOL is that it's LHS expression agnostic. Whatever
provides a value there can be checked for being eq/neq zero using the
boolean operation.

The context use in delinearize path is implicit (LHS defines RHS dtype)
and for convenience only: It merely allows printing different "flavors"
of boolean keywords depending on LHS and could easily be dropped.

Cheers, Phil

Re: [nft PATCH 0/3] Boolean comparison and exthdr existence match support

[Pablo Neira Ayuso]

On Mon, Feb 06, 2017 at 03:26:20PM +0100, Phil Sutter wrote:
[...]
> On Mon, Jan 23, 2017 at 01:57:47PM +0100, Pablo Neira Ayuso wrote:
> > On Tue, Jan 17, 2017 at 11:10:04PM +0100, Phil Sutter wrote:
> > > The following series adds two distinct features to nftables, though
> > > since the second one depends on presence of the first one this is
> > > submitted as a series.
> > > 
> > > Patch 1 adds support for a boolean variant of relational expression.
> > > It's OP is strictly implicit and determined by RHS being a boolean
> > > expression. It depends on a related kernel patch adding support for
> > > NFT_CMP_BOOL to nft_cmp.c.
> > 
> > If the problem is that we lack of context from the delinearize path,
> > then I would prefer if you scratch one bit from the fib flags to
> > indicate that we want a true (1)/false (0) return value. Just like we
> > plan to do with exthdr. This should require a small kernel patch for
> > nft_fib I think.
> > 
> > Thus, we can skip this ad hoc update for nft_cmp which seems to me
> > that it's only there to help us get the context that we lack from the
> > delinearize step.
> 
> This is not ad hoc updating nft_cmp but instead support for a new
> operation. Did you maybe reply having the first approach from my RFC in
> mind? Because I scratched that and went with the second one since it's
> more complete.

I think nft_cmp kernel doesn't need the specific boolean operation,
this is something that belongs to userspace. The existing kernel code
already allows us to match 0 and 1 which is sufficient for what we
need.

> > Then, from the delinearize path, if this fib/exthdr flag is set, we
> > attach the corresponding datatype to the expression based on this new
> > flag.
> 
> The point in NFT_CMP_BOOL is that it's LHS expression agnostic. Whatever
> provides a value there can be checked for being eq/neq zero using the
> boolean operation.
> 
> The context use in delinearize path is implicit (LHS defines RHS dtype)
> and for convenience only: It merely allows printing different "flavors"
> of boolean keywords depending on LHS and could easily be dropped.

I think we already have the context depending on the delinearize path
we follow, ie. netlink_delinearize_fib() may attach flavour A of
boolean, while netlink_delinearize_exthdr() attaches flavour B. BTW, I
would actually prefer to avoid flavouring at this stage, isn't it
found / not found enough for the usecase we have so far?

Thanks.

Re: [nft PATCH 0/3] Boolean comparison and exthdr existence match support

[Phil Sutter]

On Mon, Feb 06, 2017 at 06:16:01PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Feb 06, 2017 at 03:26:20PM +0100, Phil Sutter wrote:
> [...]
> > On Mon, Jan 23, 2017 at 01:57:47PM +0100, Pablo Neira Ayuso wrote:
> > > On Tue, Jan 17, 2017 at 11:10:04PM +0100, Phil Sutter wrote:
> > > > The following series adds two distinct features to nftables, though
> > > > since the second one depends on presence of the first one this is
> > > > submitted as a series.
> > > > 
> > > > Patch 1 adds support for a boolean variant of relational expression.
> > > > It's OP is strictly implicit and determined by RHS being a boolean
> > > > expression. It depends on a related kernel patch adding support for
> > > > NFT_CMP_BOOL to nft_cmp.c.
> > > 
> > > If the problem is that we lack of context from the delinearize path,
> > > then I would prefer if you scratch one bit from the fib flags to
> > > indicate that we want a true (1)/false (0) return value. Just like we
> > > plan to do with exthdr. This should require a small kernel patch for
> > > nft_fib I think.
> > > 
> > > Thus, we can skip this ad hoc update for nft_cmp which seems to me
> > > that it's only there to help us get the context that we lack from the
> > > delinearize step.
> > 
> > This is not ad hoc updating nft_cmp but instead support for a new
> > operation. Did you maybe reply having the first approach from my RFC in
> > mind? Because I scratched that and went with the second one since it's
> > more complete.
> 
> I think nft_cmp kernel doesn't need the specific boolean operation,
> this is something that belongs to userspace. The existing kernel code
> already allows us to match 0 and 1 which is sufficient for what we
> need.

Did you intentionally write "match [...] 1" or did you really mean
"match != 1"? I'm asking because the former would require for LHS to
normalize the result while the latter is more generic. But OTOH, if
suitable LHS expressions have to be touched anyway (as per your
suggestions below), this won't become an expression type independent
operation anymore anyway.

> > > Then, from the delinearize path, if this fib/exthdr flag is set, we
> > > attach the corresponding datatype to the expression based on this new
> > > flag.
> > 
> > The point in NFT_CMP_BOOL is that it's LHS expression agnostic. Whatever
> > provides a value there can be checked for being eq/neq zero using the
> > boolean operation.
> > 
> > The context use in delinearize path is implicit (LHS defines RHS dtype)
> > and for convenience only: It merely allows printing different "flavors"
> > of boolean keywords depending on LHS and could easily be dropped.
> 
> I think we already have the context depending on the delinearize path
> we follow, ie. netlink_delinearize_fib() may attach flavour A of
> boolean, while netlink_delinearize_exthdr() attaches flavour B. BTW, I
> would actually prefer to avoid flavouring at this stage, isn't it
> found / not found enough for the usecase we have so far?

Sure, for now just implementing exists/missing keywords is enough.

I don't really like what this is heading towards to be honest:

Assuming that I will use relational_expr to implement boolean operation,
expr_evaluate_relational() will have to do the necessary stitching to
set the "want boolean output" flag to LHS expression and it should
decide whether it is fit for that at all.

Another alternative would be to implement an independent statement, but
that feels like unnecessary code duplication.

In retrospective, I would have preferred if this discussion took place
after I sent those two alternative implementations as RFC. This way it
just feels like I wasted quite some effort for something that's not
appreciated anyway. You seem to have a much clearer idea of how this
boolean comparison should look like, and by giving more precise
instructions you could prevent quite some frustration trying to fulfill
them.

Cheers, Phil