* [PATCH net] ipv4: fix route update on metric change.
@ 2019-10-24 9:19 Paolo Abeni
2019-10-24 15:50 ` David Ahern
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Abeni @ 2019-10-24 9:19 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, Beniamino Galvani, David S. Miller
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net] ipv4: fix route update on metric change.
2019-10-24 9:19 [PATCH net] ipv4: fix route update on metric change Paolo Abeni
@ 2019-10-24 15:50 ` David Ahern
2019-10-25 10:24 ` Paolo Abeni
0 siblings, 1 reply; 4+ messages in thread
From: David Ahern @ 2019-10-24 15:50 UTC (permalink / raw)
To: Paolo Abeni, netdev; +Cc: Beniamino Galvani, David S. Miller
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>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] ipv4: fix route update on metric change.
2019-10-24 15:50 ` David Ahern
@ 2019-10-25 10:24 ` Paolo Abeni
2019-10-25 14:33 ` David Ahern
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Abeni @ 2019-10-25 10:24 UTC (permalink / raw)
To: David Ahern, netdev; +Cc: Beniamino Galvani, David S. Miller
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] ipv4: fix route update on metric change.
2019-10-25 10:24 ` Paolo Abeni
@ 2019-10-25 14:33 ` David Ahern
0 siblings, 0 replies; 4+ messages in thread
From: David Ahern @ 2019-10-25 14:33 UTC (permalink / raw)
To: Paolo Abeni, netdev; +Cc: Beniamino Galvani, David S. Miller
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-25 14:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-10-24 9:19 [PATCH net] ipv4: fix route update on metric change Paolo Abeni
2019-10-24 15:50 ` David Ahern
2019-10-25 10:24 ` Paolo Abeni
2019-10-25 14:33 ` David Ahern
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).