diff mbox series

[nft,1/2] relational: Eliminate meta OPs

Message ID 20180315230320.26776-2-phil@nwl.cc
State Accepted
Delegated to: Pablo Neira
Headers show
Series relational: Eliminate meta OPs | expand

Commit Message

Phil Sutter March 15, 2018, 11:03 p.m. UTC 
With a bit of code reorganization, relational meta OPs OP_RANGE,
OP_FLAGCMP and OP_LOOKUP become unused and can be removed. The only meta
OP left is OP_IMPLICIT which is usually treated as alias to OP_EQ.
Though it needs to stay in place for one reason: When matching against a
bitmask (e.g. TCP flags or conntrack states), it has a different
meaning:

| nft --debug=netlink add rule ip t c tcp flags syn
| ip t c
|   [ meta load l4proto => reg 1 ]
|   [ cmp eq reg 1 0x00000006 ]
|   [ payload load 1b @ transport header + 13 => reg 1 ]
|   [ bitwise reg 1 = (reg=1 & 0x00000002 ) ^ 0x00000000 ]
|   [ cmp neq reg 1 0x00000000 ]

| nft --debug=netlink add rule ip t c tcp flags == syn
| ip t c
|   [ meta load l4proto => reg 1 ]
|   [ cmp eq reg 1 0x00000006 ]
|   [ payload load 1b @ transport header + 13 => reg 1 ]
|   [ cmp eq reg 1 0x00000002 ]

OP_IMPLICIT creates a match which just checks the given flag is present,
while OP_EQ creates a match which ensures the given flag and no other is
present.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/expression.h      |   6 ---
 src/evaluate.c            | 114 +++++++---------------------------------------
 src/expression.c          |   8 +---
 src/netlink_delinearize.c |  21 ++++-----
 src/netlink_linearize.c   |  20 +++++---
 src/parser_bison.y        |   2 +-
 src/rule.c                |  13 +++++-
 7 files changed, 53 insertions(+), 131 deletions(-)
diff mbox series

Patch

diff --git a/include/expression.h b/include/expression.h
index 29dd0346a9b6b..f0ba6fc112601 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -85,12 +85,6 @@  enum ops {
 	OP_GT,
 	OP_LTE,
 	OP_GTE,
-	/* Range comparison */
-	OP_RANGE,
-	/* Flag comparison */
-	OP_FLAGCMP,
-	/* Set lookup */
-	OP_LOOKUP,
 	__OP_MAX
 };
 #define OP_MAX		(__OP_MAX - 1)
diff --git a/src/evaluate.c b/src/evaluate.c
index a2c1c7283d6ad..cbe15fedcf2b3 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1565,28 +1565,6 @@  static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 		return -1;
 	right = rel->right;
 
-	if (rel->op == OP_IMPLICIT) {
-		switch (right->ops->type) {
-		case EXPR_RANGE:
-			rel->op = OP_RANGE;
-			break;
-		case EXPR_SET:
-		case EXPR_SET_REF:
-			rel->op = OP_LOOKUP;
-			break;
-		case EXPR_LIST:
-			rel->op = OP_FLAGCMP;
-			break;
-		default:
-			if (right->dtype->basetype != NULL &&
-			    right->dtype->basetype->type == TYPE_BITMASK)
-				rel->op = OP_FLAGCMP;
-			else
-				rel->op = OP_EQ;
-			break;
-		}
-	}
-
 	if (!expr_is_constant(right))
 		return expr_binary_error(ctx->msgs, right, rel,
 					 "Right hand side of relational "
@@ -1598,56 +1576,34 @@  static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 					 "constant value",
 					 expr_op_symbols[rel->op]);
 
+	if (!datatype_equal(left->dtype, right->dtype))
+		return expr_binary_error(ctx->msgs, right, left,
+					 "datatype mismatch, expected %s, "
+					 "expression has type %s",
+					 left->dtype->desc,
+					 right->dtype->desc);
+
 	switch (rel->op) {
-	case OP_LOOKUP:
-		/* A literal set expression implicitly declares the set */
-		if (right->ops->type == EXPR_SET)
-			right = rel->right =
-				implicit_set_declaration(ctx, "__set%d",
-							 left, right);
-		else if (!datatype_equal(left->dtype, right->dtype))
-			return expr_binary_error(ctx->msgs, right, left,
-						 "datatype mismatch, expected %s, "
-						 "set has type %s",
-						 left->dtype->desc,
-						 right->dtype->desc);
-
-		/* Data for range lookups needs to be in big endian order */
-		if (right->set->flags & NFT_SET_INTERVAL &&
-		    byteorder_conversion(ctx, &rel->left,
-					 BYTEORDER_BIG_ENDIAN) < 0)
-			return -1;
-		left = rel->left;
-		break;
 	case OP_EQ:
-		if (!datatype_equal(left->dtype, right->dtype))
-			return expr_binary_error(ctx->msgs, right, left,
-						 "datatype mismatch, expected %s, "
-						 "expression has type %s",
-						 left->dtype->desc,
-						 right->dtype->desc);
+	case OP_IMPLICIT:
 		/*
 		 * Update protocol context for payload and meta iiftype
 		 * equality expressions.
 		 */
-		relational_expr_pctx_update(&ctx->pctx, rel);
-
-		if (left->ops->type == EXPR_CONCAT)
-			return 0;
+		if (expr_is_singleton(right))
+			relational_expr_pctx_update(&ctx->pctx, rel);
 
 		/* fall through */
 	case OP_NEQ:
-	case OP_FLAGCMP:
-		if (!datatype_equal(left->dtype, right->dtype))
-			return expr_binary_error(ctx->msgs, right, left,
-						 "datatype mismatch, expected %s, "
-						 "expression has type %s",
-						 left->dtype->desc,
-						 right->dtype->desc);
-
 		switch (right->ops->type) {
 		case EXPR_RANGE:
-			goto range;
+			if (byteorder_conversion(ctx, &rel->left, BYTEORDER_BIG_ENDIAN) < 0)
+				return -1;
+			if (byteorder_conversion(ctx, &right->left, BYTEORDER_BIG_ENDIAN) < 0)
+				return -1;
+			if (byteorder_conversion(ctx, &right->right, BYTEORDER_BIG_ENDIAN) < 0)
+				return -1;
+			break;
 		case EXPR_PREFIX:
 			if (byteorder_conversion(ctx, &right->prefix, left->byteorder) < 0)
 				return -1;
@@ -1657,12 +1613,10 @@  static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 				return -1;
 			break;
 		case EXPR_SET:
-			assert(rel->op == OP_NEQ);
 			right = rel->right =
 				implicit_set_declaration(ctx, "__set%d", left, right);
 			/* fall through */
 		case EXPR_SET_REF:
-			assert(rel->op == OP_NEQ);
 			/* Data for range lookups needs to be in big endian order */
 			if (right->set->flags & NFT_SET_INTERVAL &&
 			    byteorder_conversion(ctx, &rel->left, BYTEORDER_BIG_ENDIAN) < 0)
@@ -1676,13 +1630,6 @@  static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 	case OP_GT:
 	case OP_LTE:
 	case OP_GTE:
-		if (!datatype_equal(left->dtype, right->dtype))
-			return expr_binary_error(ctx->msgs, right, left,
-						 "datatype mismatch, expected %s, "
-						 "expression has type %s",
-						 left->dtype->desc,
-						 right->dtype->desc);
-
 		switch (left->ops->type) {
 		case EXPR_CONCAT:
 			return expr_binary_error(ctx->msgs, left, rel,
@@ -1706,33 +1653,6 @@  static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 		if (byteorder_conversion(ctx, &rel->right, BYTEORDER_BIG_ENDIAN) < 0)
 			return -1;
 		break;
-	case OP_RANGE:
-		if (!datatype_equal(left->dtype, right->dtype))
-			return expr_binary_error(ctx->msgs, right, left,
-						 "datatype mismatch, expected %s, "
-						 "expression has type %s",
-						 left->dtype->desc,
-						 right->dtype->desc);
-
-range:
-		switch (left->ops->type) {
-		case EXPR_CONCAT:
-			return expr_binary_error(ctx->msgs, left, rel,
-					"Relational expression (%s) is undefined"
-				        "for %s expressions",
-					expr_op_symbols[rel->op],
-					left->ops->name);
-		default:
-			break;
-		}
-
-		if (byteorder_conversion(ctx, &rel->left, BYTEORDER_BIG_ENDIAN) < 0)
-			return -1;
-		if (byteorder_conversion(ctx, &right->left, BYTEORDER_BIG_ENDIAN) < 0)
-			return -1;
-		if (byteorder_conversion(ctx, &right->right, BYTEORDER_BIG_ENDIAN) < 0)
-			return -1;
-		break;
 	default:
 		BUG("invalid relational operation %u\n", rel->op);
 	}
diff --git a/src/expression.c b/src/expression.c
index d7f54ad713732..5f023d2ad88e7 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -496,8 +496,6 @@  const char *expr_op_symbols[] = {
 	[OP_GT]		= ">",
 	[OP_LTE]	= "<=",
 	[OP_GTE]	= ">=",
-	[OP_RANGE]	= "within range",
-	[OP_LOOKUP]	= NULL,
 };
 
 static void unary_expr_print(const struct expr *expr, struct output_ctx *octx)
@@ -562,10 +560,6 @@  static void binop_arg_print(const struct expr *op, const struct expr *arg,
 
 static bool must_print_eq_op(const struct expr *expr)
 {
-	if (expr->right->dtype->basetype != NULL &&
-	    expr->right->dtype->basetype->type == TYPE_BITMASK)
-		return true;
-
 	return expr->left->ops->type == EXPR_BINOP;
 }
 
@@ -645,7 +639,7 @@  void relational_expr_pctx_update(struct proto_ctx *ctx,
 	const struct expr *left = expr->left;
 
 	assert(expr->ops->type == EXPR_RELATIONAL);
-	assert(expr->op == OP_EQ);
+	assert(expr->op == OP_EQ || expr->op == OP_IMPLICIT);
 
 	if (left->ops->pctx_update &&
 	    (left->flags & EXPR_F_PROTOCOL))
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index d65aacf8b6168..6b5c4aaae108f 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -323,7 +323,7 @@  static void netlink_parse_lookup(struct netlink_parse_ctx *ctx,
 		if (dreg != NFT_REG_VERDICT)
 			return netlink_set_register(ctx, dreg, expr);
 	} else {
-		expr = relational_expr_alloc(loc, OP_LOOKUP, left, right);
+		expr = relational_expr_alloc(loc, OP_EQ, left, right);
 	}
 
 	if (nftnl_expr_is_set(nle, NFTNL_EXPR_LOOKUP_FLAGS)) {
@@ -1514,9 +1514,14 @@  static void ct_meta_common_postprocess(struct rule_pp_ctx *ctx,
 	const struct expr *left = expr->left;
 	struct expr *right = expr->right;
 
+	if (right->ops->type == EXPR_SET || right->ops->type == EXPR_SET_REF)
+		expr_set_type(right, left->dtype, left->byteorder);
+
 	switch (expr->op) {
 	case OP_EQ:
-		if (expr->right->ops->type == EXPR_RANGE)
+		if (expr->right->ops->type == EXPR_RANGE ||
+		    expr->right->ops->type == EXPR_SET ||
+		    expr->right->ops->type == EXPR_SET_REF)
 			break;
 
 		relational_expr_pctx_update(&ctx->pctx, expr);
@@ -1533,14 +1538,6 @@  static void ct_meta_common_postprocess(struct rule_pp_ctx *ctx,
 				payload_dependency_store(&ctx->pdctx, ctx->stmt, base);
 		}
 		break;
-	case OP_NEQ:
-		if (right->ops->type != EXPR_SET && right->ops->type != EXPR_SET_REF)
-			break;
-		/* fall through */
-	case OP_LOOKUP:
-		expr_set_type(right, left->dtype, left->byteorder);
-		break;
-
 	default:
 		break;
 	}
@@ -1719,13 +1716,13 @@  static void relational_binop_postprocess(struct rule_pp_ctx *ctx, struct expr *e
 		/* Flag comparison: data & flags != 0
 		 *
 		 * Split the flags into a list of flag values and convert the
-		 * op to OP_FLAGCMP.
+		 * op to OP_EQ.
 		 */
 		expr_free(value);
 
 		expr->left  = expr_get(binop->left);
 		expr->right = binop_tree_to_list(NULL, binop->right);
-		expr->op    = OP_FLAGCMP;
+		expr->op    = OP_EQ;
 
 		expr_free(binop);
 	} else if (binop->left->dtype->flags & DTYPE_F_PREFIX &&
diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 5edb2d3d6fe86..5836231d5f3cf 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -297,6 +297,7 @@  static enum nft_cmp_ops netlink_gen_cmp_op(enum ops op)
 {
 	switch (op) {
 	case OP_EQ:
+	case OP_IMPLICIT:
 		return NFT_CMP_EQ;
 	case OP_NEQ:
 		return NFT_CMP_NEQ;
@@ -316,6 +317,9 @@  static enum nft_cmp_ops netlink_gen_cmp_op(enum ops op)
 static void netlink_gen_range(struct netlink_linearize_ctx *ctx,
 			      const struct expr *expr,
 			      enum nft_registers dreg);
+static void netlink_gen_flagcmp(struct netlink_linearize_ctx *ctx,
+				const struct expr *expr,
+				enum nft_registers dreg);
 
 static struct expr *netlink_gen_prefix(struct netlink_linearize_ctx *ctx,
 				       const struct expr *expr,
@@ -362,6 +366,8 @@  static void netlink_gen_cmp(struct netlink_linearize_ctx *ctx,
 	case EXPR_SET:
 	case EXPR_SET_REF:
 		return netlink_gen_lookup(ctx, expr, dreg);
+	case EXPR_LIST:
+		return netlink_gen_flagcmp(ctx, expr, dreg);
 	case EXPR_PREFIX:
 		sreg = get_register(ctx, expr->left);
 		if (expr_basetype(expr->left)->type != TYPE_STRING) {
@@ -376,6 +382,11 @@  static void netlink_gen_cmp(struct netlink_linearize_ctx *ctx,
 		}
 		break;
 	default:
+		if (expr->op == OP_IMPLICIT &&
+		    expr->right->dtype->basetype != NULL &&
+		    expr->right->dtype->basetype->type == TYPE_BITMASK)
+			return netlink_gen_flagcmp(ctx, expr, dreg);
+
 		sreg = get_register(ctx, expr->left);
 		len = div_round_up(expr->right->len, BITS_PER_BYTE);
 		right = expr->right;
@@ -421,8 +432,8 @@  static void netlink_gen_range(struct netlink_linearize_ctx *ctx,
 			       nld.value, nld.len);
 		nftnl_rule_add_expr(ctx->nlr, nle);
 		break;
-	case OP_RANGE:
 	case OP_EQ:
+	case OP_IMPLICIT:
 		nle = alloc_nft_expr("cmp");
 		netlink_put_register(nle, NFTNL_EXPR_CMP_SREG, sreg);
 		nftnl_expr_set_u32(nle, NFTNL_EXPR_CMP_OP,
@@ -491,6 +502,7 @@  static void netlink_gen_relational(struct netlink_linearize_ctx *ctx,
 				   enum nft_registers dreg)
 {
 	switch (expr->op) {
+	case OP_IMPLICIT:
 	case OP_EQ:
 	case OP_NEQ:
 	case OP_LT:
@@ -498,12 +510,6 @@  static void netlink_gen_relational(struct netlink_linearize_ctx *ctx,
 	case OP_LTE:
 	case OP_GTE:
 		return netlink_gen_cmp(ctx, expr, dreg);
-	case OP_RANGE:
-		return netlink_gen_range(ctx, expr, dreg);
-	case OP_FLAGCMP:
-		return netlink_gen_flagcmp(ctx, expr, dreg);
-	case OP_LOOKUP:
-		return netlink_gen_lookup(ctx, expr, dreg);
 	default:
 		BUG("invalid relational operation %u\n", expr->op);
 	}
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 5f84d794079a7..1854a8700a048 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -3173,7 +3173,7 @@  relational_expr		:	expr	/* implicit */	rhs_expr
 			}
 			|	expr	/* implicit */	list_rhs_expr
 			{
-				$$ = relational_expr_alloc(&@$, OP_FLAGCMP, $1, $2);
+				$$ = relational_expr_alloc(&@$, OP_IMPLICIT, $1, $2);
 			}
 			|	expr	relational_op	rhs_expr
 			{
diff --git a/src/rule.c b/src/rule.c
index c5bf65933ba88..4334efacbbdf7 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -2130,6 +2130,16 @@  static int payload_match_stmt_cmp(const void *p1, const void *p2)
 	return e1->left->payload.offset - e2->left->payload.offset;
 }
 
+static bool relational_ops_match(const struct expr *e1, const struct expr *e2)
+{
+	enum ops op1, op2;
+
+	op1 = e1->op == OP_IMPLICIT ? OP_EQ : e1->op;
+	op2 = e2->op == OP_IMPLICIT ? OP_EQ : e2->op;
+
+	return op1 == op2;
+}
+
 static void payload_do_merge(struct stmt *sa[], unsigned int n)
 {
 	struct expr *last, *this, *expr1, *expr2;
@@ -2144,7 +2154,7 @@  static void payload_do_merge(struct stmt *sa[], unsigned int n)
 		this = stmt->expr;
 
 		if (!payload_can_merge(last->left, this->left) ||
-		    last->op != this->op) {
+		    !relational_ops_match(last, this)) {
 			last = this;
 			j = i;
 			continue;
@@ -2227,6 +2237,7 @@  static void payload_try_merge(const struct rule *rule)
 			continue;
 		switch (stmt->expr->op) {
 		case OP_EQ:
+		case OP_IMPLICIT:
 		case OP_NEQ:
 			break;
 		default: