diff mbox

[nf-next] netfilter: nf_tables: add support for inverted login in nft_lookup

Message ID 146469443294.16092.10350972377628813816.stgit@nfdev2.cica.es
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

Arturo Borrero May 31, 2016, 11:33 a.m. UTC 
Introduce a new configuration option for this expression, which allows users
to invert the logic of set lookups.

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 include/uapi/linux/netfilter/nf_tables.h |    6 ++++++
 net/netfilter/nft_lookup.c               |   15 ++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)


--
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

Comments

Pablo Neira Ayuso May 31, 2016, 11:39 a.m. UTC | #1 
On Tue, May 31, 2016 at 01:33:53PM +0200, Arturo Borrero Gonzalez wrote:
> Introduce a new configuration option for this expression, which allows users
> to invert the logic of set lookups.
> 
> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
> ---
>  include/uapi/linux/netfilter/nf_tables.h |    6 ++++++
>  net/netfilter/nft_lookup.c               |   15 ++++++++++++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> 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..4a9ee78 100644
> --- a/net/netfilter/nft_lookup.c
> +++ b/net/netfilter/nft_lookup.c
> @@ -23,6 +23,7 @@ struct nft_lookup {
>  	enum nft_registers		sreg:8;
>  	enum nft_registers		dreg:8;
>  	struct nft_set_binding		binding;
> +	bool				invert;
>  };

pahole reports that there is a hole between dreg and binding where you
can scratch those 8 bytes for this new boolean:

struct nft_lookup {
        struct nft_set *           set;                  /*     0 8 */
        enum nft_registers         sreg:8;               /*     8:24 4 */
        enum nft_registers         dreg:8;               /*     8:16 4 */

        /* XXX 16 bits hole, try to pack */
        /* XXX 4 bytes hole, try to pack */

        struct nft_set_binding     binding;              /*    16 32 */

        /* XXX last struct has 4 bytes of padding */

        /* size: 48, cachelines: 1, members: 4 */
        /* sum members: 44, holes: 1, sum holes: 4 */
        /* bit holes: 1, sum bit holes: 16 bits */
        /* paddings: 1, sum paddings: 4 */
        /* last cacheline: 48 bytes */
}

So this should look like instead:

  	enum nft_registers		sreg:8;
  	enum nft_registers		dreg:8;
 +	bool				invert;
  	struct nft_set_binding		binding;
  };
--
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
Florian Westphal May 31, 2016, 2:44 p.m. UTC | #2 
Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote:
> -	if (set->ops->lookup(set, &regs->data[priv->sreg], &ext)) {
> +	if (set->ops->lookup(set, &regs->data[priv->sreg], &ext) ^
> +	    priv->invert) {
>  		if (set->flags & NFT_SET_MAP)
>  			nft_data_copy(&regs->data[priv->dreg],
>  				      nft_set_ext_data(ext), set->dlen);

Whats the plan for SET_MAP here?
You enter 'lookup found a result' branch here in case we did not find
anything and invert is set.

I think its better to use a

} else if (priv->invert) {
	return;
}

here.

> @@ -47,6 +49,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 +58,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 ||
> @@ -91,6 +95,12 @@ static int nft_lookup_init(const struct nft_ctx *ctx,
>  	} else if (set->flags & NFT_SET_MAP)
>  		return -EINVAL;
>  
> +	if (tb[NFTA_LOOKUP_FLAGS]) {
> +		flags = ntohl(nla_get_be32(tb[NFTA_LOOKUP_FLAGS]));
> +		if (flags & NFT_LOOKUP_F_INV)
> +			priv->invert = true;
> +	}
> +

I think we should EINVAL if NFT_LOOKUP_F_INV is given with dreg/map.
--
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
Arturo Borrero May 31, 2016, 3:50 p.m. UTC | #3 
On 31 May 2016 at 16:44, Florian Westphal <fw@strlen.de> wrote:
> Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote:
>> -     if (set->ops->lookup(set, &regs->data[priv->sreg], &ext)) {
>> +     if (set->ops->lookup(set, &regs->data[priv->sreg], &ext) ^
>> +         priv->invert) {
>>               if (set->flags & NFT_SET_MAP)
>>                       nft_data_copy(&regs->data[priv->dreg],
>>                                     nft_set_ext_data(ext), set->dlen);
>
> Whats the plan for SET_MAP here?
> You enter 'lookup found a result' branch here in case we did not find
> anything and invert is set.
>
> I think its better to use a
>
> } else if (priv->invert) {
>         return;
> }
>
> here.
>

Totally right, thanks.

>> @@ -47,6 +49,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 +58,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 ||
>> @@ -91,6 +95,12 @@ static int nft_lookup_init(const struct nft_ctx *ctx,
>>       } else if (set->flags & NFT_SET_MAP)
>>               return -EINVAL;
>>
>> +     if (tb[NFTA_LOOKUP_FLAGS]) {
>> +             flags = ntohl(nla_get_be32(tb[NFTA_LOOKUP_FLAGS]));
>> +             if (flags & NFT_LOOKUP_F_INV)
>> +                     priv->invert = true;
>> +     }
>> +
>
> I think we should EINVAL if NFT_LOOKUP_F_INV is given with dreg/map.

ok
Arturo Borrero May 31, 2016, 4:18 p.m. UTC | #4 
On 31 May 2016 at 17:50, Arturo Borrero Gonzalez
<arturo.borrero.glez@gmail.com> wrote:
> On 31 May 2016 at 16:44, Florian Westphal <fw@strlen.de> wrote:
>> Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote:
>>> -     if (set->ops->lookup(set, &regs->data[priv->sreg], &ext)) {
>>> +     if (set->ops->lookup(set, &regs->data[priv->sreg], &ext) ^
>>> +         priv->invert) {
>>>               if (set->flags & NFT_SET_MAP)
>>>                       nft_data_copy(&regs->data[priv->dreg],
>>>                                     nft_set_ext_data(ext), set->dlen);
>>
>> Whats the plan for SET_MAP here?
>> You enter 'lookup found a result' branch here in case we did not find
>> anything and invert is set.
>>
>> I think its better to use a
>>
>> } else if (priv->invert) {
>>         return;
>> }
>>

Not that easy, 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

The XOR approach in my original patch seems to over these cases, ie,
we enter the return branch only if lookup&invert are different.
I think the nft_data_copy() should not worry us if we prevent the
combination from happening in _init()
The only penalty seems the additional boolean check for NFT_SET_MAP
(which must always fail if priv->invert is true)

So, I'm updating the _init() function with the additional restrictions.

Am I missing something else?
Arturo Borrero May 31, 2016, 4:42 p.m. UTC | #5 
On 31 May 2016 at 18:18, Arturo Borrero Gonzalez
<arturo.borrero.glez@gmail.com> wrote:
> On 31 May 2016 at 17:50, Arturo Borrero Gonzalez
> <arturo.borrero.glez@gmail.com> wrote:
>> On 31 May 2016 at 16:44, Florian Westphal <fw@strlen.de> wrote:
>>> Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote:
>>>> -     if (set->ops->lookup(set, &regs->data[priv->sreg], &ext)) {
>>>> +     if (set->ops->lookup(set, &regs->data[priv->sreg], &ext) ^
>>>> +         priv->invert) {
>>>>               if (set->flags & NFT_SET_MAP)
>>>>                       nft_data_copy(&regs->data[priv->dreg],
>>>>                                     nft_set_ext_data(ext), set->dlen);
>>>
>>> Whats the plan for SET_MAP here?
>>> You enter 'lookup found a result' branch here in case we did not find
>>> anything and invert is set.
>>>
>>> I think its better to use a
>>>
>>> } else if (priv->invert) {
>>>         return;
>>> }
>>>
>
> Not that easy, 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
>
> The XOR approach in my original patch seems to over these cases, ie,
> we enter the return branch only if lookup&invert are different.
> I think the nft_data_copy() should not worry us if we prevent the
> combination from happening in _init()
> The only penalty seems the additional boolean check for NFT_SET_MAP
> (which must always fail if priv->invert is true)
>
> So, I'm updating the _init() function with the additional restrictions.
>
> Am I missing something else?

We could be also extra defensive a do something like this:

static void nft_lookup_eval(const struct nft_expr *expr,
                            struct nft_regs *regs,
                            const struct nft_pktinfo *pkt)
{
        const struct nft_lookup *priv = nft_expr_priv(expr);
        const struct nft_set *set = priv->set;
        const struct nft_set_ext *ext;
        bool found;

        found = set->ops->lookup(set, &regs->data[priv->sreg], &ext);

        if (!(found ^ priv->invert)) {
                regs->verdict.code = NFT_BREAK;
                return;
        }

        if (set->flags & NFT_SET_MAP && found && !priv->invert)
                nft_data_copy(&regs->data[priv->dreg],
                              nft_set_ext_data(ext), set->dlen);

}

( web version http://paste.debian.net/713014 )

But, once _init() has been updated to prevent the combination of flags
and dreg/map, I don't know if it worth these extra checks in _eval().

please let me know your thoughts
Florian Westphal June 1, 2016, 8:58 a.m. UTC | #6 
Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote:
> On 31 May 2016 at 17:50, Arturo Borrero Gonzalez
> <arturo.borrero.glez@gmail.com> wrote:
> > On 31 May 2016 at 16:44, Florian Westphal <fw@strlen.de> wrote:
> >> I think its better to use a
> >>
> >> } else if (priv->invert) {
> >>         return;
> >> }
> >>
> 
> Not that easy, 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

Right...

> The XOR approach in my original patch seems to over these cases, ie,
> we enter the return branch only if lookup&invert are different.
> I think the nft_data_copy() should not worry us if we prevent the
> combination from happening in _init()

Agree, thanks Arturo!
--
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
diff mbox

Patch

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..4a9ee78 100644
--- a/net/netfilter/nft_lookup.c
+++ b/net/netfilter/nft_lookup.c
@@ -23,6 +23,7 @@  struct nft_lookup {
 	enum nft_registers		sreg:8;
 	enum nft_registers		dreg:8;
 	struct nft_set_binding		binding;
+	bool				invert;
 };
 
 static void nft_lookup_eval(const struct nft_expr *expr,
@@ -33,7 +34,8 @@  static void nft_lookup_eval(const struct nft_expr *expr,
 	const struct nft_set *set = priv->set;
 	const struct nft_set_ext *ext;
 
-	if (set->ops->lookup(set, &regs->data[priv->sreg], &ext)) {
+	if (set->ops->lookup(set, &regs->data[priv->sreg], &ext) ^
+	    priv->invert) {
 		if (set->flags & NFT_SET_MAP)
 			nft_data_copy(&regs->data[priv->dreg],
 				      nft_set_ext_data(ext), set->dlen);
@@ -47,6 +49,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 +58,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 ||
@@ -91,6 +95,12 @@  static int nft_lookup_init(const struct nft_ctx *ctx,
 	} else if (set->flags & NFT_SET_MAP)
 		return -EINVAL;
 
+	if (tb[NFTA_LOOKUP_FLAGS]) {
+		flags = ntohl(nla_get_be32(tb[NFTA_LOOKUP_FLAGS]));
+		if (flags & NFT_LOOKUP_F_INV)
+			priv->invert = true;
+	}
+
 	priv->binding.flags = set->flags & NFT_SET_MAP;
 
 	err = nf_tables_bind_set(ctx, set, &priv->binding);
@@ -112,6 +122,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 +131,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: