[PATCH net] ipv4/route: do not check saddr dev if iif is LOOPBACK_IFINDEX

[PATCH net] ipv4/route: do not check saddr dev if iif is LOOPBACK_IFINDEX

[Hangbin Liu]

Jianlin reported a bug that for IPv4, ip route get from src_addr would fail
if src_addr is not an address on local system.

\# ip route get 1.1.1.1 from 2.2.2.2
RTNETLINK answers: Invalid argument

While IPv6 would get the default route

\# ip -6 route get 1111::1 from 2222::2
1111::1 from 2222::2 via fe80:52:0:4982::1fe dev eth0 proto ra src \
	2620::2cf:e2ff:fe40:37 metric 1024 hoplimit 64 pref medium

Fix it by disabling the __ip_dev_find() check if iif is LOOPBACK_IFINDEX.

Reported-by: Jianlin Shi <jishi@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv4/route.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 517300d587a7..1be78e8f9e3c 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2512,7 +2512,8 @@ struct rtable *ip_route_output_key_hash_rcu(struct net *net, struct flowi4 *fl4,
 			goto make_route;
 		}
 
-		if (!(fl4->flowi4_flags & FLOWI_FLAG_ANYSRC)) {
+		if (!(fl4->flowi4_flags & FLOWI_FLAG_ANYSRC) &&
+		    fl4->flowi4_iif != LOOPBACK_IFINDEX) {
 			/* It is equivalent to inet_addr_type(saddr) == RTN_LOCAL */
 			if (!__ip_dev_find(net, fl4->saddr, false))
 				goto out;
-- 
2.19.2

Re: [PATCH net] ipv4/route: do not check saddr dev if iif is LOOPBACK_IFINDEX

[David Ahern]

On 8/1/19 2:29 AM, Hangbin Liu wrote:
> Jianlin reported a bug that for IPv4, ip route get from src_addr would fail
> if src_addr is not an address on local system.
> 
> \# ip route get 1.1.1.1 from 2.2.2.2
> RTNETLINK answers: Invalid argument

so this is a forwarding lookup in which case iif should be set. Based on
the above 'route get' inet_rtm_getroute is doing a lookup as if it is
locally generated traffic.

Re: [PATCH net] ipv4/route: do not check saddr dev if iif is LOOPBACK_IFINDEX

[Hangbin Liu]

On Thu, Aug 01, 2019 at 01:51:25PM -0600, David Ahern wrote:
> On 8/1/19 2:29 AM, Hangbin Liu wrote:
> > Jianlin reported a bug that for IPv4, ip route get from src_addr would fail
> > if src_addr is not an address on local system.
> > 
> > \# ip route get 1.1.1.1 from 2.2.2.2
> > RTNETLINK answers: Invalid argument
> 
> so this is a forwarding lookup in which case iif should be set. Based on

with out setting iif in userspace, the kernel set iif to lo by default.

> the above 'route get' inet_rtm_getroute is doing a lookup as if it is
> locally generated traffic.

yeah... but what about the IPv6 part. That cause a different behavior in
userspace.

Thanks
Hangbin

Re: [PATCH net] ipv4/route: do not check saddr dev if iif is LOOPBACK_IFINDEX

[David Ahern]

On 8/1/19 10:13 PM, Hangbin Liu wrote:
> On Thu, Aug 01, 2019 at 01:51:25PM -0600, David Ahern wrote:
>> On 8/1/19 2:29 AM, Hangbin Liu wrote:
>>> Jianlin reported a bug that for IPv4, ip route get from src_addr would fail
>>> if src_addr is not an address on local system.
>>>
>>> \# ip route get 1.1.1.1 from 2.2.2.2
>>> RTNETLINK answers: Invalid argument
>>
>> so this is a forwarding lookup in which case iif should be set. Based on
> 
> with out setting iif in userspace, the kernel set iif to lo by default.

right, it presumes locally generated traffic.
> 
>> the above 'route get' inet_rtm_getroute is doing a lookup as if it is
>> locally generated traffic.
> 
> yeah... but what about the IPv6 part. That cause a different behavior in
> userspace.

just one of many, many annoying differences between v4 and v6. We could
try to catalog it.

Re: [PATCH net] ipv4/route: do not check saddr dev if iif is LOOPBACK_IFINDEX

[Stefano Brivio]

David,

On Thu, 1 Aug 2019 13:51:25 -0600
David Ahern <dsahern@gmail.com> wrote:

> On 8/1/19 2:29 AM, Hangbin Liu wrote:
> > Jianlin reported a bug that for IPv4, ip route get from src_addr would fail
> > if src_addr is not an address on local system.
> > 
> > \# ip route get 1.1.1.1 from 2.2.2.2
> > RTNETLINK answers: Invalid argument  
> 
> so this is a forwarding lookup in which case iif should be set.

On actual forwarding, yes, it will be set.

But if we are just doing a lookup for a route (iif is
LOOPBACK_IFINDEX), I think this should still give us the matching route,
which is what IPv6 already does and what this patch fixes for IPv4.

Otherwise, we have no way to fetch that route, no matter if source
routing is configured. So I think this patch is correct and to some
extent necessary.

-- 
Stefano

Re: [PATCH net] ipv4/route: do not check saddr dev if iif is LOOPBACK_IFINDEX

[David Miller]

From: David Ahern <dsahern@gmail.com>
Date: Thu, 1 Aug 2019 22:16:00 -0600

> On 8/1/19 10:13 PM, Hangbin Liu wrote:
>> On Thu, Aug 01, 2019 at 01:51:25PM -0600, David Ahern wrote:
>>> On 8/1/19 2:29 AM, Hangbin Liu wrote:
>>>> Jianlin reported a bug that for IPv4, ip route get from src_addr would fail
>>>> if src_addr is not an address on local system.
>>>>
>>>> \# ip route get 1.1.1.1 from 2.2.2.2
>>>> RTNETLINK answers: Invalid argument
>>>
>>> so this is a forwarding lookup in which case iif should be set. Based on
>> 
>> with out setting iif in userspace, the kernel set iif to lo by default.
> 
> right, it presumes locally generated traffic.
>> 
>>> the above 'route get' inet_rtm_getroute is doing a lookup as if it is
>>> locally generated traffic.
>> 
>> yeah... but what about the IPv6 part. That cause a different behavior in
>> userspace.
> 
> just one of many, many annoying differences between v4 and v6. We could
> try to catalog it.

I think we just have to accept this difference because this change would
change behavior for all route lookups, not just those done by ip route get.

Re: [PATCH net] ipv4/route: do not check saddr dev if iif is LOOPBACK_IFINDEX

[Stefano Brivio]

On Sun, 11 Aug 2019 20:49:18 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: David Ahern <dsahern@gmail.com>
> Date: Thu, 1 Aug 2019 22:16:00 -0600
> 
> > On 8/1/19 10:13 PM, Hangbin Liu wrote:  
> >> On Thu, Aug 01, 2019 at 01:51:25PM -0600, David Ahern wrote:  
> >>> On 8/1/19 2:29 AM, Hangbin Liu wrote:  
> >>>> Jianlin reported a bug that for IPv4, ip route get from src_addr would fail
> >>>> if src_addr is not an address on local system.
> >>>>
> >>>> \# ip route get 1.1.1.1 from 2.2.2.2
> >>>> RTNETLINK answers: Invalid argument  
> >>>
> >>> so this is a forwarding lookup in which case iif should be set. Based on  
> >> 
> >> with out setting iif in userspace, the kernel set iif to lo by default.  
> > 
> > right, it presumes locally generated traffic.  
> >>   
> >>> the above 'route get' inet_rtm_getroute is doing a lookup as if it is
> >>> locally generated traffic.  
> >> 
> >> yeah... but what about the IPv6 part. That cause a different behavior in
> >> userspace.  
> > 
> > just one of many, many annoying differences between v4 and v6. We could
> > try to catalog it.  
> 
> I think we just have to accept this difference because this change would
> change behavior for all route lookups, not just those done by ip route get.

How so, actually? I don't see how that would happen. On the forwarding
path, 'iif' is set (not to loopback interface), so that's not affected.

Is there any other route lookup possibility I'm missing?

-- 
Stefano

Re: [PATCH net] ipv4/route: do not check saddr dev if iif is LOOPBACK_IFINDEX

[David Ahern]

On 8/12/19 4:58 PM, Stefano Brivio wrote:
> How so, actually? I don't see how that would happen. On the forwarding
> path, 'iif' is set (not to loopback interface), so that's not affected.
> 
> Is there any other route lookup possibility I'm missing?

Use case is saddr is set and FLOWI_FLAG_ANYSRC is not set and that seems
pretty common to me. From a quick look, icmp_route_lookup,
ipv4_update_pmtu, ipv4_redirect, inet_csk_route_req, ...

Enable trace_fib_table_lookup and look at the flags for various use cases.