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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YjmVZzwE3XY750v6@shredder>
Date:   Tue, 22 Mar 2022 11:22:47 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     David Ahern <dsahern@...nel.org>
Cc:     netdev@...r.kernel.org, kuba@...nel.org, davem@...emloft.net,
        greearb@...delatech.com
Subject: Re: [PATCH net-next] net: Add l3mdev index to flow struct and avoid
 oif reset for port devices

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.