Date: Thu, 31 Aug 2023 13:58:48 +0200
From: Nicolas Dichtel <nicolas.dichtel@...nd.com>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: David Ahern <dsahern@...nel.org>, netdev@...r.kernel.org,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Ido Schimmel <idosch@...sch.org>,
 Thomas Haller <thaller@...hat.com>
Subject: Re: [PATCH net-next] ipv6: do not merge differe type and protocol
 routes

Le 31/08/2023 à 12:14, Hangbin Liu a écrit :
> Hi Nicolas,
> On Thu, Aug 31, 2023 at 10:17:19AM +0200, Nicolas Dichtel wrote:
>>>>> So let's skip counting the different type and protocol routes as siblings.
>>>>> After update, the different type/protocol routes will not be merged.
>>>>>
>>>>> + ip -6 route show table 100
>>>>> local 2001:db8:103::/64 via 2001:db8:101::10 dev dummy1 metric 1024 pref medium
>>>>> 2001:db8:103::/64 via 2001:db8:101::10 dev dummy2 metric 1024 pref medium
>>>>>
>>>>> + ip -6 route show table 200
>>>>> 2001:db8:104::/64 via 2001:db8:101::10 dev dummy1 proto kernel metric 1024 pref medium
>>>>> 2001:db8:104::/64 via 2001:db8:101::10 dev dummy2 proto bgp metric 1024 pref medium
>>>>
>>>> This seems wrong. The goal of 'ip route append' is to add a next hop, not to
>>>> create a new route. Ok, it adds a new route if no route exists, but it seems
>>>> wrong to me to use it by default, instead of 'add', to make things work magically.
>>>
>>> Legacy API; nothing can be done about that (ie., that append makes a new
>>> route when none exists).
>>>
>>>>
>>>> It seems more correct to return an error in these cases, but this will change
>>>> the uapi and it may break existing setups.
>>>>
>>>> Before this patch, both next hops could be used by the kernel. After it, one
>>>> route will be ignored (the former or the last one?). This is confusing and also
>>>> seems wrong.
>>>
>>> Append should match all details of a route to add to an existing entry
>>> and make it multipath. If there is a difference (especially the type -
>>> protocol difference is arguable) in attributes, then they are different
>>> routes.
>>>
>>
>> As you said, the protocol difference is arguable. It's not a property of the
>> route, just a hint.
>> I think the 'append' should match a route whatever the protocol is.
>> 'ip route change' for example does not use the protocol to find the existing
>> route, it will update it:
>>
>> $ ip -6 route add 2003:1:2:3::/64 via 2001::2 dev eth1
>> $ ip -6 route
>> 2003:1:2:3::/64 via 2001::2 dev eth1 metric 1024 pref medium
>> $ ip -6 route change 2003:1:2:3::/64 via 2001::2 dev eth1 protocol bgp
>> $ ip -6 route
>> 2003:1:2:3::/64 via 2001::2 dev eth1 proto bgp metric 1024 pref medium
>> $ ip -6 route change 2003:1:2:3::/64 via 2001::2 dev eth1 protocol kernel
>> $ ip -6 route
>> 2003:1:2:3::/64 via 2001::2 dev eth1 proto kernel metric 1024 pref medium
> 
> Not sure if I understand correctly, `ip route replace` should able to
> replace all other field other than dest and dev. It's for changing the route,
> not only nexthop.
>>
>> Why would 'append' selects route differently?
> 
> The append should also works for a single route, not only for append nexthop, no?
I don't think so. The 'append' should 'join', not add. Adding more cases where a
route is added instead of appended doesn't make the API clearer.

With this patch, it will be possible to add a new route with the 'append'
command when the 'add' command fails:
$ ip -6 route add local 2003:1:2:3::/64 via 2001::2 dev eth1 table 200
$ ip -6 route add unicast 2003:1:2:3::/64 via 2001::2 dev eth1 table 200
RTNETLINK answers: File exists

$ ip -6 route add 2003:1:2:3::/64 via 2001::2 dev eth1 protocol bgp table 200
$ ip -6 route add 2003:1:2:3::/64 via 2001::2 dev eth1 protocol kernel table 200
RTNETLINK answers: File exists

This makes the API more confusing and complex. And I don't understand how it
will be used later. There will be 2 routes on the system, but only one will be
used, which one? This is confusing.

> 
>>
>> This patch breaks the legacy API.
> 
> As the patch's description. Who would expect different type/protocol route
> should be merged as multipath route? I don't think the old API is correct.
The question is not 'who expect', but 'is there some systems somewhere that rely
on this (deliberately or not)'.
Frankly, the protocol is just informative, so I don't see why it is a problem to
ignore it with the 'append' command.
For the type, it is weird, for sure. Rejecting the command seems better than
duplicating routes. Which route is used by the stack?


Regards,
Nicolas

Powered by blists - more mailing lists