[nf] netfilter: nft_dynset: fix incorrect element expiration calculation

Hi Liping,

On søn, 2016-11-20 at 14:18 +0800, Liping Zhang wrote:
> After commit a8b1e36d0d1d ("netfilter: nft_dynset: fix element

> timeout

> for HZ != 1000"), priv->timeout was stored in jiffies, while

> set->timeout was stored in milliseconds. This is inconsistent and

> incorrect.

> 

> Firstly, we already call msecs_to_jiffies in nft_set_elem_init, so

> priv->timeout will be converted to jiffies twice.

> 

> Secondly, if the user did not specify the NFTA_DYNSET_TIMEOUT attr,

> set->timeout will be used, but we forget to call msecs_to_jiffies

> when do update elements.

> 

> So it's better to call msecs_to_jiffies when updating elements.


Sorry about not catching the interaction between dynset and traditional
sets.

I believe that updating elements in dynsets can happen much more
frequently than interactions with userspace. One of my own use cases is
traffic accounting via flow tables, where there will be many updates
per second. So I would much prefer to do the conversion between msec
and jiffies, when data are sent to or received from userspace.

Would the patch below be okay?

Regards,
Anders K. Pedersen
---
 net/netfilter/nf_tables_api.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

2016-11-20 17:38 GMT+08:00 Anders K. Pedersen | Cohaesio <akp@cohaesio.com>:
[...]
> I believe that updating elements in dynsets can happen much more
> frequently than interactions with userspace. One of my own use cases is

Make sense. Considering the performance cost, your patch looks better:)

> traffic accounting via flow tables, where there will be many updates
> per second. So I would much prefer to do the conversion between msec
> and jiffies, when data are sent to or received from userspace.
>
> Would the patch below be okay?
>
> Regards,
> Anders K. Pedersen
> ---
>  net/netfilter/nf_tables_api.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 026581b..e5194f6f 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -2570,7 +2570,8 @@ static int nf_tables_fill_set(struct sk_buff *skb, const struct nft_ctx *ctx,
>         }
>
>         if (set->timeout &&
> -           nla_put_be64(skb, NFTA_SET_TIMEOUT, cpu_to_be64(set->timeout),
> +           nla_put_be64(skb, NFTA_SET_TIMEOUT,
> +                        cpu_to_be64(jiffies_to_msecs(set->timeout)),
>                          NFTA_SET_PAD))
>                 goto nla_put_failure;
>         if (set->gc_int &&
> @@ -2859,7 +2860,8 @@ static int nf_tables_newset(struct net *net, struct sock *nlsk,
>         if (nla[NFTA_SET_TIMEOUT] != NULL) {
>                 if (!(flags & NFT_SET_TIMEOUT))
>                         return -EINVAL;
> -               timeout = be64_to_cpu(nla_get_be64(nla[NFTA_SET_TIMEOUT]));
> +               timeout = msecs_to_jiffies(be64_to_cpu(nla_get_be64(
> +                                               nla[NFTA_SET_TIMEOUT])));
>         }
>         gc_int = 0;
>         if (nla[NFTA_SET_GC_INTERVAL] != NULL) {
> @@ -3178,7 +3180,8 @@ static int nf_tables_fill_setelem(struct sk_buff *skb,
>
>         if (nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT) &&
>             nla_put_be64(skb, NFTA_SET_ELEM_TIMEOUT,
> -                        cpu_to_be64(*nft_set_ext_timeout(ext)),
> +                        cpu_to_be64(jiffies_to_msecs(
> +                                               *nft_set_ext_timeout(ext))),
>                          NFTA_SET_ELEM_PAD))
>                 goto nla_put_failure;
>
> @@ -3447,7 +3450,7 @@ void *nft_set_elem_init(const struct nft_set *set,
>                 memcpy(nft_set_ext_data(ext), data, set->dlen);
>         if (nft_set_ext_exists(ext, NFT_SET_EXT_EXPIRATION))
>                 *nft_set_ext_expiration(ext) =
> -                       jiffies + msecs_to_jiffies(timeout);
> +                       jiffies + timeout;
>         if (nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT))
>                 *nft_set_ext_timeout(ext) = timeout;
>
> @@ -3535,7 +3538,8 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
>         if (nla[NFTA_SET_ELEM_TIMEOUT] != NULL) {
>                 if (!(set->flags & NFT_SET_TIMEOUT))
>                         return -EINVAL;
> -               timeout = be64_to_cpu(nla_get_be64(nla[NFTA_SET_ELEM_TIMEOUT]));
> +               timeout = msecs_to_jiffies(be64_to_cpu(nla_get_be64(
> +                                       nla[NFTA_SET_ELEM_TIMEOUT])));
>         } else if (set->flags & NFT_SET_TIMEOUT) {
>                 timeout = set->timeout;
>         }
--
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