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 
> 
> This can prevent the nft utility from printing out the auto generated
> seed to the user, which is unnecessary and confusing.

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@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

[Laura García Liébana]

On Wed, Apr 12, 2017 at 10:43 PM, Florian Westphal  wrote:
> Liping Zhang  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;
>> >> +   boolautogen_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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@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

[Florian Westphal]

Liping Zhang  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;
> >> +   boolautogen_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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@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 :
> On Mon, Apr 3, 2017 at 10:34 AM, Liping Zhang  wrote:
>>
>> From: Liping Zhang 
>>
>> 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 
>> ---
>>  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;
>> +   boolautogen_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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@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

[Laura García Liébana]

On Mon, Apr 3, 2017 at 10:34 AM, Liping Zhang  wrote:
>
> From: Liping Zhang 
>
> 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 
> ---
>  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;
> +   boolautogen_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(>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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Liping Zhang]

From: Liping Zhang 

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 
---
 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;
+   boolautogen_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(>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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html