[v3,1/2] net: nf_tables: Make nft_meta expression more robust

nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in
situations where required data is missing leads to unexpected behaviour
with inverted checks like so:

| meta iifname != eth0 accept

This rule will never match if there is no input interface (or it is not
known) which is not intuitive and, what's worse, breaks consistency of
iptables-nft with iptables-legacy.

Fix this by falling back to placing a value in dreg which never matches
(avoiding accidental matches), i.e. zero for interface index and an
empty string for interface name.
---
Changes since v1:
- Apply same fix to net/bridge/netfilter/nft_meta_bridge.c as well.

Changes since v2:
- Limit this fix to address only expressions returning an interface
  index or name. As Florian correctly criticized, these changes may be
  problematic as they tend to change nftables' behaviour. Hence fix only
  the cases needed to establish iptables-nft compatibility.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/bridge/netfilter/nft_meta_bridge.c |  6 +-----
 net/netfilter/nft_meta.c               | 16 ++++------------
 2 files changed, 5 insertions(+), 17 deletions(-)

On Tue, Jul 23, 2019 at 03:27:52PM +0200, Phil Sutter wrote:
> nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in
> situations where required data is missing leads to unexpected behaviour
> with inverted checks like so:
> 
> | meta iifname != eth0 accept
> 
> This rule will never match if there is no input interface (or it is not
> known) which is not intuitive and, what's worse, breaks consistency of
> iptables-nft with iptables-legacy.
> 
> Fix this by falling back to placing a value in dreg which never matches
> (avoiding accidental matches), i.e. zero for interface index and an
> empty string for interface name.
> ---
> Changes since v1:
> - Apply same fix to net/bridge/netfilter/nft_meta_bridge.c as well.
> 
> Changes since v2:
> - Limit this fix to address only expressions returning an interface
>   index or name. As Florian correctly criticized, these changes may be
>   problematic as they tend to change nftables' behaviour. Hence fix only
>   the cases needed to establish iptables-nft compatibility.

This leaves things in an inconsistent situation.

What's the concern here? Matching iifgroup/oifgroup from the wrong
packet path?

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Changes since v1:
> > - Apply same fix to net/bridge/netfilter/nft_meta_bridge.c as well.
> > 
> > Changes since v2:
> > - Limit this fix to address only expressions returning an interface
> >   index or name. As Florian correctly criticized, these changes may be
> >   problematic as they tend to change nftables' behaviour. Hence fix only
> >   the cases needed to establish iptables-nft compatibility.
> 
> This leaves things in an inconsistent situation.
> 
> What's the concern here? Matching iifgroup/oifgroup from the wrong
> packet path?

There is no solution for this problem -- thats the core issue.

Purely from a technical p.o.v., we're asking to match oif, but its not
there, so to me, the current behaviour is the right one: break, because
we can't provide the requested information.

On the other hand, this makes things rather un-intuitive when asking for
not eq "foo" -- and not getting a match.

Plus, there is the "compat" angle.

For iifname and ifindex we're safe because "" is not a valid interface
name and 0 is not a valid ifindex.  But for everything else, I'd be
extra careful.

One alternative is to add the "compat" netlink attribute for
iptables-nft and only change behaviour for iptables-nft sake.

But I feel that noone has real evidence in this matter, just hunches.

If in doubt, I would even prefer to keep things as-is and add the
compat attr for meta so we get iptables-nft to behave like
iptables-legacy and keep nft as-is.

This will become relevant once we get a saner behaviour for non-matching
sets (such as a default action):

meta iifname vmap { "foo" : jump foochain, "bar" : accept, default : jump "rest" }

Would you expect the "packet has no incoming interface" to hit the
default action or not?

If we change things now (set ifindex 0 / "" name), I do not think
we can't revert it later.

Florian Westphal <fw@strlen.de> wrote:
> If we change things now (set ifindex 0 / "" name), I do not think
> we can't revert it later.

Grrr, should not reply at this hour.  I meant
"I do not think we can revert it later" -- if we change it now
it will be set in stone.

On Wed, Jul 24, 2019 at 12:33:06AM +0200, Florian Westphal wrote:
[...]
> If we change things now (set ifindex 0 / "" name), I do not think
> we can't revert it later.

OK, let's start simple as you propose, with iif/oif/iifname/oifname
and we revisit this later on.

Thanks for explaining.

On Tue, Jul 23, 2019 at 03:27:52PM +0200, Phil Sutter wrote:
> nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in
> situations where required data is missing leads to unexpected behaviour
> with inverted checks like so:
> 
> | meta iifname != eth0 accept
> 
> This rule will never match if there is no input interface (or it is not
> known) which is not intuitive and, what's worse, breaks consistency of
> iptables-nft with iptables-legacy.
> 
> Fix this by falling back to placing a value in dreg which never matches
> (avoiding accidental matches), i.e. zero for interface index and an
> empty string for interface name.

Applied, thanks.