[PATCH nft] doc: fib: explain example in more detail

[PATCH nft] doc: fib: explain example in more detail

[Florian Westphal]

As noted by Felix Dreissig, fib documentation is quite terse, so explain
the 'saddr . iif' example with a few more words.

Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1220
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 doc/primary-expression.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/doc/primary-expression.txt b/doc/primary-expression.txt
index 6eb9583ac9e9..124193626aa7 100644
--- a/doc/primary-expression.txt
+++ b/doc/primary-expression.txt
@@ -274,6 +274,12 @@ fib_addrtype
 # drop packets without a reverse path
 filter prerouting fib saddr . iif oif missing drop
 
+In this example, 'saddr . iif' lookups up routing information based on the source address and the input interface.
+oif picks the output interface index from the routing information.
+If no route was found for the source address/input interface combination, the output interface index is zero.
+In case the input interface is specified as part of the input key, the output interface index is always the same as the input interface index or zero.
+If only 'saddr oif' is given, then oif can be any interface index or zero.
+
 # drop packets to address not configured on ininterface
 filter prerouting fib daddr . iif type != { local, broadcast, multicast } drop
 
-- 
2.21.0

Re: [PATCH nft] doc: fib: explain example in more detail

[Jones Desougi]

On Sun, Jul 21, 2019 at 12:48 PM Florian Westphal <fw@strlen.de> wrote:
...
>
> diff --git a/doc/primary-expression.txt b/doc/primary-expression.txt
> index 6eb9583ac9e9..124193626aa7 100644
> --- a/doc/primary-expression.txt
> +++ b/doc/primary-expression.txt
> @@ -274,6 +274,12 @@ fib_addrtype
>  # drop packets without a reverse path
>  filter prerouting fib saddr . iif oif missing drop
>
> +In this example, 'saddr . iif' lookups up routing information based on the source address and the input interface.

s/lookups up/looks up/

Re: [PATCH nft] doc: fib: explain example in more detail

[Pablo Neira Ayuso]

Cc'ing Phil.

On Sun, Jul 21, 2019 at 12:43:05PM +0200, Florian Westphal wrote:
> As noted by Felix Dreissig, fib documentation is quite terse, so explain
> the 'saddr . iif' example with a few more words.

There are patches to disallow ifindex 0 from Phil, probably we should
leave them behind given that fib allows matching on zero in case of no
matchings. This is also done from userspace, anyone using the API
directly may also skip this.

Telling all this because we should be consistent on this.

Re: [PATCH nft] doc: fib: explain example in more detail

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Cc'ing Phil.
> 
> On Sun, Jul 21, 2019 at 12:43:05PM +0200, Florian Westphal wrote:
> > As noted by Felix Dreissig, fib documentation is quite terse, so explain
> > the 'saddr . iif' example with a few more words.
> 
> There are patches to disallow ifindex 0 from Phil

WHich ones?
I only see those that make meta write 0 in case iface doesn't exist,
so it does exactly what fib does.

Re: [PATCH nft] doc: fib: explain example in more detail

[Phil Sutter]

On Sun, Jul 21, 2019 at 08:54:32PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Cc'ing Phil.
> > 
> > On Sun, Jul 21, 2019 at 12:43:05PM +0200, Florian Westphal wrote:
> > > As noted by Felix Dreissig, fib documentation is quite terse, so explain
> > > the 'saddr . iif' example with a few more words.
> > 
> > There are patches to disallow ifindex 0 from Phil
> 
> WHich ones?
> I only see those that make meta write 0 in case iface doesn't exist,
> so it does exactly what fib does.

It is message-ID 20190718033940.12820-1-phil@nwl.cc and follow-ups,
trying to prevent users from matching against the values we decided to
use for "no data available" situations. This whole attempt feels a bit
futile. Maybe we should introduce something to signal "no value" so that
cmp expression will never match for '==' and always for '!='? Not sure
how to realize this via registers. Also undecided about '<' and '>' ops.

Cheers, Phil

Re: [PATCH nft] doc: fib: explain example in more detail

[Pablo Neira Ayuso]

On Sun, Jul 21, 2019 at 08:54:32PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Cc'ing Phil.
> > 
> > On Sun, Jul 21, 2019 at 12:43:05PM +0200, Florian Westphal wrote:
> > > As noted by Felix Dreissig, fib documentation is quite terse, so explain
> > > the 'saddr . iif' example with a few more words.
> > 
> > There are patches to disallow ifindex 0 from Phil
> 
> WHich ones?

https://patchwork.ozlabs.org/patch/1133521/

> I only see those that make meta write 0 in case iface doesn't exist,
> so it does exactly what fib does.

This one disallows matching on zero, while you can match on ifindex 0
from fib to check for no routes, correct?

Re: [PATCH nft] doc: fib: explain example in more detail

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, Jul 21, 2019 at 08:54:32PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Cc'ing Phil.
> > > 
> > > On Sun, Jul 21, 2019 at 12:43:05PM +0200, Florian Westphal wrote:
> > > > As noted by Felix Dreissig, fib documentation is quite terse, so explain
> > > > the 'saddr . iif' example with a few more words.
> > > 
> > > There are patches to disallow ifindex 0 from Phil
> > 
> > WHich ones?
> 
> https://patchwork.ozlabs.org/patch/1133521/

This is insane. "nft meta iif 0 counter" seems totally fine to me.

I think we should never try to be clever here.

Re: [PATCH nft] doc: fib: explain example in more detail

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> use for "no data available" situations. This whole attempt feels a bit
> futile. Maybe we should introduce something to signal "no value" so that
> cmp expression will never match for '==' and always for '!='? Not sure
> how to realize this via registers. Also undecided about '<' and '>' ops.

Whats the point?

Re: [PATCH nft] doc: fib: explain example in more detail

[Phil Sutter]

On Mon, Jul 22, 2019 at 02:17:47PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > use for "no data available" situations. This whole attempt feels a bit
> > futile. Maybe we should introduce something to signal "no value" so that
> > cmp expression will never match for '==' and always for '!='? Not sure
> > how to realize this via registers. Also undecided about '<' and '>' ops.
> 
> Whats the point?

IIRC, Pablo's demand for not aborting in nft_meta in case of
insufficient data was to insert a value into dreg which will never
match. I think the idea was to avoid accidental matching in situations
where a match doesn't make sense.

For ifindex or ifname I can't come up with a good example, but let's
assume we set dreg to 0 for 'meta l4proto' if pkt->tprot is not
initialized (i.e., pkt->tprot_set is false for whatever reason). A rule
'meta l4proto == 0' would start to match even if l4proto of the packet
is not 0.

Remember, the original problem was that with iptables-legacy, I can do
'! -i foobar' in POSTROUTING chain and it will always match. With
iptables-nft, the same rule will never match.

Maybe we should just go with fixing for iifname/oifname only and leave
the rest as-is?

Cheers, Phil

Re: [PATCH nft] doc: fib: explain example in more detail

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> On Mon, Jul 22, 2019 at 02:17:47PM +0200, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > use for "no data available" situations. This whole attempt feels a bit
> > > futile. Maybe we should introduce something to signal "no value" so that
> > > cmp expression will never match for '==' and always for '!='? Not sure
> > > how to realize this via registers. Also undecided about '<' and '>' ops.
> > 
> > Whats the point?
> 
> IIRC, Pablo's demand for not aborting in nft_meta in case of
> insufficient data was to insert a value into dreg which will never
> match. I think the idea was to avoid accidental matching in situations
> where a match doesn't make sense.

I think the only contraint is that it must not overlap with a
legitimate ifindex.

But 0 cannot occur, so 'meta iif 0' will only match in case no input
interface existed -- I think thats fine and might even be desired.

> For ifindex or ifname I can't come up with a good example, but let's
> assume we set dreg to 0 for 'meta l4proto' if pkt->tprot is not
> initialized (i.e., pkt->tprot_set is false for whatever reason). A rule
> 'meta l4proto == 0' would start to match even if l4proto of the packet
> is not 0.

Thats my point, we can't use a value that can occur normally.

> Remember, the original problem was that with iptables-legacy, I can do
> '! -i foobar' in POSTROUTING chain and it will always match. With
> iptables-nft, the same rule will never match.

O know.

> Maybe we should just go with fixing for iifname/oifname only and leave
> the rest as-is?

I would propose to go with '0' dreg for ifindex, "" for name and leave
rest as-is.

Re: [PATCH nft] doc: fib: explain example in more detail

[Phil Sutter]

On Mon, Jul 22, 2019 at 02:56:33PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > On Mon, Jul 22, 2019 at 02:17:47PM +0200, Florian Westphal wrote:
> > > Phil Sutter <phil@nwl.cc> wrote:
> > > > use for "no data available" situations. This whole attempt feels a bit
> > > > futile. Maybe we should introduce something to signal "no value" so that
> > > > cmp expression will never match for '==' and always for '!='? Not sure
> > > > how to realize this via registers. Also undecided about '<' and '>' ops.
> > > 
> > > Whats the point?
> > 
> > IIRC, Pablo's demand for not aborting in nft_meta in case of
> > insufficient data was to insert a value into dreg which will never
> > match. I think the idea was to avoid accidental matching in situations
> > where a match doesn't make sense.
> 
> I think the only contraint is that it must not overlap with a
> legitimate ifindex.
> 
> But 0 cannot occur, so 'meta iif 0' will only match in case no input
> interface existed -- I think thats fine and might even be desired.

OK, so we just drop my patch to reject ifindex 0 from userspace to keep
fib working?

[...]
> I would propose to go with '0' dreg for ifindex, "" for name and leave
> rest as-is.

My kernel patch also changes iftype to set ARPHRD_VOID and ifkind to set
an empty string as well.

For iftype, I also sent a userspace patch to disallow ARPHRD_VOID. Do
you think it should be dropped as well?

Thanks, Phil

Re: [PATCH nft] doc: fib: explain example in more detail

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> On Mon, Jul 22, 2019 at 02:56:33PM +0200, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > On Mon, Jul 22, 2019 at 02:17:47PM +0200, Florian Westphal wrote:
> > > > Phil Sutter <phil@nwl.cc> wrote:
> > > > > use for "no data available" situations. This whole attempt feels a bit
> > > > > futile. Maybe we should introduce something to signal "no value" so that
> > > > > cmp expression will never match for '==' and always for '!='? Not sure
> > > > > how to realize this via registers. Also undecided about '<' and '>' ops.
> > > > 
> > > > Whats the point?
> > > 
> > > IIRC, Pablo's demand for not aborting in nft_meta in case of
> > > insufficient data was to insert a value into dreg which will never
> > > match. I think the idea was to avoid accidental matching in situations
> > > where a match doesn't make sense.
> > 
> > I think the only contraint is that it must not overlap with a
> > legitimate ifindex.
> > 
> > But 0 cannot occur, so 'meta iif 0' will only match in case no input
> > interface existed -- I think thats fine and might even be desired.
> 
> OK, so we just drop my patch to reject ifindex 0 from userspace to keep
> fib working?
> 
> [...]
> > I would propose to go with '0' dreg for ifindex, "" for name and leave
> > rest as-is.
> 
> My kernel patch also changes iftype to set ARPHRD_VOID and ifkind to set
> an empty string as well.

I would keep both with current semantics, i.e. 'break'/no match until we
get more evidence that we need this ARPHDR_VOID store.

For iifkind, I am not sure.  Perhaps leave it as-is?

Kernel doesn't allow "" iifname (so we can reuse it for 'no interface'),
but what about ifkind?

> For iftype, I also sent a userspace patch to disallow ARPHRD_VOID. Do
> you think it should be dropped as well?

I think we should leave userspace alone.

Re: [PATCH nft] doc: fib: explain example in more detail

[Phil Sutter]

On Mon, Jul 22, 2019 at 03:06:24PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > On Mon, Jul 22, 2019 at 02:56:33PM +0200, Florian Westphal wrote:
> > > Phil Sutter <phil@nwl.cc> wrote:
> > > > On Mon, Jul 22, 2019 at 02:17:47PM +0200, Florian Westphal wrote:
> > > > > Phil Sutter <phil@nwl.cc> wrote:
> > > > > > use for "no data available" situations. This whole attempt feels a bit
> > > > > > futile. Maybe we should introduce something to signal "no value" so that
> > > > > > cmp expression will never match for '==' and always for '!='? Not sure
> > > > > > how to realize this via registers. Also undecided about '<' and '>' ops.
> > > > > 
> > > > > Whats the point?
> > > > 
> > > > IIRC, Pablo's demand for not aborting in nft_meta in case of
> > > > insufficient data was to insert a value into dreg which will never
> > > > match. I think the idea was to avoid accidental matching in situations
> > > > where a match doesn't make sense.
> > > 
> > > I think the only contraint is that it must not overlap with a
> > > legitimate ifindex.
> > > 
> > > But 0 cannot occur, so 'meta iif 0' will only match in case no input
> > > interface existed -- I think thats fine and might even be desired.
> > 
> > OK, so we just drop my patch to reject ifindex 0 from userspace to keep
> > fib working?
> > 
> > [...]
> > > I would propose to go with '0' dreg for ifindex, "" for name and leave
> > > rest as-is.
> > 
> > My kernel patch also changes iftype to set ARPHRD_VOID and ifkind to set
> > an empty string as well.
> 
> I would keep both with current semantics, i.e. 'break'/no match until we
> get more evidence that we need this ARPHDR_VOID store.
> 
> For iifkind, I am not sure.  Perhaps leave it as-is?
> 
> Kernel doesn't allow "" iifname (so we can reuse it for 'no interface'),
> but what about ifkind?

Well, at least every implementation of rtnl_link_ops I found in current
kernel sources initializes 'kind' field but there's indeed no guarantee.

> > For iftype, I also sent a userspace patch to disallow ARPHRD_VOID. Do
> > you think it should be dropped as well?
> 
> I think we should leave userspace alone.

OK.

Thanks, Phil

Re: [PATCH nft] doc: fib: explain example in more detail

[Pablo Neira Ayuso]

On Mon, Jul 22, 2019 at 03:02:59PM +0200, Phil Sutter wrote:
> On Mon, Jul 22, 2019 at 02:56:33PM +0200, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > On Mon, Jul 22, 2019 at 02:17:47PM +0200, Florian Westphal wrote:
> > > > Phil Sutter <phil@nwl.cc> wrote:
> > > > > use for "no data available" situations. This whole attempt feels a bit
> > > > > futile. Maybe we should introduce something to signal "no value" so that
> > > > > cmp expression will never match for '==' and always for '!='? Not sure
> > > > > how to realize this via registers. Also undecided about '<' and '>' ops.
> > > > 
> > > > Whats the point?
> > > 
> > > IIRC, Pablo's demand for not aborting in nft_meta in case of
> > > insufficient data was to insert a value into dreg which will never
> > > match. I think the idea was to avoid accidental matching in situations
> > > where a match doesn't make sense.
> > 
> > I think the only contraint is that it must not overlap with a
> > legitimate ifindex.
> > 
> > But 0 cannot occur, so 'meta iif 0' will only match in case no input
> > interface existed -- I think thats fine and might even be desired.
> 
> OK, so we just drop my patch to reject ifindex 0 from userspace to keep
> fib working?

I think so, yes.

Re: [PATCH nft] doc: fib: explain example in more detail

[Pablo Neira Ayuso]

On Sun, Jul 21, 2019 at 12:43:05PM +0200, Florian Westphal wrote:
> As noted by Felix Dreissig, fib documentation is quite terse, so explain
> the 'saddr . iif' example with a few more words.
> 
> Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1220
> Signed-off-by: Florian Westphal <fw@strlen.de>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Thanks Florian.