Re: [REGRESSION] connection timeout with routes to VRF

[Jan Luebbe <jluebbe@lasnet.de>]

On Sat, 2022-06-11 at 10:44 -0600, David Ahern wrote:
> 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.
> 
> ...

Hmm, I've not been able to find other reports. Could you point me to them?

> > 
> > 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.

Thanks! I'd be happy to test any patch in our real setup.

Regards,
Jan