Re: What is 'dynamic' set flag supposed to mean?

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Thu, Sep 19, 2019 at 01:56:36PM +0200, Florian Westphal wrote:
[...]
> My conclusion is that meters are anon sets with expressions attached to them.
> So, I don't think they should be deprecated.

At least, meters with names should be I think. There is now a way to
represent them 

> I would propose to:
> 
> 1. add this kernel patch:
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -3562,8 +3562,11 @@ static int nf_tables_newset(struct net *net, struct sock *nlsk,
>  			      NFT_SET_OBJECT))
>  			return -EINVAL;
>  		/* Only one of these operations is supported */
> -		if ((flags & (NFT_SET_MAP | NFT_SET_EVAL | NFT_SET_OBJECT)) ==
> -			     (NFT_SET_MAP | NFT_SET_EVAL | NFT_SET_OBJECT))
> +		if ((flags & (NFT_SET_MAP | NFT_SET_OBJECT)) ==
> +			     (NFT_SET_MAP | NFT_SET_OBJECT))

That's fine by now. But look, combining map and objects should be fine
in the future. A user might want to combine this by specifying an IP
address as the right hand side of a mapping and a stateful counter
(with a name) to be updated when matching on that element. This is not
supported yet though.

> +			return -EOPNOTSUPP;
> +		if ((flags & (NFT_SET_EVAL | NFT_SET_OBJECT)) ==
> +			     (NFT_SET_EVAL | NFT_SET_OBJECT))

This looks fine.

>  			return -EOPNOTSUPP;
>  	}
>  
> diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c
> --- a/net/netfilter/nft_lookup.c
> +++ b/net/netfilter/nft_lookup.c
> @@ -73,9 +73,6 @@ static int nft_lookup_init(const struct nft_ctx *ctx,
>  	if (IS_ERR(set))
>  		return PTR_ERR(set);
>  
> -	if (set->flags & NFT_SET_EVAL)
> -		return -EOPNOTSUPP;
> -
>  	priv->sreg = nft_parse_register(tb[NFTA_LOOKUP_SREG]);
>  	err = nft_validate_register_load(priv->sreg, set->klen);
>  	if (err < 0)
> 
> 2. avoid depreaction of 'meter', since thats what is documented everywhere
>    and appears to work fine

OK, but only for anonymous meters.

We have a better way, more aligned to set/map definitions, to represent
updates to dynamic named sets from the packet path now.

We can probably introduce a syntax more aligned to the existing
set/map syntax for the _anonymous_ dynamic set/map case, so we don't
need this 'meter' keyword syntactic sugar.

> 3. patch nft to hide normal sets from 'nft list meters' output

This makes no sense for anonymous meters. Since the kernel picks the
name, I don't think nft should expose them to the user.

> What to do with dynamic, I fear we have to remove it, i.e. just ignore
> the "dynamic" flag when talking to the kernel.  Otherwise sets using dynamic flag
> will only work on future/very recent kernels.

I would not go that path.

You are just hitting a limitation on the existing implementation.
People cannot make lookups on a dynamic set on existing kernels,
that's all.

There is no NFT_SET_EXPR support for nft_lookup either, is this a bug?
This infrastructure is just incomplete and just need to be finished.

There is no way to combine NFT_SET_MAP with NFT_SET_OBJ, but that is
also artificial, this is just happening again because the code is
incomplete and needs an extension.