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

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

[Paolo Abeni]

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

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@redhat.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.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 */
-- 
2.21.0

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

[David Ahern]

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@redhat.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.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@gmail.com>

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

[Paolo Abeni]

On Thu, 2019-10-24 at 09:50 -0600, David Ahern wrote:
> 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.

Do you prefer a net-next patch for that, or a repost on -net with a
separate patch for the self-test appended?

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

Just tested, it works without issue, It looks like IPv6 has not special
handing connected route with peers/128 bit masks.

Thank you,

Paolo

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

[David Ahern]

On 10/25/19 4:24 AM, Paolo Abeni wrote:
> On Thu, 2019-10-24 at 09:50 -0600, David Ahern wrote:
>> 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.
> 
> Do you prefer a net-next patch for that, or a repost on -net with a
> separate patch for the self-test appended?

As I recall I added the test cases when I added the feature. IMHO, it
would be best to add the new tests with the bug fix.

> 
>> Also, does a similar sequence for IPv6 work as expected?
> 
> Just tested, it works without issue, It looks like IPv6 has not special
> handing connected route with peers/128 bit masks.
> 

thanks for checking.