relational: Eliminate meta OPs

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>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/include/expression.h b/include/expression.h
index 29dd0346a9b6b74886c78003dac34adffd2a3a97..f0ba6fc1126018e57a8cd23f9b243af1369b6519 100644 (file)
--- 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 b71b67b96fc65c8bcaf62cded6b622c3225e94f0..114c831f5379c7152692c10f0f07de823ede7d70 100644 (file)
--- 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 d7f54ad71373238c4b5b07573564741fbbbfc90d..5f023d2ad88e778acb39b505a353f2f829edc149 100644 (file)
--- 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 d225b3e4b43fefba7bd6d53dd80ff2b47037aae0..997fb53b94997a3cc9f2e88b83644a4b9c007e97 100644 (file)
--- 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)) {
@@ -1524,9 +1524,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);
@@ -1543,14 +1548,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;
        }
@@ -1729,13 +1726,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 be1c750cf452640a8ce5c2b2d09f1235e5bb7020..dce8f5ce24d05df974d220f74aa8912a0136bead 100644 (file)
--- 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 6fba7e59555ca07242b56405688dc1e157bd54be..bdf2fb491736a7fed5288f8946942105330fc93d 100644 (file)
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -3185,7 +3185,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 c5bf65933ba88c127b291838be41a200517ae3df..4334efacbbdf7034ab877889b8203c1213a4d658 100644 (file)
--- 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: