What is 'dynamic' set flag supposed to mean?

What is 'dynamic' set flag supposed to mean?

[Florian Westphal]

Hi.

Following example loads fine:
table ip NAT {
  set set1 {
    type ipv4_addr
    size 64
    flags dynamic,timeout
    timeout 1m
  }

  chain PREROUTING {
     type nat hook prerouting priority -101; policy accept;
  }
}

But adding/using this set doesn't work:
nft -- add rule NAT PREROUTING tcp dport 80 ip saddr @set1 counter
Error: Could not process rule: Operation not supported

This is because the 'dynamic' flag sets NFT_SET_EVAL.

According to kernel comment, that flag means:
 * @NFT_SET_EVAL: set can be updated from the evaluation path

The rule add is rejected from the lookup expression (nft_lookup_init)
which has:

if (set->flags & NFT_SET_EVAL)
    return -EOPNOTSUPP;

From looking at the git history, the NFT_SET_EVAL flag means that the
set contains expressions (i.e., a meter).

And I can see why doing a lookup on meters isn't meaningful.

Can someone please explain the exact precise meaning of 'dynamic'?
Was it supposed to mean 'set can be updated from packet path'?
Or was it supposed to mean 'set contains expressions'?

If its the latter, do we need a new NFT_SET flag to convey 'set
needs to support updates from packet path'?

Thanks.

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

[Laura Garcia]

Hi Florian,

On Wed, Sep 18, 2019 at 3:20 PM Florian Westphal <fw@strlen.de> wrote:
>
> Hi.
>
> Following example loads fine:
> table ip NAT {
>   set set1 {
>     type ipv4_addr
>     size 64
>     flags dynamic,timeout
>     timeout 1m
>   }
>
>   chain PREROUTING {
>      type nat hook prerouting priority -101; policy accept;
>   }
> }
>
> But adding/using this set doesn't work:
> nft -- add rule NAT PREROUTING tcp dport 80 ip saddr @set1 counter
> Error: Could not process rule: Operation not supported
>

If this set is only for matching, 'dynamic' is not required.

> This is because the 'dynamic' flag sets NFT_SET_EVAL.
>
> According to kernel comment, that flag means:
>  * @NFT_SET_EVAL: set can be updated from the evaluation path
>
> The rule add is rejected from the lookup expression (nft_lookup_init)
> which has:
>
> if (set->flags & NFT_SET_EVAL)
>     return -EOPNOTSUPP;
>
> From looking at the git history, the NFT_SET_EVAL flag means that the
> set contains expressions (i.e., a meter).
>
> And I can see why doing a lookup on meters isn't meaningful.
>
> Can someone please explain the exact precise meaning of 'dynamic'?
> Was it supposed to mean 'set can be updated from packet path'?
> Or was it supposed to mean 'set contains expressions'?
>

AFAIK, I traduce the 'dynamic' flag as a 'set that is updated from the
packet path using an expression', formerly 'meter'.

> If its the latter, do we need a new NFT_SET flag to convey 'set
> needs to support updates from packet path'?
>

In all use cases I have (mainly connection limits), 'update' is not
required so far.

> Thanks.

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

[Florian Westphal]

Laura Garcia <nevola@gmail.com> wrote:
> > Following example loads fine:
> > table ip NAT {
> >   set set1 {
> >     type ipv4_addr
> >     size 64
> >     flags dynamic,timeout
> >     timeout 1m
> >   }
> >
> >   chain PREROUTING {
> >      type nat hook prerouting priority -101; policy accept;
> >   }
> > }
> >
> > But adding/using this set doesn't work:
> > nft -- add rule NAT PREROUTING tcp dport 80 ip saddr @set1 counter
> > Error: Could not process rule: Operation not supported
> 
> If this set is only for matching, 'dynamic' is not required.

I know, and the example above works if the 'dynamic' flag is omitted.

> > This is because the 'dynamic' flag sets NFT_SET_EVAL.
> >
> > According to kernel comment, that flag means:
> >  * @NFT_SET_EVAL: set can be updated from the evaluation path
> >
> > The rule add is rejected from the lookup expression (nft_lookup_init)
> > which has:
> >
> > if (set->flags & NFT_SET_EVAL)
> >     return -EOPNOTSUPP;
> >
> > From looking at the git history, the NFT_SET_EVAL flag means that the
> > set contains expressions (i.e., a meter).
> >
> > And I can see why doing a lookup on meters isn't meaningful.
> >
> > Can someone please explain the exact precise meaning of 'dynamic'?
> > Was it supposed to mean 'set can be updated from packet path'?
> > Or was it supposed to mean 'set contains expressions'?
> >
> 
> AFAIK, I traduce the 'dynamic' flag as a 'set that is updated from the
> packet path using an expression', formerly 'meter'.

That would mean the kernel comment quoted above is wrong and should be
patched to say

* @NFT_SET_EVAL: set can be updated from the packet path and stores expressions.

Unfortunately, this seems to contradict various nftables.git changelog
messages which seem to imply that 'dynamic' means

@NFT_SET_EVAL: set can be updated from the evaluation path

i.e., make sure that set provides an ->update() implementation so

'add @set1 { ip saddr }' and the like work.

The core issue is that when saying

   set set1 {
      type ipv4_addr
      size 64
       flags timeout
       timeout 1m
    }

The kernel has no information on how this set is going to be used.
For 'ip saddr @set1' this will just work as all sets implement
->lookup().

But will this work reliably:
nft add rule ... add @set1 { ip saddr }

At this time, it looks like when specifiying the mandatory 'timeout'
flag, the kernel happens to pick the rhashtable backend so it will work.

But I wonder if we're just lucky or if this is intentional, i.e.
'timeout' means that the set can be altered from the packet path.

In any case, this needs to be documented in nft.8, its telling that I
can't be 100% about the intent of dynamic/EVAL even after reading both
nftables.git and kernel implementation.

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

[Pablo Neira Ayuso]

On Wed, Sep 18, 2019 at 04:42:35PM +0200, Florian Westphal wrote:
> Laura Garcia <nevola@gmail.com> wrote:
> > > Following example loads fine:
> > > table ip NAT {
> > >   set set1 {
> > >     type ipv4_addr
> > >     size 64
> > >     flags dynamic,timeout
> > >     timeout 1m
> > >   }
> > >
> > >   chain PREROUTING {
> > >      type nat hook prerouting priority -101; policy accept;
> > >   }
> > > }
> > >
> > > But adding/using this set doesn't work:
> > > nft -- add rule NAT PREROUTING tcp dport 80 ip saddr @set1 counter
> > > Error: Could not process rule: Operation not supported
> > 
> > If this set is only for matching, 'dynamic' is not required.
> 
> I know, and the example above works if the 'dynamic' flag is omitted.

Looks like a kernel bug, kernel is selecting the fixed size hash with
the dynamic flag. That should not happen.

> > > This is because the 'dynamic' flag sets NFT_SET_EVAL.
> > >
> > > According to kernel comment, that flag means:
> > >  * @NFT_SET_EVAL: set can be updated from the evaluation path
> > >
> > > The rule add is rejected from the lookup expression (nft_lookup_init)
> > > which has:
> > >
> > > if (set->flags & NFT_SET_EVAL)
> > >     return -EOPNOTSUPP;
> > >
> > > From looking at the git history, the NFT_SET_EVAL flag means that the
> > > set contains expressions (i.e., a meter).
> > >
> > > And I can see why doing a lookup on meters isn't meaningful.
> > >
> > > Can someone please explain the exact precise meaning of 'dynamic'?
> > > Was it supposed to mean 'set can be updated from packet path'?
> > > Or was it supposed to mean 'set contains expressions'?
> > >
> > 
> > AFAIK, I traduce the 'dynamic' flag as a 'set that is updated from the
> > packet path using an expression', formerly 'meter'.
> 
> That would mean the kernel comment quoted above is wrong and should be
> patched to say
> 
> * @NFT_SET_EVAL: set can be updated from the packet path and stores expressions.

The comment is correct. NFT_SET_EVAL semantics is "this set can be
updated from the packet path", this helps the kernel selects a set
backend that implements the ->update interface.

The expression is option, if the netlink attribute to describe the
extension is set, then it used.

> Unfortunately, this seems to contradict various nftables.git changelog
> messages which seem to imply that 'dynamic' means
> 
> @NFT_SET_EVAL: set can be updated from the evaluation path
> 
> i.e., make sure that set provides an ->update() implementation so
> 
> 'add @set1 { ip saddr }' and the like work.
> 
> The core issue is that when saying
> 
>    set set1 {
>       type ipv4_addr
>       size 64
>        flags timeout
>        timeout 1m
>     }
> 
> The kernel has no information on how this set is going to be used.
> For 'ip saddr @set1' this will just work as all sets implement
> ->lookup().
> 
> But will this work reliably:
> nft add rule ... add @set1 { ip saddr }

No, because the dynamic flag is not set.

> At this time, it looks like when specifiying the mandatory 'timeout'
> flag, the kernel happens to pick the rhashtable backend so it will work.

Probably it would be good to implicitly set on timeout flag if dynamic
flag is set on, just like it happens with set size. To relax the
control plane interface a bit (IIRC the user currently gets an error
if you forget to set the timeout flag in a set that is updated from
the packet path).

> But I wonder if we're just lucky or if this is intentional, i.e.
> 'timeout' means that the set can be altered from the packet path.

I think we're just being lucky :-)

> In any case, this needs to be documented in nft.8, its telling that I
> can't be 100% about the intent of dynamic/EVAL even after reading both
> nftables.git and kernel implementation.

Sure.

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Sep 18, 2019 at 04:42:35PM +0200, Florian Westphal wrote:
> > Laura Garcia <nevola@gmail.com> wrote:
> > > > Following example loads fine:
> > > > table ip NAT {
> > > >   set set1 {
> > > >     type ipv4_addr
> > > >     size 64
> > > >     flags dynamic,timeout
> > > >     timeout 1m
> > > >   }
> > > >
> > > >   chain PREROUTING {
> > > >      type nat hook prerouting priority -101; policy accept;
> > > >   }
> > > > }
> > > >
> > > > But adding/using this set doesn't work:
> > > > nft -- add rule NAT PREROUTING tcp dport 80 ip saddr @set1 counter
> > > > Error: Could not process rule: Operation not supported
> > > 
> > > If this set is only for matching, 'dynamic' is not required.
> > 
> > I know, and the example above works if the 'dynamic' flag is omitted.
> 
> Looks like a kernel bug, kernel is selecting the fixed size hash with
> the dynamic flag. That should not happen.

No, it selects the rhashtable one -- its the only one that sets
NFT_SET_EVAL.

> > > > The rule add is rejected from the lookup expression (nft_lookup_init)
> > > > which has:
> > > >
> > > > if (set->flags & NFT_SET_EVAL)
> > > >     return -EOPNOTSUPP;

... and thats the reason why it won't work.  "dynamic" flag disables
lookup expression for the given set.

I can't remove the if () because that would make it possible to lookup
for meter-type sets.

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

[Pablo Neira Ayuso]

On Thu, Sep 19, 2019 at 11:24:42AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, Sep 18, 2019 at 04:42:35PM +0200, Florian Westphal wrote:
> > > Laura Garcia <nevola@gmail.com> wrote:
> > > > > Following example loads fine:
> > > > > table ip NAT {
> > > > >   set set1 {
> > > > >     type ipv4_addr
> > > > >     size 64
> > > > >     flags dynamic,timeout
> > > > >     timeout 1m
> > > > >   }
> > > > >
> > > > >   chain PREROUTING {
> > > > >      type nat hook prerouting priority -101; policy accept;
> > > > >   }
> > > > > }
> > > > >
> > > > > But adding/using this set doesn't work:
> > > > > nft -- add rule NAT PREROUTING tcp dport 80 ip saddr @set1 counter
> > > > > Error: Could not process rule: Operation not supported
> > > > 
> > > > If this set is only for matching, 'dynamic' is not required.
> > > 
> > > I know, and the example above works if the 'dynamic' flag is omitted.
> > 
> > Looks like a kernel bug, kernel is selecting the fixed size hash with
> > the dynamic flag. That should not happen.
> 
> No, it selects the rhashtable one -- its the only one that sets
> NFT_SET_EVAL.
> 
> > > > > The rule add is rejected from the lookup expression (nft_lookup_init)
> > > > > which has:
> > > > >
> > > > > if (set->flags & NFT_SET_EVAL)
> > > > >     return -EOPNOTSUPP;
> 
> ... and thats the reason why it won't work.  "dynamic" flag disables
> lookup expression for the given set.
> 
> I can't remove the if () because that would make it possible to lookup
> for meter-type sets.

Why is this a problem? meter-set are basically a set with a
counter/quota/etc... that is created from the packet path. It should
be possible to make lookups on the content of this set.

I think we can just check instead from nft_lookup if there is an
extension in this then, instead of checking for the NFT_SET_EVAL flag
to fix this. Hence, you can make lookups on dynamic sets, but not on
dynamic sets with extensions.

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I can't remove the if () because that would make it possible to lookup
> > for meter-type sets.
> 
> Why is this a problem?

I was worried about this exposing expr pointers in the nft registers but
that won't happen (lookup expr doesn't care, only dynset will check for
attached expression coming from set).

I will send a patch to zap this check.
However, that still is a problem because that means "dynamic" can't
be used in kernels < 5.4 .

> I think we can just check instead from nft_lookup if there is an
> extension in this then, instead of checking for the NFT_SET_EVAL flag
> to fix this. Hence, you can make lookups on dynamic sets, but not on
> dynamic sets with extensions.

What do you mean?

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

[Pablo Neira Ayuso]

On Thu, Sep 19, 2019 at 12:03:29PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > I can't remove the if () because that would make it possible to lookup
> > > for meter-type sets.
> > 
> > Why is this a problem?
> 
> I was worried about this exposing expr pointers in the nft registers but
> that won't happen (lookup expr doesn't care, only dynset will check for
> attached expression coming from set).

See reply at the bottom of this email regarding ignoring the attached
expression.

> I will send a patch to zap this check.
> However, that still is a problem because that means "dynamic" can't
> be used in kernels < 5.4 .

I think this qualifies as a fix, it will be a two-liner, we could
send it to -stable?

> > I think we can just check instead from nft_lookup if there is an
> > extension in this then, instead of checking for the NFT_SET_EVAL flag
> > to fix this. Hence, you can make lookups on dynamic sets, but not on
> > dynamic sets with extensions.
> 
> What do you mean?

I was thinking about the counter per set element case, this is
something we don't support and IIRC ipset does. After this fix, we can
probably make a patch to check if the NFT_SET_EXT_EXPR exists, so we
can add counter per element matching a lookup. We also need a way to
say that this set has an expression counter when definiting the set.
At some point we might need to support for stateful objects per set
element too so users can also dump-and-reset an specific element
counter.

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

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > I can't remove the if () because that would make it possible to lookup
> > > for meter-type sets.
> > 
> > Why is this a problem?
> 
> I was worried about this exposing expr pointers in the nft registers but
> that won't happen (lookup expr doesn't care, only dynset will check for
> attached expression coming from set).
> 
> I will send a patch to zap this check.

Scratch that, I still don't understand all of this.
nf_tables_api.c had this check:

/* Only one of both operations is supported */
if ((flags & (NFT_SET_MAP | NFT_SET_EVAL)) == (NFT_SET_MAP | NFT_SET_EVAL))
     return -EOPNOTSUPP;

This got converted to
/* 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))
     return -EOPNOTSUPP;

So, comment and code do not match anymore

Fixing the code to reflect the comment:

const u32 excl = NFT_SET_MAP | NFT_SET_EVAL | NFT_SET_OBJECT;
if (hweight32(flags & excl) > 1)
      return -EOPNOTSUPP;

breaks cases in the test suite, for example:
W: [FAILED]     tests/shell/testcases/cache/0005_cache_chain_flush: got 1
/dev/stdin:4:1-79: Error: Could not process rule: Operation not supported
add map ip x mapping { type ipv4_addr : inet_service; flags dynamic,timeout; }

... because this sets NFT_SET_MAP|NFT_SET_EVAL.

What is the purpose/intent of that check, i.e. which combinations
are safe and which should be rejected?

I would guess the check should be
NFT_SET_MAP|NFT_SET_OBJECT -> reject
NFT_SET_EVAL|NFT_SET_OBJECT -> reject

... and nft tests pass on a kernel with that change.

Removing the NFT_SET_EVAL rejection in nft_lookup init routine makes
this work:

table ip filter {
        set set1 {
                type ipv4_addr
                size 65535
                flags dynamic,timeout
        }

        chain input {
                type filter hook input priority filter; policy accept;
                add @set1 { ip saddr }
                update @set1 { ip daddr timeout 23s counter }
                ip saddr @set1 counter
        }
}

$ nft list ruleset
table ip filter {
  set set1 {
     type ipv4_addr
     size 65535
     flags dynamic,timeout
     elements = { 192.168.7.1, 192.168.7.2 expires 22s991ms counter packets 33 bytes 2300 }
  }
 ...

but nft -f can't restore this because it chokes on unexpected 'counter' appearing in elements = {}.

Note that the new proposed 'set dynamic' as replacement of 'meters' also means users can mix
different expressions, e.g.
add @set1 { ip saddr }			# add element with no expression
add @set1 { ip daddr counter }		# add element with counter
# this could be another rule adding another address with quota etc.

This is fine from kernel pov, but it needs to be documented as well.

Things get even more confusing when adding a meter:

nft add rule filter input meter foo { ip saddr timeout 5m limit rate 10/second }

yields:
table ip filter {
        set set1 {
                type ipv4_addr
                size 65535
                flags dynamic,timeout
                elements = { 192.168.7.1, 192.168.7.2 expires 22s992ms counter packets 15 bytes 1064 }
        }

        chain input {
                type filter hook input priority filter; policy accept;
                add @set1 { ip saddr }
                update @set1 { ip daddr timeout 23s counter }
                ip saddr @set1 counter packets 706 bytes 49516
                meter foo size 65535 { ip saddr timeout 5m limit rate 10/second }
        }
}

nft list meters
table ip filter {
        set set1 {
                type ipv4_addr
                size 65535
                flags dynamic,timeout
        }
        meter foo {
                type ipv4_addr
                size 65535
                flags dynamic,timeout
        }
}
# Note: set1 is a set, foo is a meter.
# This is because nft will show "meter" for sets
# that have flags NFT_SET_EVAL|NFT_SET_ANONYMOUS.

I think it might be better to suppress the 'set1' in this
listing, or treat it like a meter:
nft list meter filter set1
Error: No such file or directory

... because they can't be listed this way.
'nft list set filter set1' works of course.

nft list meter filter foo
table ip filter {
        meter foo {
                type ipv4_addr
                size 65535
                flags dynamic,timeout
                elements = { 192.168.7.1 expires 4m59s992ms limit rate 10/second }
        }
}

My conclusion is that meters are anon sets with expressions attached to them.
So, I don't think they should be deprecated.

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))
+			return -EOPNOTSUPP;
+		if ((flags & (NFT_SET_EVAL | NFT_SET_OBJECT)) ==
+			     (NFT_SET_EVAL | NFT_SET_OBJECT))
 			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

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

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.

Seems we need to rely on 'flag timeout' picking an update-able set.

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

[Pablo Neira Ayuso]

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.

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >  		/* 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.

I'll submit a patch then.

> > 2. avoid depreaction of 'meter', since thats what is documented everywhere
> >    and appears to work fine
> 
> OK, but only for anonymous meters.

Meters are always anonymous afaiu, they are bound to the rule that
creates them.  Delete the rule -- meter is gone.

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

See
commit 24a912eea21f9d18909c53a865cf623839616281
parser_bison: dismiss anonymous meters

nft frontend only allows to create meter with a name.

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

I guess thats one way to put it.

> There is no NFT_SET_EXPR support for nft_lookup either, is this a bug?

Do you mean NFT_SET_EVAL?
If so, NFT_SET_EVAL is handled by nft_dynset.c (this is what gets used
when you use the 'meter' syntax).

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

Ok.  We can drop the checks once that is done.

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

[Pablo Neira Ayuso]

On Thu, Sep 19, 2019 at 04:01:44PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >  		/* 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.
> 
> I'll submit a patch then.

Thanks.

> > > 2. avoid depreaction of 'meter', since thats what is documented everywhere
> > >    and appears to work fine
> > 
> > OK, but only for anonymous meters.
> 
> Meters are always anonymous afaiu, they are bound to the rule that
> creates them.  Delete the rule -- meter is gone.

OK. sorry, NFT_SET_ANONYMOUS is misleading, this anonymous flag means
'set is bound to rule', it was wrong to choose this flag name,
probably NFT_SET_BOUND would be better. Anyway, I was referring to
meter with no name.

> > > 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.
> 
> See
> commit 24a912eea21f9d18909c53a865cf623839616281
> parser_bison: dismiss anonymous meters
> 
> nft frontend only allows to create meter with a name.

Oh, we already deprecated meters with no name :-)

And meters with a name should not be used, there's a better syntax
now for this rather than this sloppy way to create an object from the
rule itself without an explicit definition.

> > > 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.
> 
> I guess thats one way to put it.
>
> > There is no NFT_SET_EXPR support for nft_lookup either, is this a bug?
> 
> Do you mean NFT_SET_EVAL?

No, I mean there is no NFT_SET_EXT_EXPR handling yet, sorry I forgot
the _EXT_ infix.

nft_lookup should invoke the expression that is attached. Control
plane code is also missing, there is no way to create the
NFT_SET_EXT_EXPR from newsetelem() in nf_tables_api.c.

If NFT_SET_EVAL is set or not from nft_lookup is completely
irrelevant, nft_lookup should not care about this flag.

> If so, NFT_SET_EVAL is handled by nft_dynset.c (this is what gets used
> when you use the 'meter' syntax).
> 
> > 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.
> 
> Ok.  We can drop the checks once that is done.

Agreed.

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Sep 19, 2019 at 04:01:44PM +0200, Florian Westphal wrote:
> > Do you mean NFT_SET_EVAL?
> 
> No, I mean there is no NFT_SET_EXT_EXPR handling yet, sorry I forgot
> the _EXT_ infix.
> 
> nft_lookup should invoke the expression that is attached. Control
> plane code is also missing, there is no way to create the
> NFT_SET_EXT_EXPR from newsetelem() in nf_tables_api.c.

Hmm, no, I don't think it should.
Otherwise lookups on a set that has counters added to it will
increment the counter values.

I think we should leave all munging to nft_dynset.c, i.e. add/update
in terms of nft frontend set syntax.

> If NFT_SET_EVAL is set or not from nft_lookup is completely
> irrelevant, nft_lookup should not care about this flag.

Right, I will try to reflect that in the commit message.

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

[Pablo Neira Ayuso]

On Thu, Sep 19, 2019 at 04:34:31PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Thu, Sep 19, 2019 at 04:01:44PM +0200, Florian Westphal wrote:
> > > Do you mean NFT_SET_EVAL?
> > 
> > No, I mean there is no NFT_SET_EXT_EXPR handling yet, sorry I forgot
> > the _EXT_ infix.
> > 
> > nft_lookup should invoke the expression that is attached. Control
> > plane code is also missing, there is no way to create the
> > NFT_SET_EXT_EXPR from newsetelem() in nf_tables_api.c.
> 
> Hmm, no, I don't think it should.
> Otherwise lookups on a set that has counters added to it will
> increment the counter values.

ipset can attach counter to elements, so matching lookups bump the
element counter. I think users might want for this in the future, just
to keep this usecase in the radar.

> I think we should leave all munging to nft_dynset.c, i.e. add/update
> in terms of nft frontend set syntax.
> 
> > If NFT_SET_EVAL is set or not from nft_lookup is completely
> > irrelevant, nft_lookup should not care about this flag.
> 
> Right, I will try to reflect that in the commit message.

Thanks.