Re: [PATCH v3 net-next 1/1] Allow user to set metric on default route learned via Router Advertisement.
> On Jan 23, 2021, at 5:13 PM, David Ahern wrote: > > On 1/23/21 1:00 PM, Jakub Kicinski wrote: >> On Fri, 22 Jan 2021 22:16:41 -0700 David Ahern wrote: >>> On 1/22/21 9:02 PM, Maciej Żenczykowski wrote: Why can't we get rid of the special case for 0 and simply make 1024 the default value? >>> >>> That would work too. >> >> Should we drop it then? Easier to bring it back than to change the >> interpretation later. It doesn't seem to serve any clear purpose right >> now. >> >> (Praveen if you post v4 please take a look at the checkpatch --strict >> warnings and address the ones which make sense, e.g. drop the brackets >> around comparisons, those are just noise, basic grasp of C operator >> precedence can be assumed in readers of kernel code). >> > > let's do a v4. > > Praveen: set the initial value to IP6_RT_PRIO_USER, do not allow 0, > remove the checks on value and don't forget to update documentation. > Sure, I will respin V4, with above mentioned changes. Also, I will address checkpatch --strict warnings. I wanted to set initial value to IP6_RT_PRIO_USER in v1, but avoided till review for 2 simple coding reasons: 1.) IP6_RT_PRIO_USER must be exposed in net/ipv6/addrconf.c by including include/uapi/linux/ipv6_route.h. 2.) If rt6_add_dflt_router() will be called from other files in future, IP6_RT_PRIO_USER should be included in all those files as well, because caller will pass most probably default value. > Oh and cc me on the next otherwise the review depends on me finding time > to scan netdev. Sure, I will cc you and will add “Reviewed by” as well. I will also send you the lkml link to v4. Thanks Jakub and you for reviewing this over the weekend.
Re: [PATCH v3 net-next 1/1] Allow user to set metric on default route learned via Router Advertisement.
On 1/23/21 1:00 PM, Jakub Kicinski wrote: > On Fri, 22 Jan 2021 22:16:41 -0700 David Ahern wrote: >> On 1/22/21 9:02 PM, Maciej Żenczykowski wrote: >>> Why can't we get rid of the special case for 0 and simply make 1024 the >>> default value? >> >> That would work too. > > Should we drop it then? Easier to bring it back than to change the > interpretation later. It doesn't seem to serve any clear purpose right > now. > > (Praveen if you post v4 please take a look at the checkpatch --strict > warnings and address the ones which make sense, e.g. drop the brackets > around comparisons, those are just noise, basic grasp of C operator > precedence can be assumed in readers of kernel code). > let's do a v4. Praveen: set the initial value to IP6_RT_PRIO_USER, do not allow 0, remove the checks on value and don't forget to update documentation. Oh and cc me on the next otherwise the review depends on me finding time to scan netdev.
Re: [PATCH v3 net-next 1/1] Allow user to set metric on default route learned via Router Advertisement.
On Fri, 22 Jan 2021 22:16:41 -0700 David Ahern wrote: > On 1/22/21 9:02 PM, Maciej Żenczykowski wrote: > > Why can't we get rid of the special case for 0 and simply make 1024 the > > default value? > > That would work too. Should we drop it then? Easier to bring it back than to change the interpretation later. It doesn't seem to serve any clear purpose right now. (Praveen if you post v4 please take a look at the checkpatch --strict warnings and address the ones which make sense, e.g. drop the brackets around comparisons, those are just noise, basic grasp of C operator precedence can be assumed in readers of kernel code).
Re: [PATCH v3 net-next 1/1] Allow user to set metric on default route learned via Router Advertisement.
On 1/22/21 9:02 PM, Maciej Żenczykowski wrote: > Why can't we get rid of the special case for 0 and simply make 1024 the > default value? That would work too. > > As for making it an RA option: it's not clear how that would work, the > use case I see for this is for example two connections to the internet, > of which one is clearly better (higher throughput, lower latency, lower > packet loss, etc) then the other. > > The upstream routers would have to somehow coordinate with each other > the metric values... that seems impossible to achieve in practice - > unless they do something like report expected down/up > bandwidth, latency, etc... While some sort of policy on the machine > itself seems much more feasible (for example wired interface > wireless > interface > cell interface or something like that) I was thinking the admin of the network controls the RAs and knows which paths are preferred over the admin of the node receiving the RA (not practical for a mobile setup with cell vs wifi, but is for a DC which is the driving use case). But it takes an extension to IPv6/ndisc to add metric as an RA option, so not realistic in a reasonable time frame.
Re: [PATCH v3 net-next 1/1] Allow user to set metric on default route learned via Router Advertisement.
On 1/19/21 2:29 PM, Praveen Chaudhary wrote: > For IPv4, default route is learned via DHCPv4 and user is allowed to change > metric using config etc/network/interfaces. But for IPv6, default route can > be learned via RA, for which, currently a fixed metric value 1024 is used. > > Ideally, user should be able to configure metric on default route for IPv6 > similar to IPv4. This fix adds sysctl for the same. > > Signed-off-by: Praveen Chaudhary > Signed-off-by: Zhenggen Xu > > Changes in v1. > 1.) Correct the call to rt6_add_dflt_router. > > Changes in v2. > 1.) Replace accept_ra_defrtr_metric to ra_defrtr_metric. > 2.) Change Type to __u32 instead of __s32. > 3.) Change description in Documentation/networking/ip-sysctl.rst. > 4.) Use proc_douintvec instead of proc_dointvec. > 5.) Code style in ndisc_router_discovery(). > 6.) Change Type to u32 instead of unsigned int. > > Changes in v3: > 1.) Removed '---' and '```' from description. > 2.) Remove stray ' after accept_ra_defrtr. > 3.) Fix tab in net/ipv6/addrconf.c. > > Logs: > > For IPv4: > > Config in etc/network/interfaces: > auto eth0 > iface eth0 inet dhcp > metric 4261413864 > > IPv4 Kernel Route Table: > $ ip route list > default via 172.21.47.1 dev eth0 metric 4261413864 > > FRR Table, if a static route is configured: > [In real scenario, it is useful to prefer BGP learned default route over > DHCPv4 default route.] > Codes: K - kernel route, C - connected, S - static, R - RIP, >O - OSPF, I - IS-IS, B - BGP, P - PIM, E - EIGRP, N - NHRP, >T - Table, v - VNC, V - VNC-Direct, A - Babel, D - SHARP, >> - selected route, * - FIB route > > S>* 0.0.0.0/0 [20/0] is directly connected, eth0, 00:00:03 > K 0.0.0.0/0 [254/1000] via 172.21.47.1, eth0, 6d08h51m > > i.e. User can prefer Default Router learned via Routing Protocol in IPv4. > Similar behavior is not possible for IPv6, without this fix. > > After fix [for IPv6]: > sudo sysctl -w > net.ipv6.conf.eth0.net.ipv6.conf.eth0.ra_defrtr_metric=1996489705 > > IP monitor: [When IPv6 RA is received] > default via fe80::xx16::feb3:ce8e dev eth0 proto ra metric 1996489705 > pref high > > Kernel IPv6 routing table > $ ip -6 route list > default via fe80::be16:65ff:feb3:ce8e dev eth0 proto ra metric 1996489705 > expires 21sec hoplimit 64 pref high > > FRR Table, if a static route is configured: > [In real scenario, it is useful to prefer BGP learned default route over IPv6 > RA default route.] > Codes: K - kernel route, C - connected, S - static, R - RIPng, >O - OSPFv3, I - IS-IS, B - BGP, N - NHRP, T - Table, >v - VNC, V - VNC-Direct, A - Babel, D - SHARP, >> - selected route, * - FIB route > > S>* ::/0 [20/0] is directly connected, eth0, 00:00:06 > K ::/0 [119/1001] via fe80::xx16::feb3:ce8e, eth0, 6d07h43m > > If the metric is changed later, the effect will be seen only when next IPv6 > RA is received, because the default route must be fully controlled by RA msg. > Below metric is changed from 1996489705 to 1996489704. > > $ sudo sysctl -w net.ipv6.conf.eth0.ra_defrtr_metric=1996489704 > net.ipv6.conf.eth0.ra_defrtr_metric = 1996489704 > > IP monitor: > [On next IPv6 RA msg, Kernel deletes prev route and installs new route with > updated metric] > > Deleted default via fe80::xx16::feb3:ce8e dev eth0 proto ra metric > 1996489705 expires 3sec hoplimit 64 pref high > default via fe80::xx16::feb3:ce8e dev eth0 proto ra metric 1996489704 > pref high > --- > Documentation/networking/ip-sysctl.rst | 12 > include/linux/ipv6.h | 1 + > include/net/ip6_route.h| 3 ++- > include/uapi/linux/ipv6.h | 1 + > include/uapi/linux/sysctl.h| 1 + > net/ipv6/addrconf.c| 10 ++ > net/ipv6/ndisc.c | 14 ++ > net/ipv6/route.c | 5 +++-- > 8 files changed, 40 insertions(+), 7 deletions(-) > LGTM. I can't think of a better way to do this than a sysctl. Shame that the metric/priority is not an RA option. Reviewed-by: David Ahern
[PATCH v3 net-next 1/1] Allow user to set metric on default route learned via Router Advertisement.
For IPv4, default route is learned via DHCPv4 and user is allowed to change metric using config etc/network/interfaces. But for IPv6, default route can be learned via RA, for which, currently a fixed metric value 1024 is used. Ideally, user should be able to configure metric on default route for IPv6 similar to IPv4. This fix adds sysctl for the same. Signed-off-by: Praveen Chaudhary Signed-off-by: Zhenggen Xu Changes in v1. 1.) Correct the call to rt6_add_dflt_router. Changes in v2. 1.) Replace accept_ra_defrtr_metric to ra_defrtr_metric. 2.) Change Type to __u32 instead of __s32. 3.) Change description in Documentation/networking/ip-sysctl.rst. 4.) Use proc_douintvec instead of proc_dointvec. 5.) Code style in ndisc_router_discovery(). 6.) Change Type to u32 instead of unsigned int. Changes in v3: 1.) Removed '---' and '```' from description. 2.) Remove stray ' after accept_ra_defrtr. 3.) Fix tab in net/ipv6/addrconf.c. Logs: For IPv4: Config in etc/network/interfaces: auto eth0 iface eth0 inet dhcp metric 4261413864 IPv4 Kernel Route Table: $ ip route list default via 172.21.47.1 dev eth0 metric 4261413864 FRR Table, if a static route is configured: [In real scenario, it is useful to prefer BGP learned default route over DHCPv4 default route.] Codes: K - kernel route, C - connected, S - static, R - RIP, O - OSPF, I - IS-IS, B - BGP, P - PIM, E - EIGRP, N - NHRP, T - Table, v - VNC, V - VNC-Direct, A - Babel, D - SHARP, > - selected route, * - FIB route S>* 0.0.0.0/0 [20/0] is directly connected, eth0, 00:00:03 K 0.0.0.0/0 [254/1000] via 172.21.47.1, eth0, 6d08h51m i.e. User can prefer Default Router learned via Routing Protocol in IPv4. Similar behavior is not possible for IPv6, without this fix. After fix [for IPv6]: sudo sysctl -w net.ipv6.conf.eth0.net.ipv6.conf.eth0.ra_defrtr_metric=1996489705 IP monitor: [When IPv6 RA is received] default via fe80::xx16::feb3:ce8e dev eth0 proto ra metric 1996489705 pref high Kernel IPv6 routing table $ ip -6 route list default via fe80::be16:65ff:feb3:ce8e dev eth0 proto ra metric 1996489705 expires 21sec hoplimit 64 pref high FRR Table, if a static route is configured: [In real scenario, it is useful to prefer BGP learned default route over IPv6 RA default route.] Codes: K - kernel route, C - connected, S - static, R - RIPng, O - OSPFv3, I - IS-IS, B - BGP, N - NHRP, T - Table, v - VNC, V - VNC-Direct, A - Babel, D - SHARP, > - selected route, * - FIB route S>* ::/0 [20/0] is directly connected, eth0, 00:00:06 K ::/0 [119/1001] via fe80::xx16::feb3:ce8e, eth0, 6d07h43m If the metric is changed later, the effect will be seen only when next IPv6 RA is received, because the default route must be fully controlled by RA msg. Below metric is changed from 1996489705 to 1996489704. $ sudo sysctl -w net.ipv6.conf.eth0.ra_defrtr_metric=1996489704 net.ipv6.conf.eth0.ra_defrtr_metric = 1996489704 IP monitor: [On next IPv6 RA msg, Kernel deletes prev route and installs new route with updated metric] Deleted default via fe80::xx16::feb3:ce8e dev eth0 proto ra metric 1996489705 expires 3sec hoplimit 64 pref high default via fe80::xx16::feb3:ce8e dev eth0 proto ra metric 1996489704 pref high --- Documentation/networking/ip-sysctl.rst | 12 include/linux/ipv6.h | 1 + include/net/ip6_route.h| 3 ++- include/uapi/linux/ipv6.h | 1 + include/uapi/linux/sysctl.h| 1 + net/ipv6/addrconf.c| 10 ++ net/ipv6/ndisc.c | 14 ++ net/ipv6/route.c | 5 +++-- 8 files changed, 40 insertions(+), 7 deletions(-) diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst index dd2b12a32b73..6a644e794605 100644 --- a/Documentation/networking/ip-sysctl.rst +++ b/Documentation/networking/ip-sysctl.rst @@ -1871,6 +1871,18 @@ accept_ra_defrtr - BOOLEAN - enabled if accept_ra is enabled. - disabled if accept_ra is disabled. +ra_defrtr_metric - INTEGER + Route metric for default route learned in Router Advertisement. This value + will be assigned as metric for the default route learned via IPv6 Router + Advertisement. Takes affect only if accept_ra_defrtr is enabled. + + Possible values are: + 0: + default value will be used for route metric + i.e. IP6_RT_PRIO_USER 1024. + 1 to 0x: + current value will be used for route metric. + accept_ra_from_local - BOOLEAN Accept RA with source-address that is found on local machine if the RA is otherwise proper and able to be accepted. diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h index dda61d150a13..9d1f29f0c512 100644 --- a/include/linux/ipv6.h +++ b/include/linux/ipv6.h @@ -31,6 +31,7
