netdev - Re: [PATCH net] ipv4: fix route update on metric change.

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: <a93347d4-b363-23c8-75e4-d5d0c8ad4592@gmail.com>
Date:   Thu, 24 Oct 2019 09:50:58 -0600
From:   David Ahern <dsahern@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc:     Beniamino Galvani <bgalvani@...hat.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net] ipv4: fix route update on metric change.

On 10/24/19 3:19 AM, Paolo Abeni wrote:
> Since commit af4d768ad28c ("net/ipv4: Add support for specifying metric
> of connected routes"), when updating an IP address with a different metric,
> the associated connected route is updated, too.
> 
> Still, the mentioned commit doesn't handle properly some corner cases:
> 
> $ ip addr add dev eth0 192.168.1.0/24
> $ ip addr add dev eth0 192.168.2.1/32 peer 192.168.2.2
> $ ip addr add dev eth0 192.168.3.1/24
> $ ip addr change dev eth0 192.168.1.0/24 metric 10
> $ ip addr change dev eth0 192.168.2.1/32 peer 192.168.2.2 metric 10
> $ ip addr change dev eth0 192.168.3.1/24 metric 10
> $ ip -4 route
> 192.168.1.0/24 dev eth0 proto kernel scope link src 192.168.1.0
> 192.168.2.2 dev eth0 proto kernel scope link src 192.168.2.1
> 192.168.3.0/24 dev eth0 proto kernel scope link src 192.168.2.1 metric 10

Please add this test and route checking to
tools/testing/selftests/net/fib_tests.sh. There is a
ipv4_addr_metric_test function that handles permutations and I guess the
above was missed.

Also, does a similar sequence for IPv6 work as expected?


> 
> Only the last route is correctly updated.
> 
> The problem is the current test in fib_modify_prefix_metric():
> 
> 	if (!(dev->flags & IFF_UP) ||
> 	    ifa->ifa_flags & (IFA_F_SECONDARY | IFA_F_NOPREFIXROUTE) ||
> 	    ipv4_is_zeronet(prefix) ||
> 	    prefix == ifa->ifa_local || ifa->ifa_prefixlen == 32)
> 
> Which should be the logical 'not' of the pre-existing test in
> fib_add_ifaddr():
> 
> 	if (!ipv4_is_zeronet(prefix) && !(ifa->ifa_flags & IFA_F_SECONDARY) &&
> 	    (prefix != addr || ifa->ifa_prefixlen < 32))
> 
> To properly negate the original expression, we need to change the last
> logical 'or' to a logical 'and'.
> 
> Fixes: af4d768ad28c ("net/ipv4: Add support for specifying metric of connected routes")
> Reported-and-suggested-by: Beniamino Galvani <bgalvani@...hat.com>
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> ---
>  net/ipv4/fib_frontend.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index dde77f72e03e..71c78d223dfd 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -1148,7 +1148,7 @@ void fib_modify_prefix_metric(struct in_ifaddr *ifa, u32 new_metric)
>  	if (!(dev->flags & IFF_UP) ||
>  	    ifa->ifa_flags & (IFA_F_SECONDARY | IFA_F_NOPREFIXROUTE) ||
>  	    ipv4_is_zeronet(prefix) ||
> -	    prefix == ifa->ifa_local || ifa->ifa_prefixlen == 32)
> +	    (prefix == ifa->ifa_local && ifa->ifa_prefixlen == 32))
>  		return;
>  
>  	/* add the new */
> 

Thanks for the patch

Reviewed-by: David Ahern <dsahern@...il.com>