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

[praveen chaudhary <praveen5582@gmail.com>]

> On Jan 16, 2021, at 9:13 AM, David Ahern <dsahern@gmail.com> wrote:
> 
> On 1/15/21 1:02 AM, 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 <pchaudhary@linkedin.com>
>> Signed-off-by: Zhenggen Xu <zxu@linkedin.com>
>> 
>> 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.

>> 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.


>> 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.

>> +
>> +	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.
> 
> 
> 
>> 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.

> 
>> 	.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.

> 
>> 	.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