[RFC PATCH 03/14] config: clamp ra_mininterval, ra_maxinterval, ra_lifetime at load time

[Christian Marangi]

On Fri, May 10, 2024 at 12:30:35AM +0200, Paul Donald wrote:
> From: Paul Donald <newtwen at gmail.com>
> 
> Signed-off-by: Paul Donald <newtwen at gmail.com>
> ---
>  src/config.c | 48 +++++++++++++++++++++++++++++++++++++++++-------
>  src/router.c | 10 ----------
>  src/router.h |  2 +-
>  3 files changed, 42 insertions(+), 18 deletions(-)
> 
> diff --git a/src/config.c b/src/config.c
> index 346d74a..2ccf742 100644
> --- a/src/config.c
> +++ b/src/config.c
> @@ -18,6 +18,7 @@
>  #include <libubox/vlist.h>
>  
>  #include "odhcpd.h"
> +#include "router.h"
>  
>  static struct blob_buf b;
>  static int reload_pipe[2] = { -1, -1 };
> @@ -227,8 +228,12 @@ static void set_interface_defaults(struct interface *iface)
>  	iface->ra_flags = ND_RA_FLAG_OTHER;
>  	iface->ra_slaac = true;
>  	iface->ra_maxinterval = 600;
> +	/*
> +	 * RFC4861: MinRtrAdvInterval: Default: 0.33 * MaxRtrAdvInterval If
> +	 * MaxRtrAdvInterval >= 9 seconds; otherwise, the Default is MaxRtrAdvInterval.
> +	 */

Any idea how to handle the if condition? If possible?

Maybe we set here min to -1 and handle later in calc?

>  	iface->ra_mininterval = iface->ra_maxinterval/3;
> -	iface->ra_lifetime = -1;
> +	iface->ra_lifetime = 3 * iface->ra_maxinterval; /* RFC4861: AdvDefaultLifetime: Default: 3 * MaxRtrAdvInterval */
>  	iface->ra_dns = true;
>  }
>  
> @@ -966,14 +971,43 @@ int config_parse_interface(void *data, size_t len, const char *name, bool overwr
>  	if ((c = tb[IFACE_ATTR_RA_ADVROUTER]))
>  		iface->ra_advrouter = blobmsg_get_bool(c);
>  
> -	if ((c = tb[IFACE_ATTR_RA_MININTERVAL]))
> -		iface->ra_mininterval =  blobmsg_get_u32(c);
> +	/*
> +	 * RFC4861: MaxRtrAdvInterval: MUST be no less than 4 seconds and no greater than 1800 seconds.
> +	 * RFC8319: MaxRtrAdvInterval: MUST be no less than 4 seconds and no greater than 65535 seconds.
> +	 * Default: 600 seconds
> +	 */
> +	if ((c = tb[IFACE_ATTR_RA_MAXINTERVAL])){
> +		uint32_t ra_maxinterval = blobmsg_get_u32(c);
> +		iface->ra_maxinterval = (ra_maxinterval < 4) ? 4 :
> +			(ra_maxinterval > MaxRtrAdvInterval_CEILING) ?
> +				MaxRtrAdvInterval_CEILING : ra_maxinterval;
> +	}

Mhhh ? : are ok for simple condition but here we have nested ones...
Better to use the full if condition form.

>  
> -	if ((c = tb[IFACE_ATTR_RA_MAXINTERVAL]))
> -		iface->ra_maxinterval = blobmsg_get_u32(c);
> +	/*
> +	 * RFC4861: MinRtrAdvInterval: MUST be no less than 3 seconds and no greater than .75 * MaxRtrAdvInterval.
> +	 * Default: 0.33 * MaxRtrAdvInterval If MaxRtrAdvInterval >= 9 seconds; otherwise, the
> +	 * Default is MaxRtrAdvInterval.
> +	 */
> +	if ((c = tb[IFACE_ATTR_RA_MININTERVAL])){
> +		uint32_t ra_mininterval = blobmsg_get_u32(c);
> +		iface->ra_mininterval = (ra_mininterval < MinRtrAdvInterval_FLOOR) ? // clamp min
> +			MinRtrAdvInterval_FLOOR : (ra_mininterval > 0.75 * (uint32_t)iface->ra_maxinterval) ? // clamp max
> +				0.75 * (uint32_t)iface->ra_maxinterval : ra_mininterval;
> +	}

Ditto.

>  
> -	if ((c = tb[IFACE_ATTR_RA_LIFETIME]))
> -		iface->ra_lifetime = blobmsg_get_u32(c);
> +	/* 
> +	 * RFC4861: AdvDefaultLifetime: MUST be either zero or between MaxRtrAdvInterval and 9000 seconds.
> +	 * RFC8319: AdvDefaultLifetime: MUST be either zero or between MaxRtrAdvInterval and 65535 seconds.
> +	 * Default: 3 * MaxRtrAdvInterval
> +	 * i.e. 3 * 65535 => 65535 seconds.
> +	 */
> +	if ((c = tb[IFACE_ATTR_RA_LIFETIME])){
> +		uint32_t ra_lifetime = blobmsg_get_u32(c);
> +		iface->ra_lifetime = (ra_lifetime == 0) ? 0 : // leave at 0
> +			(ra_lifetime < (uint32_t)iface->ra_maxinterval) ? (uint32_t)iface->ra_maxinterval : // clamp min
> +				(ra_lifetime > AdvDefaultLifetime_CEILING) ? 
> +					AdvDefaultLifetime_CEILING : ra_lifetime; // clamp max
> +	}

Ditto.

>  
>  	if ((c = tb[IFACE_ATTR_RA_USELEASETIME]))
>  		iface->ra_useleasetime = blobmsg_get_bool(c);
> diff --git a/src/router.c b/src/router.c
> index ae0c545..96237d6 100644
> --- a/src/router.c
> +++ b/src/router.c
> @@ -350,16 +350,6 @@ static int calc_adv_interval(struct interface *iface, uint32_t lowest_found_life
>  	if (*maxival > lowest_found_lifetime/3)
>  		*maxival = lowest_found_lifetime/3;
>  
> -	if (*maxival > MaxRtrAdvInterval)
> -		*maxival = MaxRtrAdvInterval;
> -	else if (*maxival < 4)
> -		*maxival = 4;
> -
> -	if (minival < MinRtrAdvInterval)
> -		minival = MinRtrAdvInterval;
> -	else if (minival > (*maxival * 3)/4)
> -		minival = (*maxival >= 9 ? *maxival/3 : *maxival);
> -
>  	odhcpd_urandom(&msecs, sizeof(msecs));
>  	msecs = (labs(msecs) % ((*maxival != minival) ? (*maxival - minival)*1000 : 500)) +
>  			minival*1000;
> diff --git a/src/router.h b/src/router.h
> index 1f8d156..8e9499e 100644
> --- a/src/router.h
> +++ b/src/router.h
> @@ -43,7 +43,7 @@ struct icmpv6_opt {
>  	AdvDefaultLifetime, change 9000 to 65535 seconds.
>  */
>  #define MaxRtrAdvInterval_CEILING		65535
> -#define MinRtrAdvInterval		3
> +#define MinRtrAdvInterval_FLOOR			3
>  #define AdvDefaultLifetime_CEILING		65535
>  /* RFC8319 §4
>  	This document updates §4.2 and 6.2.1 of [RFC4861] to change

Same here and for rest of the series, do we really need these additional
define? (and bisectability problem)

-- 
	Ansuel