[Patch net] fib: relax source validation check for loopback packets

[Patch net] fib: relax source validation check for loopback packets

[Cong Wang]

In a rare case where we redirect local packets from veth to lo,
these packets fail to pass the source validation when rp_filter
is turned on, as the tracing shows:

  <...>-311708 [040] ..s1 7951180.957825: fib_table_lookup: table 254 oif 0 iif 1 src 10.53.180.130 dst 10.53.180.130 tos 0 scope 0 flags 0
  <...>-311708 [040] ..s1 7951180.957826: fib_table_lookup_nh: nexthop dev eth0 oif 4 src 10.53.180.130

So, the fib table lookup returns eth0 as the nexthop even though
the packets are local and should be routed to loopback nonetheless,
but they can't pass the dev match check in fib_info_nh_uses_dev().

It should be safe to relax this check for this special case, as
normally packets coming out of loopback device still have skb_dst
so they won't even hit this slow path.

Cc: Julian Anastasov <ja@ssi.bg>
Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/ipv4/fib_frontend.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 317339cd7f03..8662a44a28f9 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -388,6 +388,12 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
 	fib_combine_itag(itag, &res);
 
 	dev_match = fib_info_nh_uses_dev(res.fi, dev);
+	/* This is rare, loopback packets retain skb_dst so normally they
+	 * would not even hit this slow path.
+	 */
+	dev_match = dev_match || (res.type == RTN_LOCAL &&
+				  dev == net->loopback_dev &&
+				  IN_DEV_ACCEPT_LOCAL(idev));
 	if (dev_match) {
 		ret = FIB_RES_NHC(res)->nhc_scope >= RT_SCOPE_HOST;
 		return ret;
-- 
2.21.0

Re: [Patch net] fib: relax source validation check for loopback packets

[David Ahern]

On 7/12/19 2:17 PM, Cong Wang wrote:
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index 317339cd7f03..8662a44a28f9 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -388,6 +388,12 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
>  	fib_combine_itag(itag, &res);
>  
>  	dev_match = fib_info_nh_uses_dev(res.fi, dev);
> +	/* This is rare, loopback packets retain skb_dst so normally they
> +	 * would not even hit this slow path.
> +	 */
> +	dev_match = dev_match || (res.type == RTN_LOCAL &&
> +				  dev == net->loopback_dev &&

The dev should not be needed. res.type == RTN_LOCAL should be enough, no?

> +				  IN_DEV_ACCEPT_LOCAL(idev));

Why is this check needed? Can you give an example use that is fixed -
and add one to selftests/net/fib_tests.sh?


>  	if (dev_match) {
>  		ret = FIB_RES_NHC(res)->nhc_scope >= RT_SCOPE_HOST;
>  		return ret;
> 

Re: [Patch net] fib: relax source validation check for loopback packets

[David Ahern]

On 7/13/19 4:42 PM, David Ahern wrote:
> On 7/12/19 2:17 PM, Cong Wang wrote:
>> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
>> index 317339cd7f03..8662a44a28f9 100644
>> --- a/net/ipv4/fib_frontend.c
>> +++ b/net/ipv4/fib_frontend.c
>> @@ -388,6 +388,12 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
>>  	fib_combine_itag(itag, &res);
>>  
>>  	dev_match = fib_info_nh_uses_dev(res.fi, dev);
>> +	/* This is rare, loopback packets retain skb_dst so normally they
>> +	 * would not even hit this slow path.
>> +	 */
>> +	dev_match = dev_match || (res.type == RTN_LOCAL &&
>> +				  dev == net->loopback_dev &&
> 
> The dev should not be needed. res.type == RTN_LOCAL should be enough, no?
> 

nevermind, I see why you have the dev check.

Re: [Patch net] fib: relax source validation check for loopback packets

[Cong Wang]

On Sat, Jul 13, 2019 at 3:42 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 7/12/19 2:17 PM, Cong Wang wrote:
> > diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> > index 317339cd7f03..8662a44a28f9 100644
> > --- a/net/ipv4/fib_frontend.c
> > +++ b/net/ipv4/fib_frontend.c
> > @@ -388,6 +388,12 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
> >       fib_combine_itag(itag, &res);
> >
> >       dev_match = fib_info_nh_uses_dev(res.fi, dev);
> > +     /* This is rare, loopback packets retain skb_dst so normally they
> > +      * would not even hit this slow path.
> > +      */
> > +     dev_match = dev_match || (res.type == RTN_LOCAL &&
> > +                               dev == net->loopback_dev &&
>
> The dev should not be needed. res.type == RTN_LOCAL should be enough, no?
>
> > +                               IN_DEV_ACCEPT_LOCAL(idev));
>
> Why is this check needed? Can you give an example use that is fixed -

I am not sure if I should have this check either, my initial version didn't
have it either, later I add it because I find out it is checked for rp_filter=0
case too.

On the other hand, loopback always accepts local traffic, so it may be
redundant to check it. So, I am not sure.

What do you think?

> and add one to selftests/net/fib_tests.sh?

It's complicated, Mesos network isolation uses this case:
https://cgit.twitter.biz/mesos/tree/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp

Even if I use a simplified case, it still has to use TC filters and mirred
action to redirect the packet, which I am not sure they fit in fib_tests.sh.

Thanks.

Re: [Patch net] fib: relax source validation check for loopback packets

[Cong Wang]

On Sat, Jul 13, 2019 at 11:30 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> It's complicated, Mesos network isolation uses this case:
> https://cgit.twitter.biz/mesos/tree/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp

Oops, please use the open source link instead:
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp