[PATCH v2 6/9] router: Apply updated values from RFC8319 (updates RFC4861) to RA/ND

[Jonas Gorski]

Hi,

On Fri, 5 Apr 2024 at 13:11, Paul Donald <newtwen+github at gmail.com> wrote:
>
> From: Paul Donald <newtwen at gmail.com>
>
> https://www.rfc-editor.org/rfc/rfc8319#section-4
>
> Signed-off-by: Paul Donald <newtwen at gmail.com>
> Reviewed-by: Daniel Golle <daniel at makrotopia.org>
> ---
>  src/router.c |  6 ++++--
>  src/router.h | 21 ++++++++++++++++++++-
>  2 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/src/router.c b/src/router.c
> index a1a7829..4239aa8 100644
> --- a/src/router.c
> +++ b/src/router.c
> @@ -377,8 +377,10 @@ static uint32_t calc_ra_lifetime(struct interface *iface, uint32_t maxival)
>                 lifetime = iface->ra_lifetime;
>                 if (lifetime > 0 && lifetime < maxival)
>                         lifetime = maxival;
> -               else if (lifetime > 9000)
> -                       lifetime = 9000;
> +               else if (lifetime > AdvDefaultLifetime)
> +                       lifetime = AdvDefaultLifetime;

This clamping should be done in src/config.c, where we get
iface->ra_lifetime from ubus.

> +               else if (lifetime > RouterLifetime)
> +                       lifetime = RouterLifetime;

The only caller of calc_ra_lifetime() already clamps the returned
value to UINT16_MAX ( = 65535), you could as well drop all limiting
here now.

>         }
>
>         return lifetime;
> diff --git a/src/router.h b/src/router.h
> index 0444da8..b91c60a 100644
> --- a/src/router.h
> +++ b/src/router.h
> @@ -32,8 +32,27 @@ struct icmpv6_opt {
>
>  #define MaxInitialRtrAdvInterval       16
>  #define MaxInitialRtAdvs               3
> -#define MaxRtrAdvInterval              1800
> +/* RFC8319 §4
> +   This document updates §4.2 and 6.2.1 of [RFC4861] to change
> +   the following router configuration variables.
> +
> +   In §6.2.1, inside the paragraph that defines
> +   MaxRtrAdvInterval, change 1800 to 65535 seconds.
> +
> +   In §6.2.1, inside the paragraph that defines
> +   AdvDefaultLifetime, change 9000 to 65535 seconds.
> +*/
> +#define MaxRtrAdvInterval              65535

This is a configuration variable, not a constant, naming it like a
defined configuration variable is confusing.

RFC4861 says the default for MaxRtrAdvInterval is 600 seconds (which
we use in src/config.c), not 65535. The maximum allowed value is
increased to 65535 in RFC8319.

Where this limit should be applied is in src/config.c, where we get
the IFACE_ATTR_RA_MAXINTERVAL value (which is currently missing a
range check).

>  #define MinRtrAdvInterval              3
> +#define AdvDefaultLifetime             65535

Likewise, this should be used in src/config.c for a range check of
IFACE_ATTR_RA_LIFETIME.

IFACE_ATTR_RA_MININTERVAL could also use a range check.

> +/* RFC8319 §4
> +   This document updates §4.2 and 6.2.1 of [RFC4861] to change
> +   the following router configuration variables.
> +
> +   In §4.2, inside the paragraph that defines Router Lifetime,
> +   change 9000 to 65535 seconds.
> +*/
> +#define RouterLifetime          65535

This is a limit, not the value to use, so it should be named appropriately.

Best Regards,
Jonas