[RFC] nftables: reverse path filtering for nft

[RFC] nftables: reverse path filtering for nft

[Florian Westphal]

Hi.

Linux has a builtin rp filter for ipv4, but not for ipv6.
xtables has rpfilter match for both ipv4 and ipv6.
nftables currently does not have such a feature.

Any idea on how specific or generic this should be for nft?

Current idea is to add 'fib' expression that initially supports
lookup of outinterface index for reply direction, i.e.:

nft ... fib reply oif ne 0 accept (found something)
nft ... fib reply oif eq 0 drop   (no route)
nft ... fib reply oif eq eth0 (reply would be routed via eth0)

Problem is that we might need some options to influence/control
input to the fib lookup routines, e.g. if we want to consider
skb->mark or if we're only interested in routes via particular interface
(ipv6 needs this, this is what the --loose option does for -m rpfilter
in iptables).

Unfortunately, use of 'mark' results in grammar ambiguity in the parser.

What would work is this:

fib_expr                :       FIB     STRING  fib_args fib_type
                        {
                                $$ = fib_expr_alloc(&@$, $4, get_dir($2));
                        }
                        ;

fib_type                :       OIF     { $$ = NFT_FIB_OIF; }
                        ;

fib_args                :       fib_arg
                        {
                                $<expr>$        = $<expr>0;
                        }
                        |       fib_args        fib_arg
                        ;

fib_arg                 :       MARK
                        {
                                $<expr>0->fib.use_mark = 1;
                        }
                        |       LOOSE
                        {
                                $<expr>0->fib.loose = 1;
                        }
                        ;


Which results in following syntax:

nft .. fib reply mark loose oif eq 0 drop   # no route at all
nft .. fib reply mark oif eq 0 drop   # no route via iif
nft .. fib reply oif eq 0 drop   # no route via iif, do not use skb->mark


Other features that might make sense to implement for fib:
- query mtu on the route  (maybe useful with future tcp option mangling
to create TCPMSS target equvalent...?)

- query what fib says about type of saddr/daddr (iptables -m addrtype match)


Main 'problem' is that I don't want to muck with the syntax later so
it should be flexible enough to cover other uses beside rpf.

What do others think?

Re: [RFC] nftables: reverse path filtering for nft

[Pablo Neira Ayuso]

Hi Florian,

On Sat, Sep 10, 2016 at 10:01:02PM +0200, Florian Westphal wrote:
> Hi.
> 
> Linux has a builtin rp filter for ipv4, but not for ipv6.
> xtables has rpfilter match for both ipv4 and ipv6.
> nftables currently does not have such a feature.
> 
> Any idea on how specific or generic this should be for nft?
> 
> Current idea is to add 'fib' expression that initially supports
> lookup of outinterface index for reply direction, i.e.:
> 
> nft ... fib reply oif ne 0 accept (found something)

Probably use 'fib lookup' instead of 'fib reply'?

> nft ... fib reply oif eq 0 drop   (no route)

We need something similar for nft_exthdr in other to check for
presence of the extension header (ie. no need to access any extension
header field, just say it's there or not). So from the linearize path,
we can set on a NFT_EXTHDR_F_PRESENT to indicate that we only care for
its presence.

We would end up having a expression that returns a boolean or a value,
so the datatype needs to be attached in runtime.

Regarding syntax, it would be good to explore something like:

        nft ... fib lookup oif found accept
        nft ... fib lookup oif not found drop

And introduce a boolean datatype, only problem with this is that we'll
have an expression that returns boolean...

> nft ... fib reply oif eq eth0 (reply would be routed via eth0)

... or string in this case. I guess we can something like a dummy
datatype that becomes the real datatype during the evaluation.

> Problem is that we might need some options to influence/control
> input to the fib lookup routines, e.g. if we want to consider
> skb->mark or if we're only interested in routes via particular interface
> (ipv6 needs this, this is what the --loose option does for -m rpfilter
> in iptables).
> 
> Unfortunately, use of 'mark' results in grammar ambiguity in the parser.

What syntax would be causing the ambiguity? I guess this is related to
meta unqualified.

> What would work is this:
> 
> fib_expr                :       FIB     STRING  fib_args fib_type
>                         {
>                                 $$ = fib_expr_alloc(&@$, $4, get_dir($2));
>                         }
>                         ;
> 
> fib_type                :       OIF     { $$ = NFT_FIB_OIF; }
>                         ;
> 
> fib_args                :       fib_arg
>                         {
>                                 $<expr>$        = $<expr>0;
>                         }
>                         |       fib_args        fib_arg
>                         ;
> 
> fib_arg                 :       MARK
>                         {
>                                 $<expr>0->fib.use_mark = 1;
>                         }
>                         |       LOOSE
>                         {
>                                 $<expr>0->fib.loose = 1;
>                         }
>                         ;
> 
> 
> Which results in following syntax:
> 
> nft .. fib reply mark loose oif eq 0 drop   # no route at all
> nft .. fib reply mark oif eq 0 drop   # no route via iif
> nft .. fib reply oif eq 0 drop   # no route via iif, do not use skb->mark
> 
> 
> Other features that might make sense to implement for fib:
> - query mtu on the route  (maybe useful with future tcp option mangling
> to create TCPMSS target equvalent...?)
> 
> - query what fib says about type of saddr/daddr (iptables -m addrtype match)

Having addrtype into this generic expression looks very interesting.

> Main 'problem' is that I don't want to muck with the syntax later so
> it should be flexible enough to cover other uses beside rpf.

Agreed.

This fib stuff looks very cool.

Re: [RFC] nftables: reverse path filtering for nft

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sat, Sep 10, 2016 at 10:01:02PM +0200, Florian Westphal wrote:
> > Hi.
> > 
> > Linux has a builtin rp filter for ipv4, but not for ipv6.
> > xtables has rpfilter match for both ipv4 and ipv6.
> > nftables currently does not have such a feature.
> > 
> > Any idea on how specific or generic this should be for nft?
> > 
> > Current idea is to add 'fib' expression that initially supports
> > lookup of outinterface index for reply direction, i.e.:
> > 
> > nft ... fib reply oif ne 0 accept (found something)
> 
> Probably use 'fib lookup' instead of 'fib reply'?

I was thinking that we might want to support lookup in original
direction as well at some point, so 'fib original oif' would do
a route lookup for daddr (fib reply/rpf uses saddr).

> > nft ... fib reply oif eq 0 drop   (no route)
> 
> We need something similar for nft_exthdr in other to check for
> presence of the extension header (ie. no need to access any extension
> header field, just say it's there or not). So from the linearize path,
> we can set on a NFT_EXTHDR_F_PRESENT to indicate that we only care for
> its presence.
> 
> We would end up having a expression that returns a boolean or a value,
> so the datatype needs to be attached in runtime.

That doesn't seem too bad since kernel doesn't have to care
beyond making sure that the dreg is writeable (i.e., we don't need
a lot of extra code in kernel to make this work).

> Regarding syntax, it would be good to explore something like:
> 
>         nft ... fib lookup oif found accept
>         nft ... fib lookup oif not found drop

I dislike this, this seems to add expression specific negation
which i think is a big no-no.

> And introduce a boolean datatype, only problem with this is that we'll
> have an expression that returns boolean...

Ah, wait, you mean the found/not found is syntax sugar for
'oif != 0' and 'oif == 0'... ok, will think about this.

> > Problem is that we might need some options to influence/control
> > input to the fib lookup routines, e.g. if we want to consider
> > skb->mark or if we're only interested in routes via particular interface
> > (ipv6 needs this, this is what the --loose option does for -m rpfilter
> > in iptables).
> > 
> > Unfortunately, use of 'mark' results in grammar ambiguity in the parser.
> 
> What syntax would be causing the ambiguity? I guess this is related to
> meta unqualified.

Yes, it goes away when I remove MARK from meta_unqialified list.

> > What would work is this:
> > 
> > fib_expr                :       FIB     STRING  fib_args fib_type
> >                         {
> >                                 $$ = fib_expr_alloc(&@$, $4, get_dir($2));
> >                         }
> >                         ;
> > 
> > fib_type                :       OIF     { $$ = NFT_FIB_OIF; }
> >                         ;
> > 
> > fib_args                :       fib_arg
> >                         {
> >                                 $<expr>$        = $<expr>0;
> >                         }
> >                         |       fib_args        fib_arg
> >                         ;
> > 
> > fib_arg                 :       MARK
> >                         {
> >                                 $<expr>0->fib.use_mark = 1;
> >                         }
> >                         |       LOOSE
> >                         {
> >                                 $<expr>0->fib.loose = 1;
> >                         }
> >                         ;
> > 
> > 
> > Which results in following syntax:
> > 
> > nft .. fib reply mark loose oif eq 0 drop   # no route at all
> > nft .. fib reply mark oif eq 0 drop   # no route via iif
> > nft .. fib reply oif eq 0 drop   # no route via iif, do not use skb->mark
> > 
> > Other features that might make sense to implement for fib:
> > - query mtu on the route  (maybe useful with future tcp option mangling
> > to create TCPMSS target equvalent...?)
> > 
> > - query what fib says about type of saddr/daddr (iptables -m addrtype match)
> 
> Having addrtype into this generic expression looks very interesting.

Good, I will think about this some more so this can be added later
without syntax changes.

Re: [RFC] nftables: reverse path filtering for nft

[Pablo Neira Ayuso]

On Mon, Sep 12, 2016 at 02:21:07PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sat, Sep 10, 2016 at 10:01:02PM +0200, Florian Westphal wrote:
> > > Hi.
> > > 
> > > Linux has a builtin rp filter for ipv4, but not for ipv6.
> > > xtables has rpfilter match for both ipv4 and ipv6.
> > > nftables currently does not have such a feature.
> > > 
> > > Any idea on how specific or generic this should be for nft?
> > > 
> > > Current idea is to add 'fib' expression that initially supports
> > > lookup of outinterface index for reply direction, i.e.:
> > > 
> > > nft ... fib reply oif ne 0 accept (found something)
> > 
> > Probably use 'fib lookup' instead of 'fib reply'?
> 
> I was thinking that we might want to support lookup in original
> direction as well at some point, so 'fib original oif' would do
> a route lookup for daddr (fib reply/rpf uses saddr).

Then, I'd suggest:

        fib lookup ip daddr

to look up for the route base on the IPv4 destinarion address.

If you include the interface, you can express this through a
concatenation:

        fib lookup ip daddr . oif

As you are basically looking for the route based on IPv4 address and
the output interface, so this boils down to:

        fib lookup $expr $flags

Re: [RFC] nftables: reverse path filtering for nft

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>         fib lookup ip daddr . oif
> 
> As you are basically looking for the route based on IPv4 address and
> the output interface, so this boils down to:
> 
>         fib lookup $expr $flags

How would the kernel disentangle the register data?
(i.e., how do i know where in the sreg e.g. the daddr is
 that i need to stuff in the flowi struct?)

Re: [RFC] nftables: reverse path filtering for nft

[Pablo Neira Ayuso]

On Mon, Sep 12, 2016 at 09:00:25PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >         fib lookup ip daddr . oif
> > 
> > As you are basically looking for the route based on IPv4 address and
> > the output interface, so this boils down to:
> > 
> >         fib lookup $expr $flags
> 
> How would the kernel disentangle the register data?

What I'm proposing is to represent this as a concatenation, since this
represents the tuple that you use to look up for route.

> (i.e., how do i know where in the sreg e.g. the daddr is
>  that i need to stuff in the flowi struct?)

You can iterate over the concatenation compound from the
netlink_linearize path, it is just a list of expressions. Then, you
can set the NFTA_FIB_* netlink attribute using them.

>From the evaluation step, you would need to validate that the
expressions that we're using in the concatenation fit into flowi
struct, otherwise tell the user that they are not supported.

We cannot not actually support every tuple, as we are constrained by
flowi.

Re: [RFC] nftables: reverse path filtering for nft

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Sep 12, 2016 at 09:00:25PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >         fib lookup ip daddr . oif
> > > 
> > > As you are basically looking for the route based on IPv4 address and
> > > the output interface, so this boils down to:
> > > 
> > >         fib lookup $expr $flags
> > 
> > How would the kernel disentangle the register data?
> 
> What I'm proposing is to represent this as a concatenation, since this
> represents the tuple that you use to look up for route.
> 
> > (i.e., how do i know where in the sreg e.g. the daddr is
> >  that i need to stuff in the flowi struct?)
> 
> You can iterate over the concatenation compound from the
> netlink_linearize path, it is just a list of expressions. Then, you
> can set the NFTA_FIB_* netlink attribute using them.

Ah, ok.  Interesting idea, I'll give this a shot.

Re: [RFC] nftables: reverse path filtering for nft

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Mon, Sep 12, 2016 at 09:00:25PM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > >         fib lookup ip daddr . oif
> > > > 
> > > > As you are basically looking for the route based on IPv4 address and
> > > > the output interface, so this boils down to:
> > > > 
> > > >         fib lookup $expr $flags
> > > 
> > > How would the kernel disentangle the register data?
> > 
> > What I'm proposing is to represent this as a concatenation, since this
> > represents the tuple that you use to look up for route.
> > 
> > > (i.e., how do i know where in the sreg e.g. the daddr is
> > >  that i need to stuff in the flowi struct?)
> > 
> > You can iterate over the concatenation compound from the
> > netlink_linearize path, it is just a list of expressions. Then, you
> > can set the NFTA_FIB_* netlink attribute using them.
> 
> Ah, ok.  Interesting idea, I'll give this a shot.

Seems it might be better to handle this from the evaluation step
since it allows earlier error detection (e.g. use of
fib .. ip6 flowlabel from an ipv4 base for instance).

Re: [RFC] nftables: reverse path filtering for nft

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Sep 12, 2016 at 09:00:25PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >         fib lookup ip daddr . oif
> > > 
> > > As you are basically looking for the route based on IPv4 address and
> > > the output interface, so this boils down to:
> > > 
> > >         fib lookup $expr $flags
> > 
> > How would the kernel disentangle the register data?
> 
> What I'm proposing is to represent this as a concatenation, since this
> represents the tuple that you use to look up for route.
> 
> > (i.e., how do i know where in the sreg e.g. the daddr is
> >  that i need to stuff in the flowi struct?)
> 
> You can iterate over the concatenation compound from the
> netlink_linearize path, it is just a list of expressions. Then, you
> can set the NFTA_FIB_* netlink attribute using them.

I found this to be ugly and cumbersome, I'd propose following
syntax instead:

FIB     fib_type     fib_family   '{' fib_addr fib_key_flags '}'

The {} are needed because I'd like to use 'mark' and 'oif' in flags but
these can also be expressions, i.e. I need something that tells
the parser when end of FIB flags are reached (so instead of { }
it could also use single ';' or something else ...)

This gives following examples:

 fib oif { saddr }  # ip route get $saddr, place ifindex into register)
 fib oif { saddr mark,saddr,oif } # same, but populate flowi .saddr,mark,oif
  				    members as well

 fib oif { daddr mark,saddr,oif } # same, except that flowi.daddr is set
				  # to iph->daddr)

so fib_addr is the direction (if you ask for daddr, flowi.daddr is
set to iph->daddr, else iph->saddr).

If you specifiy oif flag, flowi.oif is set to skb->dev.
(Needed for ipv6 to allow strict filtering/ limit results).

Alternative would be to toss 'oif' flag name and use opposite
('loose'), like in iptables.

Yet another alternative would be to use the sysctl we have for this
(net.ipv4.conf.all.rp_filter) but I'd like to avoid it.

The family is optional, it would later allow to e.g. ask for
inet fib lookups from bridge family, and to ask for MAC FIB
lookups for bridge family ('fib oif bridge { saddr }').

If you prefer original "." for this it would be easy to change
but using "," seems more in line with other flag parameters
that nft has.

> From the evaluation step, you would need to validate that the
> expressions that we're using in the concatenation fit into flowi
> struct, otherwise tell the user that they are not supported.

That works (I have a patch) but it requires strcmp() voodoo to find
out what the user asked for initially.

For reference, above grammar looks like this:

fib_expr                :       FIB     fib_type        fib_family   '{' fib_addr       fib_key_flags '}'
                        {
                                $$ = fib_expr_alloc(&@$, $2, $3);
                                $$->fib.addr = $5;
                                $$->fib.flags = $6;
                        }
                        ;

fib_type                :       OIF     { $$ = NFT_FIBTYPE_OIF; }
                        ;

fib_addr                :       SADDR { $$ = IPHDR_SADDR; }
                        |       DADDR { $$ = IPHDR_DADDR; }
                        ;
fib_family              :       /* empty */             { $$ = PROTO_BASE_NETWORK_HDR; }
                        |       INET                    { $$ = PROTO_BASE_NETWORK_HDR; }
                        ;

fib_key_flag            :       MARK    { $$ = NFTA_FIB_F_MARK; }
                        |       SADDR   { $$ = NFTA_FIB_F_SADDR; }
                        |       DSCP    { $$ = NFTA_FIB_F_DSCP; }
                        |       OIF     { $$ = NFTA_FIB_F_OIF; }
                        |       { $$ = 0; }
                        ;

fib_key_flags           :       fib_key_flag
                        |       fib_key_flag COMMA fib_key_flags
                        {
                                $$ = $1 | $3;
                        }
                        ;

So parser handles everything and I don't have to throw 'unsupported expression'
errors from eval step either.

For -m addrtype support, this might look like this:
fib addrtype { saddr oif }

I think it can re-use all of this and I don't see any problems so far.

Let me know and I will start working on kernel side for this.

Re: [RFC] nftables: reverse path filtering for nft

[Arturo Borrero Gonzalez]

Hi Florian,

thanks for working on this, here my comments.

On 14 September 2016 at 19:45, Florian Westphal <fw@strlen.de> wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> On Mon, Sep 12, 2016 at 09:00:25PM +0200, Florian Westphal wrote:
>> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > >         fib lookup ip daddr . oif
>> > >
>> > > As you are basically looking for the route based on IPv4 address and
>> > > the output interface, so this boils down to:
>> > >
>> > >         fib lookup $expr $flags
>> >
>> > How would the kernel disentangle the register data?
>>
>> What I'm proposing is to represent this as a concatenation, since this
>> represents the tuple that you use to look up for route.
>>
>> > (i.e., how do i know where in the sreg e.g. the daddr is
>> >  that i need to stuff in the flowi struct?)
>>
>> You can iterate over the concatenation compound from the
>> netlink_linearize path, it is just a list of expressions. Then, you
>> can set the NFTA_FIB_* netlink attribute using them.
>
> I found this to be ugly and cumbersome, I'd propose following
> syntax instead:
>
> FIB     fib_type     fib_family   '{' fib_addr fib_key_flags '}'
>
> The {} are needed because I'd like to use 'mark' and 'oif' in flags but
> these can also be expressions, i.e. I need something that tells
> the parser when end of FIB flags are reached (so instead of { }
> it could also use single ';' or something else ...)
>
> This gives following examples:
>
>  fib oif { saddr }  # ip route get $saddr, place ifindex into register)
>  fib oif { saddr mark,saddr,oif } # same, but populate flowi .saddr,mark,oif
>                                     members as well
>
>  fib oif { daddr mark,saddr,oif } # same, except that flowi.daddr is set
>                                   # to iph->daddr)
>


Using {} in the syntax for something which is not a set or a map seems
a bit confusing to me.

I'm sorry I have no sensible alternatives ATM.

-- 
Arturo Borrero González

Re: [RFC] nftables: reverse path filtering for nft

[Florian Westphal]

Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote:
> Hi Florian,
> 
> thanks for working on this, here my comments.
> 
> On 14 September 2016 at 19:45, Florian Westphal <fw@strlen.de> wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> On Mon, Sep 12, 2016 at 09:00:25PM +0200, Florian Westphal wrote:
> >> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> > >         fib lookup ip daddr . oif
> >> > >
> >> > > As you are basically looking for the route based on IPv4 address and
> >> > > the output interface, so this boils down to:
> >> > >
> >> > >         fib lookup $expr $flags
> >> >
> >> > How would the kernel disentangle the register data?
> >>
> >> What I'm proposing is to represent this as a concatenation, since this
> >> represents the tuple that you use to look up for route.
> >>
> >> > (i.e., how do i know where in the sreg e.g. the daddr is
> >> >  that i need to stuff in the flowi struct?)
> >>
> >> You can iterate over the concatenation compound from the
> >> netlink_linearize path, it is just a list of expressions. Then, you
> >> can set the NFTA_FIB_* netlink attribute using them.
> >
> > I found this to be ugly and cumbersome, I'd propose following
> > syntax instead:
> >
> > FIB     fib_type     fib_family   '{' fib_addr fib_key_flags '}'
> >
> > The {} are needed because I'd like to use 'mark' and 'oif' in flags but
> > these can also be expressions, i.e. I need something that tells
> > the parser when end of FIB flags are reached (so instead of { }
> > it could also use single ';' or something else ...)
> >
> > This gives following examples:
> >
> >  fib oif { saddr }  # ip route get $saddr, place ifindex into register)
> >  fib oif { saddr mark,saddr,oif } # same, but populate flowi .saddr,mark,oif
> >                                     members as well
> >
> >  fib oif { daddr mark,saddr,oif } # same, except that flowi.daddr is set
> >                                   # to iph->daddr)
> >
> 
> 
> Using {} in the syntax for something which is not a set or a map seems
> a bit confusing to me.

We also use it for the flow statement, but I agree its not nice.

Other solution I see is to not use mark and oif and come up
with new/different keyword, but thats not good either.

Yet another option:

FIB     fib_type     fib_family   fib_key_flags	fib_addr

Which is not ambiguous anymore as either saddr or daddr will terminate
the statement.  We'd have to remove the saddr option but I don't think its a
problem (the iptables rpfilter modules set flowi.saddr if packet daddr
is unicast address).

Would give following syntax :

fib oif mark saddr
fib oif saddr
fib oif mark,oif daddr

fib addrtype oif  daddr

Or remove unqualified meta keywords, that should work as well.

Re: [RFC] nftables: reverse path filtering for nft

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Yet another option:
> 
> FIB     fib_type     fib_family   fib_key_flags	fib_addr
> 
> Which is not ambiguous anymore as either saddr or daddr will terminate
> the statement.  We'd have to remove the saddr option but I don't think its a
> problem (the iptables rpfilter modules set flowi.saddr if packet daddr
> is unicast address).
> 
> Would give following syntax :
> 
> fib oif mark saddr
> fib oif saddr
> fib oif mark,oif daddr

But also
fib oif oif saddr

Yuck.

Re: [RFC] nftables: reverse path filtering for nft

[Arturo Borrero Gonzalez]

On 14 September 2016 at 23:13, Florian Westphal <fw@strlen.de> wrote:
>
> Or remove unqualified meta keywords, that should work as well.

That was my first idea, but discarded it because it means breaking the syntax.

Not sure if it worth.
-- 
Arturo Borrero González

Re: [RFC] nftables: reverse path filtering for nft

[Arturo Borrero Gonzalez]

On 14 September 2016 at 23:13, Florian Westphal <fw@strlen.de> wrote:
>
> Other solution I see is to not use mark and oif and come up
> with new/different keyword, but thats not good either.
>

this solution is the less disruptive I think

-- 
Arturo Borrero González