Re: [PATCH v2 net-next 1/1] Allow user to set metric on default route learned via Router Advertisement.

On Jan 16, 2021, at 9:13 AM, David Ahern [off-list ref] wrote:

On 1/15/21 1:02 AM, Praveen Chaudhary wrote:

quoted

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 <redacted>
Signed-off-by: Zhenggen Xu <redacted>

Changes in v1.
---

your trying to be too fancy in the log messages; everything after this
first '---' is dropped. Just Remove all of the '---' lines and '```' tags.

Removed all ‘—‘ and ‘```’ in v3.

quoted

1.) Correct the call to rt6_add_dflt_router.
---

Changes in v2.
[Refer: lkml.org/lkml/2021/1/14/1400]
---
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.
---

Logs:
----------------------------------------------------------------
For IPv4:
----------------------------------------------------------------

Config in etc/network/interfaces
----------------------------------------------------------------

auto eth0
iface eth0 inet dhcp
   metric 4261413864

how does that work for IPv4? Is the metric passed to the dhclient and it
inserts the route with the given metric or is a dhclient script used to
replace the route after insert?

Yes, DHCP client picks config under “iface eth0 inet dhcp” line and if metric is configured, then it adds the metric for all added routes.

quoted

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index dd2b12a32b73..c4b8d4b8d213 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.

stray ' after accept_ra_defrtr

Removed.

quoted

+
+	Possible values are:
+		0:
+			default value will be used for route metric
+			i.e. IP6_RT_PRIO_USER 1024.
+		1 to 0xFFFFFFFF:
+			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.

quoted

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index eff2cacd5209..b13d3213e58f 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c

@@ -205,6 +205,7 @@ static struct ipv6_devconf ipv6_devconf __read_mostly = {

	.max_desync_factor	= MAX_DESYNC_FACTOR,
	.max_addresses		= IPV6_MAX_ADDRESSES,
	.accept_ra_defrtr	= 1,
+	.ra_defrtr_metric = 0,

make the the '=' align column wise with the existing entries; seems like
your new line is missing a tab

Fixed.

quoted

	.accept_ra_from_local	= 0,
	.accept_ra_min_hop_limit= 1,
	.accept_ra_pinfo	= 1,

@@ -260,6 +261,7 @@ static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {

	.max_desync_factor	= MAX_DESYNC_FACTOR,
	.max_addresses		= IPV6_MAX_ADDRESSES,
	.accept_ra_defrtr	= 1,
+	.ra_defrtr_metric = 0,

same here

Fixed.

quoted

	.accept_ra_from_local	= 0,
	.accept_ra_min_hop_limit= 1,
	.accept_ra_pinfo	= 1,

@@ -5475,6 +5477,7 @@ static inline void ipv6_store_devconf(struct ipv6_devconf *cnf,

	array[DEVCONF_MAX_DESYNC_FACTOR] = cnf->max_desync_factor;
	array[DEVCONF_MAX_ADDRESSES] = cnf->max_addresses;
	array[DEVCONF_ACCEPT_RA_DEFRTR] = cnf->accept_ra_defrtr;
+	array[DEVCONF_RA_DEFRTR_METRIC] = cnf->ra_defrtr_metric;
	array[DEVCONF_ACCEPT_RA_MIN_HOP_LIMIT] = cnf->accept_ra_min_hop_limit;
	array[DEVCONF_ACCEPT_RA_PINFO] = cnf->accept_ra_pinfo;
#ifdef CONFIG_IPV6_ROUTER_PREF

@@ -6667,6 +6670,13 @@ static const struct ctl_table addrconf_sysctl[] = {

		.mode		= 0644,
		.proc_handler	= proc_dointvec,
	},
+	{
+		.procname	= "ra_defrtr_metric",
+		.data		= &ipv6_devconf.ra_defrtr_metric,
+		.maxlen		= sizeof(u32),
+		.mode		= 0644,
+		.proc_handler	= proc_douintvec,
+	},
	{
		.procname	= "accept_ra_min_hop_limit",
		.data		= &ipv6_devconf.accept_ra_min_hop_limit,

diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 76717478f173..2bffed49f5c0 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c

@@ -1173,6 +1173,7 @@ static void ndisc_router_discovery(struct sk_buff *skb)

	struct neighbour *neigh = NULL;
	struct inet6_dev *in6_dev;
	struct fib6_info *rt = NULL;
+	u32 defrtr_usr_metric;
	struct net *net;
	int lifetime;
	struct ndisc_options ndopts;

@@ -1303,18 +1304,23 @@ static void ndisc_router_discovery(struct sk_buff *skb)

			return;
		}
	}
-	if (rt && lifetime == 0) {
+	/* Set default route metric if specified by user */
+	defrtr_usr_metric = in6_dev->cnf.accept_ra_defrtr_metric;

this tells me you did not compile this version of the patch since the
'accept_' has been dropped. Always compile and test every patch before
sending.

Yeah one patch was pushed bit early. Sorry about that. I will take care of this now onwards.

I have respined the Patch (v3) after addressing your review comments. Build is done in our pipeline.
Build logs: https://dev.azure.com/sonicswitch/build/_build/results?buildId=1669&view=logs&j=011e1ec8-6569-5e69-4f06-baf193d1351e

Thanks a lot again for spending time for this Review,
This feature will help SONiC OS [and others Linux flavors] for better IPv6 support, so thanks again.


Praveen 
https://github.com/praveen-li