Re: [PATCH net-next] net: Add l3mdev index to flow struct and avoid oif reset for port devices

[Ido Schimmel <idosch@idosch.org>]

On Mon, Mar 14, 2022 at 02:45:51PM -0600, David Ahern wrote:
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index 2af2b99e0bea..fb0e49c36c2e 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -1429,11 +1429,8 @@ bool fib_lookup_good_nhc(const struct fib_nh_common *nhc, int fib_flags,
>  	    !(fib_flags & FIB_LOOKUP_IGNORE_LINKSTATE))
>  		return false;
>  
> -	if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) {
> -		if (flp->flowi4_oif &&
> -		    flp->flowi4_oif != nhc->nhc_oif)
> -			return false;
> -	}
> +	if (flp->flowi4_oif && flp->flowi4_oif != nhc->nhc_oif)
> +		return false;

David, we have several test cases that are failing which I have tracked
down to this patch.

Before the patch, if the original output interface was enslaved to a
VRF, the output interface in the flow struct would be updated to the VRF
and the 'FLOWI_FLAG_SKIP_NH_OIF' flag would be set, causing the above
check to be skipped.

After the patch, the check is no longer skipped, as original output
interface is retained and the flag was removed.

This breaks scenarios where a GRE tunnel specifies a dummy device
enslaved to a VRF as its physical device. The purpose of this
configuration is to redirect the underlay lookup to the table associated
with the VRF to which the dummy device is enslaved to. The check fails
because 'flp->flowi4_oif' points to the dummy device, whereas
'nhc->nhc_oif' points to the interface via which the encapsulated packet
should egress.

Skipping the check when an l3mdev was set seems to solve the problem:

diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index fb0e49c36c2e..cf1164e05d92 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -1429,7 +1429,8 @@ bool fib_lookup_good_nhc(const struct fib_nh_common *nhc, int fib_flags,
            !(fib_flags & FIB_LOOKUP_IGNORE_LINKSTATE))
                return false;
 
-       if (flp->flowi4_oif && flp->flowi4_oif != nhc->nhc_oif)
+       if (!flp->flowi4_l3mdev &&
+           flp->flowi4_oif && flp->flowi4_oif != nhc->nhc_oif)
                return false;
 
        return true;

AFAICT, this scenario does not break with ip6gre/ip6gretap tunnels
because 'RT6_LOOKUP_F_IFACE' is not set in
ip6_route_output_flags_noref() in this case.

WDYT? I plan to test this patch in our regression, but I'm not sure if I
missed other cases that might remain broken.