[net] fib: relax source validation check for loopback packets
diff mbox series

Message ID 20190712201749.28421-2-xiyou.wangcong@gmail.com
State Superseded
Delegated to: David Miller
Headers show
Series
  • [net] fib: relax source validation check for loopback packets
Related show

Commit Message

Cong Wang July 12, 2019, 8:17 p.m. UTC
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(+)

Comments

David Ahern July 13, 2019, 10:42 p.m. UTC | #1
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;
>
David Ahern July 14, 2019, 12:29 a.m. UTC | #2
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.
Cong Wang July 14, 2019, 6:30 a.m. UTC | #3
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.
Cong Wang July 14, 2019, 6:33 a.m. UTC | #4
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

Patch
diff mbox series

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;