diff mbox series

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

Message ID 20240509223213.97389-4-newtwen+github@gmail.com
State Changes Requested
Delegated to: Ansuel Smith
Headers show
Series odhcpd config value clamping | expand

Commit Message

Paul Donald May 9, 2024, 10:30 p.m. UTC 
From: Paul Donald <newtwen@gmail.com>

Signed-off-by: Paul Donald <newtwen@gmail.com>
---
 src/config.c | 48 +++++++++++++++++++++++++++++++++++++++++-------
 src/router.c | 10 ----------
 src/router.h |  2 +-
 3 files changed, 42 insertions(+), 18 deletions(-)

Comments

Christian Marangi Nov. 16, 2024, 2:57 p.m. UTC | #1 
On Fri, May 10, 2024 at 12:30:35AM +0200, Paul Donald wrote:
> From: Paul Donald <newtwen@gmail.com>
> 
> Signed-off-by: Paul Donald <newtwen@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)
diff mbox series

Patch

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.
+	 */
 	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;
+	}
 
-	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;
+	}
 
-	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
+	}
 
 	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