[nf-next PATCH v4] netfilter: nf_tables: add support for inverted logic in nft_lookup

[nf-next PATCH v4] netfilter: nf_tables: add support for inverted logic in nft_lookup

[Arturo Borrero Gonzalez]

Introduce a new configuration option for this expression, which allows users
to invert the logic of set lookups.

In _init() we will now return EINVAL if NFT_LOOKUP_F_INV is anyway
related to a map lookup.

The code in the _eval() function has been untangled and updated to sopport the
XOR of options, as we should consider 4 cases:
 * lookup false, invert false -> NFT_BREAK
 * lookup false, invert true -> return w/o NFT_BREAK
 * lookup true, invert false -> return w/o NFT_BREAK
 * lookup true, invert true -> NFT_BREAK

I decided to be extra defensive and check again in the _eval() path for an
invalid combination of the invert flag, in order to avoid the potential
disaster of copying registers uncontrollably.

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
v2: better aling memory of struct nft_lookup, put 'bool invert' before bindings
v3: include flags coherence validation
v4: stronger validation in _init(), stronger validation in _eval(), also untangle
    code, and update patch description

 include/uapi/linux/netfilter/nf_tables.h |    6 +++++
 net/netfilter/nft_lookup.c               |   36 ++++++++++++++++++++++++++----
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 6a4dbe0..01751fa 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -546,6 +546,10 @@ enum nft_cmp_attributes {
 };
 #define NFTA_CMP_MAX		(__NFTA_CMP_MAX - 1)
 
+enum nft_lookup_flags {
+	NFT_LOOKUP_F_INV = (1 << 0),
+};
+
 /**
  * enum nft_lookup_attributes - nf_tables set lookup expression netlink attributes
  *
@@ -553,6 +557,7 @@ enum nft_cmp_attributes {
  * @NFTA_LOOKUP_SREG: source register of the data to look for (NLA_U32: nft_registers)
  * @NFTA_LOOKUP_DREG: destination register (NLA_U32: nft_registers)
  * @NFTA_LOOKUP_SET_ID: uniquely identifies a set in a transaction (NLA_U32)
+ * @NFTA_LOOKUP_FLAGS: flags (NLA_U32: enum nft_lookup_flags)
  */
 enum nft_lookup_attributes {
 	NFTA_LOOKUP_UNSPEC,
@@ -560,6 +565,7 @@ enum nft_lookup_attributes {
 	NFTA_LOOKUP_SREG,
 	NFTA_LOOKUP_DREG,
 	NFTA_LOOKUP_SET_ID,
+	NFTA_LOOKUP_FLAGS,
 	__NFTA_LOOKUP_MAX
 };
 #define NFTA_LOOKUP_MAX		(__NFTA_LOOKUP_MAX - 1)
diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c
index b3c31ef..e87e960 100644
--- a/net/netfilter/nft_lookup.c
+++ b/net/netfilter/nft_lookup.c
@@ -22,6 +22,7 @@ struct nft_lookup {
 	struct nft_set			*set;
 	enum nft_registers		sreg:8;
 	enum nft_registers		dreg:8;
+	bool				invert;
 	struct nft_set_binding		binding;
 };
 
@@ -32,14 +33,19 @@ static void nft_lookup_eval(const struct nft_expr *expr,
 	const struct nft_lookup *priv = nft_expr_priv(expr);
 	const struct nft_set *set = priv->set;
 	const struct nft_set_ext *ext;
+	bool found;
 
-	if (set->ops->lookup(set, &regs->data[priv->sreg], &ext)) {
-		if (set->flags & NFT_SET_MAP)
-			nft_data_copy(&regs->data[priv->dreg],
-				      nft_set_ext_data(ext), set->dlen);
+	found = set->ops->lookup(set, &regs->data[priv->sreg], &ext);
+
+	if (!(found ^ priv->invert)) {
+		regs->verdict.code = NFT_BREAK;
 		return;
 	}
-	regs->verdict.code = NFT_BREAK;
+
+	if (set->flags & NFT_SET_MAP && found && !priv->invert)
+		nft_data_copy(&regs->data[priv->dreg],
+			      nft_set_ext_data(ext), set->dlen);
+
 }
 
 static const struct nla_policy nft_lookup_policy[NFTA_LOOKUP_MAX + 1] = {
@@ -47,6 +53,7 @@ static const struct nla_policy nft_lookup_policy[NFTA_LOOKUP_MAX + 1] = {
 	[NFTA_LOOKUP_SET_ID]	= { .type = NLA_U32 },
 	[NFTA_LOOKUP_SREG]	= { .type = NLA_U32 },
 	[NFTA_LOOKUP_DREG]	= { .type = NLA_U32 },
+	[NFTA_LOOKUP_FLAGS]	= { .type = NLA_U32 },
 };
 
 static int nft_lookup_init(const struct nft_ctx *ctx,
@@ -55,6 +62,7 @@ static int nft_lookup_init(const struct nft_ctx *ctx,
 {
 	struct nft_lookup *priv = nft_expr_priv(expr);
 	struct nft_set *set;
+	u32 flags;
 	int err;
 
 	if (tb[NFTA_LOOKUP_SET] == NULL ||
@@ -79,7 +87,22 @@ static int nft_lookup_init(const struct nft_ctx *ctx,
 	if (err < 0)
 		return err;
 
+	if (tb[NFTA_LOOKUP_FLAGS]) {
+		flags = ntohl(nla_get_be32(tb[NFTA_LOOKUP_FLAGS]));
+
+		if (flags & ~NFT_LOOKUP_F_INV)
+			return -EINVAL;
+
+		if (flags & NFT_LOOKUP_F_INV) {
+			if (set->flags & NFT_SET_MAP)
+				return -EINVAL;
+			priv->invert = true;
+		}
+	}
+
 	if (tb[NFTA_LOOKUP_DREG] != NULL) {
+		if (priv->invert)
+			return -EINVAL;
 		if (!(set->flags & NFT_SET_MAP))
 			return -EINVAL;
 
@@ -112,6 +135,7 @@ static void nft_lookup_destroy(const struct nft_ctx *ctx,
 static int nft_lookup_dump(struct sk_buff *skb, const struct nft_expr *expr)
 {
 	const struct nft_lookup *priv = nft_expr_priv(expr);
+	u32 flags = priv->invert ? NFT_LOOKUP_F_INV : 0;
 
 	if (nla_put_string(skb, NFTA_LOOKUP_SET, priv->set->name))
 		goto nla_put_failure;
@@ -120,6 +144,8 @@ static int nft_lookup_dump(struct sk_buff *skb, const struct nft_expr *expr)
 	if (priv->set->flags & NFT_SET_MAP)
 		if (nft_dump_register(skb, NFTA_LOOKUP_DREG, priv->dreg))
 			goto nla_put_failure;
+	if (nla_put_be32(skb, NFTA_LOOKUP_FLAGS, htonl(flags)))
+		goto nla_put_failure;
 	return 0;
 
 nla_put_failure:

Re: [nf-next PATCH v4] netfilter: nf_tables: add support for inverted logic in nft_lookup

[Pablo Neira Ayuso]

On Wed, Jun 01, 2016 at 05:23:02PM +0200, Arturo Borrero Gonzalez wrote:
> Introduce a new configuration option for this expression, which allows users
> to invert the logic of set lookups.

Any plan to add the missing nftables bits? Let me know, I would like
to have all the pieces in places so we can also add automated tests
for this.

Thanks!

Re: [nf-next PATCH v4] netfilter: nf_tables: add support for inverted logic in nft_lookup

[Arturo Borrero Gonzalez]

On 22 June 2016 at 20:22, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Jun 01, 2016 at 05:23:02PM +0200, Arturo Borrero Gonzalez wrote:
>> Introduce a new configuration option for this expression, which allows users
>> to invert the logic of set lookups.
>
> Any plan to add the missing nftables bits? Let me know, I would like
> to have all the pieces in places so we can also add automated tests
> for this.

Eventually yes, but not in the short term.


-- 
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [nf-next PATCH v4] netfilter: nf_tables: add support for inverted logic in nft_lookup

[Arturo Borrero Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On 23 June 2016 at 09:32, Arturo Borrero Gonzalez
<arturo.borrero.glez@gmail.com> wrote:
> On 22 June 2016 at 20:22, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> On Wed, Jun 01, 2016 at 05:23:02PM +0200, Arturo Borrero Gonzalez wrote:
>>> Introduce a new configuration option for this expression, which allows users
>>> to invert the logic of set lookups.
>>
>> Any plan to add the missing nftables bits? Let me know, I would like
>> to have all the pieces in places so we can also add automated tests
>> for this.
>
> Eventually yes, but not in the short term.
>

BTW, if you would like to have it done now, I'm attaching the [half]
patch I developed back in May.

AFAIK, this patch implements the user-to-kernel path but the
kernel-to-user path was missing.

-- 
Arturo Borrero González

[-- Attachment #2: lookup-inversion.patch --]
[-- Type: text/x-patch, Size: 4665 bytes --]

evaluate: add support for set lookups inversion

From: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 include/expression.h      |    1 +
 src/evaluate.c            |   13 +++++++++++++
 src/expression.c          |    1 +
 src/netlink_delinearize.c |    9 +++++++++
 src/netlink_linearize.c   |    9 +++++++++
 5 files changed, 33 insertions(+)

diff --git a/include/expression.h b/include/expression.h
index 6e5e835..c6c6ef8 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -82,6 +82,7 @@ enum ops {
 	OP_FLAGCMP,
 	/* Set lookup */
 	OP_LOOKUP,
+	OP_LOOKUP_INV,
 	__OP_MAX
 };
 #define OP_MAX		(__OP_MAX - 1)
diff --git a/src/evaluate.c b/src/evaluate.c
index f24e5f3..e1faa79 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1350,6 +1350,17 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 		return -1;
 	right = rel->right;
 
+	if (rel->op == OP_NEQ) {
+		switch (right->ops->type) {
+		case EXPR_SET:
+		case EXPR_SET_REF:
+			rel->op = OP_LOOKUP_INV;
+			break;
+		default:
+			break;
+		}
+	}
+
 	if (rel->op == OP_IMPLICIT) {
 		switch (right->ops->type) {
 		case EXPR_RANGE:
@@ -1372,6 +1383,7 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 		}
 	}
 
+
 	if (!expr_is_constant(right))
 		return expr_binary_error(ctx->msgs, right, rel,
 					 "Right hand side of relational "
@@ -1385,6 +1397,7 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 
 	switch (rel->op) {
 	case OP_LOOKUP:
+	case OP_LOOKUP_INV:
 		switch (right->ops->type) {
 		case EXPR_SET:
 			/* A literal set expression implicitly declares
diff --git a/src/expression.c b/src/expression.c
index a10af5d..5d6d6b3 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -451,6 +451,7 @@ const char *expr_op_symbols[] = {
 	[OP_GTE]	= ">=",
 	[OP_RANGE]	= "within range",
 	[OP_LOOKUP]	= NULL,
+	[OP_LOOKUP_INV]	= "!=",
 };
 
 static void unary_expr_print(const struct expr *expr)
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 7735699..35bad01 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -259,6 +259,7 @@ static void netlink_parse_lookup(struct netlink_parse_ctx *ctx,
 	const char *name;
 	struct expr *expr, *left, *right;
 	struct set *set;
+	uint32_t flags;
 
 	name = nftnl_expr_get_str(nle, NFTNL_EXPR_LOOKUP_SET);
 	set  = set_lookup(ctx->table, name);
@@ -281,13 +282,20 @@ static void netlink_parse_lookup(struct netlink_parse_ctx *ctx,
 
 	right = set_ref_expr_alloc(loc, set);
 
+	flags = nftnl_expr_get_u32(nle, NFTNL_EXPR_LOOKUP_FLAGS);
+
 	if (nftnl_expr_is_set(nle, NFTNL_EXPR_LOOKUP_DREG)) {
 		dreg = netlink_parse_register(nle, NFTNL_EXPR_LOOKUP_DREG);
 		expr = map_expr_alloc(loc, left, right);
+		if (flags & NFT_LOOKUP_F_INV)
+			expr->op = OP_LOOKUP_INV;
+
 		if (dreg != NFT_REG_VERDICT)
 			return netlink_set_register(ctx, dreg, expr);
 	} else {
 		expr = relational_expr_alloc(loc, OP_LOOKUP, left, right);
+		if (flags & NFT_LOOKUP_F_INV)
+			expr->op = OP_LOOKUP_INV;
 	}
 
 	ctx->stmt = expr_stmt_alloc(loc, expr);
@@ -1168,6 +1176,7 @@ static void ct_meta_common_postprocess(const struct expr *expr)
 
 	switch (expr->op) {
 	case OP_LOOKUP:
+	case OP_LOOKUP_INV:
 		expr_set_type(right, left->dtype, left->byteorder);
 		if (right->dtype == &integer_type)
 			integer_type_postprocess(right);
diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 01a85d0..f6f608c 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -191,6 +191,10 @@ static void netlink_gen_map(struct netlink_linearize_ctx *ctx,
 	nftnl_expr_set_u32(nle, NFTNL_EXPR_LOOKUP_SET_ID,
 			   expr->mappings->set->handle.set_id);
 
+	if (expr->op == OP_LOOKUP_INV)
+		nftnl_expr_set_u32(nle, NFTNL_EXPR_LOOKUP_FLAGS,
+				   NFT_LOOKUP_F_INV);
+
 	if (dreg == NFT_REG_VERDICT)
 		release_register(ctx, expr->map);
 
@@ -217,6 +221,10 @@ static void netlink_gen_lookup(struct netlink_linearize_ctx *ctx,
 	nftnl_expr_set_u32(nle, NFTNL_EXPR_LOOKUP_SET_ID,
 			   expr->right->set->handle.set_id);
 
+	if (expr->op == OP_LOOKUP_INV)
+		nftnl_expr_set_u32(nle, NFTNL_EXPR_LOOKUP_FLAGS,
+				   NFT_LOOKUP_F_INV);
+
 	release_register(ctx, expr->left);
 	nftnl_rule_add_expr(ctx->nlr, nle);
 }
@@ -433,6 +441,7 @@ static void netlink_gen_relational(struct netlink_linearize_ctx *ctx,
 	case OP_FLAGCMP:
 		return netlink_gen_flagcmp(ctx, expr, dreg);
 	case OP_LOOKUP:
+	case OP_LOOKUP_INV:
 		return netlink_gen_lookup(ctx, expr, dreg);
 	default:
 		BUG("invalid relational operation %u\n", expr->op);

Re: [nf-next PATCH v4] netfilter: nf_tables: add support for inverted logic in nft_lookup

[Pablo Neira Ayuso]

On Wed, Jun 01, 2016 at 05:23:02PM +0200, Arturo Borrero Gonzalez wrote:
> @@ -32,14 +33,19 @@ static void nft_lookup_eval(const struct nft_expr *expr,
>  	const struct nft_lookup *priv = nft_expr_priv(expr);
>  	const struct nft_set *set = priv->set;
>  	const struct nft_set_ext *ext;
> +	bool found;
>  
> -	if (set->ops->lookup(set, &regs->data[priv->sreg], &ext)) {
> -		if (set->flags & NFT_SET_MAP)
> -			nft_data_copy(&regs->data[priv->dreg],
> -				      nft_set_ext_data(ext), set->dlen);
> +	found = set->ops->lookup(set, &regs->data[priv->sreg], &ext);
> +
> +	if (!(found ^ priv->invert)) {
> +		regs->verdict.code = NFT_BREAK;
>  		return;
>  	}
> -	regs->verdict.code = NFT_BREAK;
> +
> +	if (set->flags & NFT_SET_MAP && found && !priv->invert)

I think this is a bit defensive: if set->flags & NFT_SET_MAP evaluates
true, then we can assume priv->invert is always false. We can just add
a comment on top of this.

Note that, from the _init() path, we reject inversions with maps.

So probably something like:

        /* nft_lookup_init() already rejects maps with inverted lookups.
         * We assume that inversion is always false with maps.
         */
	found = set->ops->lookup(set, &regs->data[priv->sreg], &ext) ^
                priv->invert;
        if (found && set->flags & NFT_SET_MAP)
                nft_data_copy(...);

would be a bit more simple.

> +		nft_data_copy(&regs->data[priv->dreg],
> +			      nft_set_ext_data(ext), set->dlen);
> +
>  }
>  
>  static const struct nla_policy nft_lookup_policy[NFTA_LOOKUP_MAX + 1] = {