[PATCH nf-next] netfilter: nft_fib: store loopback interface to dreg when rt is local

[PATCH nf-next] netfilter: nft_fib: store loopback interface to dreg when rt is local

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

In general, we haven't do routing lookup in PREROUTING hook, so it's
very likely that fib4/6_is_local will not be met. Then the *dest will
be set to 0 because we do nothing when the fib result is RTN_LOCAL.

So if the user want to drop all packets which cannot be routed,
and input the following nft rule:
  # nft add rule filter prerouting fib daddr oif eq 0 drop

Then all the packets which destinate to local will be dropped
incorrectly.

Fixes: f6d0cbcf09c5 ("netfilter: nf_tables: add fib expression")
Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 net/ipv4/netfilter/nft_fib_ipv4.c | 3 ++-
 net/ipv6/netfilter/nft_fib_ipv6.c | 8 ++++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/netfilter/nft_fib_ipv4.c b/net/ipv4/netfilter/nft_fib_ipv4.c
index 2581363..2107775 100644
--- a/net/ipv4/netfilter/nft_fib_ipv4.c
+++ b/net/ipv4/netfilter/nft_fib_ipv4.c
@@ -130,7 +130,8 @@ void nft_fib4_eval(const struct nft_expr *expr, struct nft_regs *regs,
 	switch (res.type) {
 	case RTN_UNICAST:
 		break;
-	case RTN_LOCAL:	/* should not appear here, see fib4_is_local() above */
+	case RTN_LOCAL:
+		nft_fib_store_result(dest, priv->result, pkt, LOOPBACK_IFINDEX);
 		return;
 	default:
 		break;
diff --git a/net/ipv6/netfilter/nft_fib_ipv6.c b/net/ipv6/netfilter/nft_fib_ipv6.c
index c947aad..5e2de1b 100644
--- a/net/ipv6/netfilter/nft_fib_ipv6.c
+++ b/net/ipv6/netfilter/nft_fib_ipv6.c
@@ -175,8 +175,12 @@ void nft_fib6_eval(const struct nft_expr *expr, struct nft_regs *regs,
 	if (rt->dst.error)
 		goto put_rt_err;
 
-	/* Should not see RTF_LOCAL here */
-	if (rt->rt6i_flags & (RTF_REJECT | RTF_ANYCAST | RTF_LOCAL))
+	if (rt->rt6i_flags & RTF_LOCAL) {
+		nft_fib_store_result(dest, priv->result, pkt, LOOPBACK_IFINDEX);
+		goto put_rt_err;
+	}
+
+	if (rt->rt6i_flags & (RTF_REJECT | RTF_ANYCAST))
 		goto put_rt_err;
 
 	if (oif && oif != rt->rt6i_idev->dev) {
-- 
2.5.5

Re: [PATCH nf-next] netfilter: nft_fib: store loopback interface to dreg when rt is local

[Florian Westphal]

Liping Zhang <zlpnobody@163.com> wrote:
> In general, we haven't do routing lookup in PREROUTING hook, so it's
> very likely that fib4/6_is_local will not be met.

loopback packets retain skb->dst (and thats what this test is about).

> Then the *dest will
> be set to 0 because we do nothing when the fib result is RTN_LOCAL.

Yes.

> So if the user want to drop all packets which cannot be routed,
> and input the following nft rule:
>   # nft add rule filter prerouting fib daddr oif eq 0 drop
> 
> Then all the packets which destinate to local will be dropped
> incorrectly.

but in "saddr oif eq 0 drop" case they really should have no oif, the
address should not be considered routeable.

Pablo, please don't apply this; I would like to look at this next week.

Msybe this needs a check if we're testing daddr or saddr.

Re: [PATCH nf-next] netfilter: nft_fib: store loopback interface to dreg when rt is local

[Liping Zhang]

Hi Florian,

At 2016-11-24 21:50:14, "Florian Westphal" <fw@strlen.de> wrote:
>Liping Zhang <zlpnobody@163.com> wrote:
>> In general, we haven't do routing lookup in PREROUTING hook, so it's
>> very likely that fib4/6_is_local will not be met.
>
>loopback packets retain skb->dst (and thats what this test is about).

Yes, so I use the words "very likely" :)
[...]
>but in "saddr oif eq 0 drop" case they really should have no oif, the
>address should not be considered routeable.

Yes, I read the ipt_rpfilter.c's source codes, and I find that there's a test flag
XT_RPFILTER_ACCEPT_LOCAL, so I guess your initial intention is (just my
guess, maybe I'm wrong):
   0 - no route
   1 - local route
   others - routing oif 

>
>Pablo, please don't apply this; I would like to look at this next week.
>
>Msybe this needs a check if we're testing daddr or saddr.

Re: [PATCH nf-next] netfilter: nft_fib: store loopback interface to dreg when rt is local

[Florian Westphal]

Liping Zhang <zlpnobody@163.com> wrote:
> At 2016-11-24 21:50:14, "Florian Westphal" <fw@strlen.de> wrote:
> >Liping Zhang <zlpnobody@163.com> wrote:
> >> In general, we haven't do routing lookup in PREROUTING hook, so it's
> >> very likely that fib4/6_is_local will not be met.

[..]

> Yes, so I use the words "very likely" :)
> [...]
> >but in "saddr oif eq 0 drop" case they really should have no oif, the
> >address should not be considered routeable.
>
> Yes, I read the ipt_rpfilter.c's source codes, and I find that there's a test flag
> XT_RPFILTER_ACCEPT_LOCAL, so I guess your initial intention is (just my
> guess, maybe I'm wrong):
>    0 - no route
>    1 - local route
>    others - routing oif

Yes, thats right.

"1" should only appear if lookup-up address is configured on this machine.
For saddr, I don't think its good idea, because it will pass

oif ne 0 accept

For ACCEPT_LOCAL i think its easier to combine this with the addrtype
check of just add explicit accept rules that make it bypass nft_fib
rule.

What do you think?

I agree that for your prerouting daddr example 0 makes no sense and 1
would indeed be a better option.

Re: [PATCH nf-next] netfilter: nft_fib: store loopback interface to dreg when rt is local

[Liping Zhang]

At 2016-11-24 22:48:59, "Florian Westphal" <fw@strlen.de> wrote:
>Liping Zhang <zlpnobody@163.com> wrote:
[...]
>"1" should only appear if lookup-up address is configured on this machine.
>For saddr, I don't think its good idea, because it will pass
>
>oif ne 0 accept

Yes, my patch will break this. 

>
>For ACCEPT_LOCAL i think its easier to combine this with the addrtype
>check of just add explicit accept rules that make it bypass nft_fib
>rule.

Yes, combine this with addrtype will be easier. My first thought was that
we can also use "fib saddr oif eq 1" to simulate the ACCECPT_LOCAL, but
I'm wrong, it will become more complicated.

>
>What do you think?
>
>I agree that for your prerouting daddr example 0 makes no sense and 1
>would indeed be a better option.
>

Re: [PATCH nf-next] netfilter: nft_fib: store loopback interface to dreg when rt is local

[Florian Westphal]

Liping Zhang <zlpnobody@163.com> wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> In general, we haven't do routing lookup in PREROUTING hook, so it's
> very likely that fib4/6_is_local will not be met. Then the *dest will
> be set to 0 because we do nothing when the fib result is RTN_LOCAL.
> 
> So if the user want to drop all packets which cannot be routed,
> and input the following nft rule:
>   # nft add rule filter prerouting fib daddr oif eq 0 drop
> 
> Then all the packets which destinate to local will be dropped
> incorrectly.
> 
> Fixes: f6d0cbcf09c5 ("netfilter: nf_tables: add fib expression")
> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
> ---
>  net/ipv4/netfilter/nft_fib_ipv4.c | 3 ++-
>  net/ipv6/netfilter/nft_fib_ipv6.c | 8 ++++++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv4/netfilter/nft_fib_ipv4.c b/net/ipv4/netfilter/nft_fib_ipv4.c
> index 2581363..2107775 100644
> --- a/net/ipv4/netfilter/nft_fib_ipv4.c
> +++ b/net/ipv4/netfilter/nft_fib_ipv4.c
> @@ -130,7 +130,8 @@ void nft_fib4_eval(const struct nft_expr *expr, struct nft_regs *regs,
>  	switch (res.type) {
>  	case RTN_UNICAST:
>  		break;
> -	case RTN_LOCAL:	/* should not appear here, see fib4_is_local() above */
> +	case RTN_LOCAL:
> +		nft_fib_store_result(dest, priv->result, pkt, LOOPBACK_IFINDEX);

Liping, what about doing:

	case RTN_LOCAL:
		if (priv->flags & NFTA_FIB_F_DADDR)
			nft_fib_store_result(dest, priv->result, pkt, LOOPBACK_IFINDEX);


AFAICS this will make above rule work while the saddr test will
still appear to not have a route at all.

What do you think?

Re: [PATCH nf-next] netfilter: nft_fib: store loopback interface to dreg when rt is local

[Liping Zhang]

Hi Florian,

2016-11-28 20:25 GMT+08:00 Florian Westphal <fw@strlen.de>:
[...]
>> diff --git a/net/ipv4/netfilter/nft_fib_ipv4.c b/net/ipv4/netfilter/nft_fib_ipv4.c
>> index 2581363..2107775 100644
>> --- a/net/ipv4/netfilter/nft_fib_ipv4.c
>> +++ b/net/ipv4/netfilter/nft_fib_ipv4.c
>> @@ -130,7 +130,8 @@ void nft_fib4_eval(const struct nft_expr *expr, struct nft_regs *regs,
>>       switch (res.type) {
>>       case RTN_UNICAST:
>>               break;
>> -     case RTN_LOCAL: /* should not appear here, see fib4_is_local() above */
>> +     case RTN_LOCAL:
>> +             nft_fib_store_result(dest, priv->result, pkt, LOOPBACK_IFINDEX);
>
> Liping, what about doing:
>
>         case RTN_LOCAL:
>                 if (priv->flags & NFTA_FIB_F_DADDR)
>                         nft_fib_store_result(dest, priv->result, pkt, LOOPBACK_IFINDEX);
>
>
> AFAICS this will make above rule work while the saddr test will
> still appear to not have a route at all.
>
> What do you think?

Yes, this will work both for *rpfilter* and my user case.
It seems a little ugly but I cannot find a better solution now ...