| Message ID | 20240405111025.26478-7-newtwen+github@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | odhcpd patchset | expand |
Hi, On Fri, 5 Apr 2024 at 13:11, Paul Donald <newtwen+github@gmail.com> wrote: > > From: Paul Donald <newtwen@gmail.com> > > https://www.rfc-editor.org/rfc/rfc8319#section-4 > > Signed-off-by: Paul Donald <newtwen@gmail.com> > Reviewed-by: Daniel Golle <daniel@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
On 2024-04-06 12:05, Jonas Gorski wrote:
> Hi,
You're right on all counts. I separated out the value clamping and config values to another patch-set.
Thanks for your sharp eyes.
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; + else if (lifetime > RouterLifetime) + lifetime = RouterLifetime; } 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 #define MinRtrAdvInterval 3 +#define AdvDefaultLifetime 65535 +/* 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 #define ND_RA_FLAG_PROXY 0x4 #define ND_RA_PREF_HIGH (1 << 3)