Re: [REGRESSION] connection timeout with routes to VRF

[David Ahern <dsahern@kernel.org>]

On 6/11/22 5:14 AM, Jan Luebbe wrote:
> Hi,
> 
> TL;DR: We think we have found a regression in the handling of VRF route leaking
> caused by "net: allow binding socket in a VRF when there's an unbound socket"
> (3c82a21f4320).

This is the 3rd report in the past few months about this commit.

...

> 
> Our minimized test case looks like this:
>  ip rule add pref 32765 from all lookup local
>  ip rule del pref 0 from all lookup local
>  ip link add red type vrf table 1000
>  ip link set red up
>  ip route add vrf red unreachable default metric 8192
>  ip addr add dev red 172.16.0.1/24
>  ip route add 172.16.0.0/24 dev red
>  ip vrf exec red socat -dd TCP-LISTEN:1234,reuseaddr,fork SYSTEM:"echo connected" &
>  sleep 1
>  nc 172.16.0.1 1234 < /dev/null
> 

...
Thanks for the detailed analysis and reproducer.

> 
> The partial revert
> diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
> index 98e1ec1a14f0..41e7f20d7e51 100644
> --- a/include/net/inet_hashtables.h
> +++ b/include/net/inet_hashtables.h
> @@ -310,8 +310,9 @@ static inline struct sock *inet_lookup_listener(struct net *net,
>  #define INET_MATCH(__sk, __net, __cookie, __saddr, __daddr, __ports, __dif, __sdif) \
>         (((__sk)->sk_portpair == (__ports))                     &&      \
>          ((__sk)->sk_addrpair == (__cookie))                    &&      \
> -        (((__sk)->sk_bound_dev_if == (__dif))                  ||      \
> -         ((__sk)->sk_bound_dev_if == (__sdif)))                &&      \
> +        (!(__sk)->sk_bound_dev_if      ||                              \
> +          ((__sk)->sk_bound_dev_if == (__dif))                 ||      \
> +          ((__sk)->sk_bound_dev_if == (__sdif)))               &&      \
>          net_eq(sock_net(__sk), (__net)))
>  #else /* 32-bit arch */
>  #define INET_ADDR_COOKIE(__name, __saddr, __daddr) \
> @@ -321,8 +322,9 @@ static inline struct sock *inet_lookup_listener(struct net *net,
>         (((__sk)->sk_portpair == (__ports))             &&              \
>          ((__sk)->sk_daddr      == (__saddr))           &&              \
>          ((__sk)->sk_rcv_saddr  == (__daddr))           &&              \
> -        (((__sk)->sk_bound_dev_if == (__dif))          ||              \
> -         ((__sk)->sk_bound_dev_if == (__sdif)))        &&              \
> +        (!(__sk)->sk_bound_dev_if      ||                              \
> +          ((__sk)->sk_bound_dev_if == (__dif))         ||              \
> +          ((__sk)->sk_bound_dev_if == (__sdif)))       &&              \
>          net_eq(sock_net(__sk), (__net)))
>  #endif /* 64-bit arch */
> 
> restores the original behavior when applied on v5.18. This doesn't apply
> directly on master, as the macro was replaced by an inline function in "inet:
> add READ_ONCE(sk->sk_bound_dev_if) in INET_MATCH()" (4915d50e300e).
> 
> I have to admit I don't quite understand 3c82a21f4320, so I'm not sure how to
> proceed. What would be broken by the partial revert above? Are there better ways
> to configure routing into the VRF than simply "ip route add 172.16.0.0/24 dev
> red" that still work?
> 
> Thanks,
> Jan
> 
> #regzbot introduced: 3c82a21f4320
> 
> 
> 

Andy Roulin suggested the same fix to the same problem a few weeks back.
Let's do it along with a test case in fcnl-test.sh which covers all of
these vrf permutations.