[PATCH nf] netfilter: nft_hash: do not dump the auto generated seed

[PATCH nf] netfilter: nft_hash: do not dump the auto generated seed

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

This can prevent the nft utility from printing out the auto generated
seed to the user, which is unnecessary and confusing.

Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 net/netfilter/nft_hash.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c
index eb2721a..c4dad12 100644
--- a/net/netfilter/nft_hash.c
+++ b/net/netfilter/nft_hash.c
@@ -21,6 +21,7 @@ struct nft_hash {
 	enum nft_registers      sreg:8;
 	enum nft_registers      dreg:8;
 	u8			len;
+	bool			autogen_seed:1;
 	u32			modulus;
 	u32			seed;
 	u32			offset;
@@ -82,10 +83,12 @@ static int nft_hash_init(const struct nft_ctx *ctx,
 	if (priv->offset + priv->modulus - 1 < priv->offset)
 		return -EOVERFLOW;
 
-	if (tb[NFTA_HASH_SEED])
+	if (tb[NFTA_HASH_SEED]) {
 		priv->seed = ntohl(nla_get_be32(tb[NFTA_HASH_SEED]));
-	else
+	} else {
+		priv->autogen_seed = true;
 		get_random_bytes(&priv->seed, sizeof(priv->seed));
+	}
 
 	return nft_validate_register_load(priv->sreg, len) &&
 	       nft_validate_register_store(ctx, priv->dreg, NULL,
@@ -105,7 +108,8 @@ static int nft_hash_dump(struct sk_buff *skb,
 		goto nla_put_failure;
 	if (nla_put_be32(skb, NFTA_HASH_MODULUS, htonl(priv->modulus)))
 		goto nla_put_failure;
-	if (nla_put_be32(skb, NFTA_HASH_SEED, htonl(priv->seed)))
+	if (!priv->autogen_seed &&
+	    nla_put_be32(skb, NFTA_HASH_SEED, htonl(priv->seed)))
 		goto nla_put_failure;
 	if (priv->offset != 0)
 		if (nla_put_be32(skb, NFTA_HASH_OFFSET, htonl(priv->offset)))
-- 
2.5.5

Re: [PATCH nf] netfilter: nft_hash: do not dump the auto generated seed

[Laura García Liébana]

On Mon, Apr 3, 2017 at 10:34 AM, Liping Zhang <zlpnobody@163.com> wrote:
>
> From: Liping Zhang <zlpnobody@gmail.com>
>
> This can prevent the nft utility from printing out the auto generated
> seed to the user, which is unnecessary and confusing.
>
> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
> ---
>  net/netfilter/nft_hash.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c
> index eb2721a..c4dad12 100644
> --- a/net/netfilter/nft_hash.c
> +++ b/net/netfilter/nft_hash.c
> @@ -21,6 +21,7 @@ struct nft_hash {
>         enum nft_registers      sreg:8;
>         enum nft_registers      dreg:8;
>         u8                      len;
> +       bool                    autogen_seed:1;

Hi Liping, I don't think that hiding the seed value would be useful, and
even adding this attribute doesn't worth it just to hide the seed.

>         u32                     modulus;
>         u32                     seed;
>         u32                     offset;
> @@ -82,10 +83,12 @@ static int nft_hash_init(const struct nft_ctx *ctx,
>         if (priv->offset + priv->modulus - 1 < priv->offset)
>                 return -EOVERFLOW;
>
> -       if (tb[NFTA_HASH_SEED])
> +       if (tb[NFTA_HASH_SEED]) {
>                 priv->seed = ntohl(nla_get_be32(tb[NFTA_HASH_SEED]));
> -       else
> +       } else {
> +               priv->autogen_seed = true;
>                 get_random_bytes(&priv->seed, sizeof(priv->seed));
> +       }
>
>         return nft_validate_register_load(priv->sreg, len) &&
>                nft_validate_register_store(ctx, priv->dreg, NULL,
> @@ -105,7 +108,8 @@ static int nft_hash_dump(struct sk_buff *skb,
>                 goto nla_put_failure;
>         if (nla_put_be32(skb, NFTA_HASH_MODULUS, htonl(priv->modulus)))
>                 goto nla_put_failure;
> -       if (nla_put_be32(skb, NFTA_HASH_SEED, htonl(priv->seed)))
> +       if (!priv->autogen_seed &&
> +           nla_put_be32(skb, NFTA_HASH_SEED, htonl(priv->seed)))
>                 goto nla_put_failure;
>         if (priv->offset != 0)
>                 if (nla_put_be32(skb, NFTA_HASH_OFFSET, htonl(priv->offset)))
> --
> 2.5.5
>
>
> --
> 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: [PATCH nf] netfilter: nft_hash: do not dump the auto generated seed

[Liping Zhang]

Hi Laura,

2017-04-08 5:19 GMT+08:00 Laura García Liébana <laura.garcia@zevenet.com>:
> On Mon, Apr 3, 2017 at 10:34 AM, Liping Zhang <zlpnobody@163.com> wrote:
>>
>> From: Liping Zhang <zlpnobody@gmail.com>
>>
>> This can prevent the nft utility from printing out the auto generated
>> seed to the user, which is unnecessary and confusing.
>>
>> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
>> ---
>>  net/netfilter/nft_hash.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c
>> index eb2721a..c4dad12 100644
>> --- a/net/netfilter/nft_hash.c
>> +++ b/net/netfilter/nft_hash.c
>> @@ -21,6 +21,7 @@ struct nft_hash {
>>         enum nft_registers      sreg:8;
>>         enum nft_registers      dreg:8;
>>         u8                      len;
>> +       bool                    autogen_seed:1;
>
> Hi Liping, I don't think that hiding the seed value would be useful, and
> even adding this attribute doesn't worth it just to hide the seed.
>

If we don't do this thing, if the user inputting the following nft rules:
  # nft add rule x y ct mark set jhash ip saddr mod 2

Then nft list ruleset will display something like this, where 0xd6ab633c
is very unpredictable, and the user doesn't care the seed at all:
  ct mark set jhash ip saddr mod 2 seed 0xd6ab633c

This will cause annoying complain when running "nft-test.py ip/hash.t".

But another problem is this, I remember that Pablo is implementing a new
delete rule syntax, something like this:
"nft del rule x y ct mark set jhash ip saddr mod 2"

The unpredictable seed will cause the above rule failed, since the seed
is not the same, so we cannot find a matched nft rule.

Re: [PATCH nf] netfilter: nft_hash: do not dump the auto generated seed

[Florian Westphal]

Liping Zhang <zlpnobody@gmail.com> wrote:
> >> +++ b/net/netfilter/nft_hash.c
> >> @@ -21,6 +21,7 @@ struct nft_hash {
> >>         enum nft_registers      sreg:8;
> >>         enum nft_registers      dreg:8;
> >>         u8                      len;
> >> +       bool                    autogen_seed:1;
> >
> > Hi Liping, I don't think that hiding the seed value would be useful, and
> > even adding this attribute doesn't worth it just to hide the seed.
> >
> 
> If we don't do this thing, if the user inputting the following nft rules:
>   # nft add rule x y ct mark set jhash ip saddr mod 2
> 
> Then nft list ruleset will display something like this, where 0xd6ab633c
> is very unpredictable, and the user doesn't care the seed at all:
>   ct mark set jhash ip saddr mod 2 seed 0xd6ab633c
> 
> This will cause annoying complain when running "nft-test.py ip/hash.t".
> 
> But another problem is this, I remember that Pablo is implementing a new
> delete rule syntax, something like this:
> "nft del rule x y ct mark set jhash ip saddr mod 2"
> 
> The unpredictable seed will cause the above rule failed, since the seed
> is not the same, so we cannot find a matched nft rule.

FWIW I agree with Liping, we should eat the extra bool and
supress seed dump.

I also think most users should not ever have to even know of seed arg
existence.

In fact is there a use case where it is needed?
We might want to ditch it completely and always just
generate it privately in kernel (symhash f.e. uses
a private seed) already.

Re: [PATCH nf] netfilter: nft_hash: do not dump the auto generated seed

[Laura García Liébana]

On Wed, Apr 12, 2017 at 10:43 PM, Florian Westphal <fw@strlen.de> wrote:
> Liping Zhang <zlpnobody@gmail.com> wrote:
>> >> +++ b/net/netfilter/nft_hash.c
>> >> @@ -21,6 +21,7 @@ struct nft_hash {
>> >>         enum nft_registers      sreg:8;
>> >>         enum nft_registers      dreg:8;
>> >>         u8                      len;
>> >> +       bool                    autogen_seed:1;
>> >
>> > Hi Liping, I don't think that hiding the seed value would be useful, and
>> > even adding this attribute doesn't worth it just to hide the seed.
>> >
>>
>> If we don't do this thing, if the user inputting the following nft rules:
>>   # nft add rule x y ct mark set jhash ip saddr mod 2
>>
>> Then nft list ruleset will display something like this, where 0xd6ab633c
>> is very unpredictable, and the user doesn't care the seed at all:
>>   ct mark set jhash ip saddr mod 2 seed 0xd6ab633c
>>
>> This will cause annoying complain when running "nft-test.py ip/hash.t".
>>
>> But another problem is this, I remember that Pablo is implementing a new
>> delete rule syntax, something like this:
>> "nft del rule x y ct mark set jhash ip saddr mod 2"
>>
>> The unpredictable seed will cause the above rule failed, since the seed
>> is not the same, so we cannot find a matched nft rule.
>
> FWIW I agree with Liping, we should eat the extra bool and
> supress seed dump.
>
> I also think most users should not ever have to even know of seed arg
> existence.
>
> In fact is there a use case where it is needed?
> We might want to ditch it completely and always just
> generate it privately in kernel (symhash f.e. uses
> a private seed) already.

In the case that is always generated in the kernel side, then we can
discard the seed parameter in nft and there is no need for an
additional bool.

Another option could be to relax the check of the seed attribute in
the tests and in the rules deletion.

Re: [PATCH nf] netfilter: nft_hash: do not dump the auto generated seed

[Pablo Neira Ayuso]

On Mon, Apr 03, 2017 at 04:34:38PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> This can prevent the nft utility from printing out the auto generated
> seed to the user, which is unnecessary and confusing.

Applied, thanks.