Re: [PATCH 6/6] ndp: Send unsolicited Router Advertisements

[Stefano Brivio <sbrivio@redhat.com>]

Kind of nits only, the whole series looks good to me otherwise:

On Tue, 12 Nov 2024 15:06:18 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently, our NDP implementation only sends Router Advertisements (RA)
> when it receives a Router Solicitation (RS) from the guest.  However,
> RFC 4861 requires that we periodically send unsolicited RAs.
> 
> Linux as a guest also requires this: it will send an RS when a link first
> comes up, but the route it gets from this will have a finite lifetime (we
> set this to 65535s, the maximum allowed, around 18 hours).  When that
> expires the guest will not send a new RS, but instead expects the route to
> have been renewed (if still valid) by an unsolicited RA.
> 
> Implement sending unsolicited RAs on a partially randomised timer, as
> required by RFC 4861.  The RFC also specifies that solicited RAs should
> also be delayed, or even not omitted, if the next unsolicited RA is soon

s/not//

> enough.  For now we don't do that, always sending an immediate RA in
> response to an RS.  We can get away with this because in our use cases
> we expect to just have passt itself and the guest on the link, rather than
> a large broadcast domain.
> 
> Link: https://github.com/kubevirt/kubevirt/issues/13191
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  ip.h    |  9 +++++++++
>  ndp.c   | 41 +++++++++++++++++++++++++++++++++++++++++
>  ndp.h   |  3 +++
>  passt.c |  3 +++
>  4 files changed, 56 insertions(+)
> 
> diff --git a/ip.h b/ip.h
> index b8d4a5b..0742612 100644
> --- a/ip.h
> +++ b/ip.h
> @@ -92,4 +92,13 @@ struct ipv6_opt_hdr {
>  
>  char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
>  		 size_t *dlen);
> +
> +/* IPv6 link-local all-nodes multicast adddress, ff02::1 */
> +static const struct in6_addr in6addr_ll_all_nodes = {
> +	.s6_addr = {
> +		0xff, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
> +	},
> +};
> +
>  #endif /* IP_H */
> diff --git a/ndp.c b/ndp.c
> index c5fbccf..8c019df 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -372,3 +372,44 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
>  
>  	return 1;
>  }
> +
> +/* Default interval between unsolicited RAs (seconds) */
> +#define DEFAULT_MAX_RTR_ADV_INTERVAL	600	/* RFC 4861, 6.2.1 */
> +
> +/* Minimum required interval between RAs (seconds) */
> +#define MIN_DELAY_BETWEEN_RAS		3	/* RFC 4861, 10 */

By the way, I think this is still all correct, as I quickly double
checked it against RFC 8319. If you're not aware: see also sections 3
and 4 there.

> +
> +static time_t next_ra;
> +
> +/**
> + * ndp_timer() - Send unsolicited NDP messages if necessary
> + * @c:		Execution context
> + * @now:	Current (monotonic) time
> + */
> +void ndp_timer(const struct ctx *c, const struct timespec *now)
> +{
> +	time_t max_rtr_adv_interval = DEFAULT_MAX_RTR_ADV_INTERVAL;
> +	time_t min_rtr_adv_interval, interval;
> +
> +	if (c->no_ra || now->tv_sec < next_ra)
> +		return;
> +
> +	/* We must advertise before the route's lifetime expires */
> +	max_rtr_adv_interval = MIN(max_rtr_adv_interval, RT_LIFETIME - 1);
> +
> +	/* But we must not go smaller than the minimum delay */
> +	max_rtr_adv_interval = MAX(max_rtr_adv_interval, MIN_DELAY_BETWEEN_RAS);
> +
> +	/* RFC 4861, 6.2.1 */
> +	min_rtr_adv_interval = MAX(max_rtr_adv_interval / 3,
> +				   MIN_DELAY_BETWEEN_RAS);
> +
> +	interval = min_rtr_adv_interval +
> +		random() % (max_rtr_adv_interval - min_rtr_adv_interval);

So, I would have called srandom() before this, especially in the case
we get, one day, two instances of passt advertising at the same time.

But Coverity is more annoying than I am and reports:

/home/sbrivio/passt/ndp.c:408:3:
  Type: Calling risky function (DC.WEAK_CRYPTO)

/home/sbrivio/passt/ndp.c:408:3:
  dont_call: "random" should not be used for security-related applications, because linear congruential algorithms are too easy to break.
/home/sbrivio/passt/ndp.c:408:3:
  remediation: Use a compliant random number generator, such as "/dev/random" or "/dev/urandom" on Unix-like systems, and CNG (Cryptography API: Next Generation) on Windows.

Of course it's all bogus but I would have a *slight* preference to get
rid of this, by either picking a fixed interval deviation at the
beginning with getrandom(), or using something like tcp_init_seq()
modulo something << 600.

Alternatively, we could also keep /dev/random or /dev/urandom open but
it looks totally overkill. At that point I'd rather keep random() here.

> +
> +	info("NDP: sending unsolicited RA, next in %llds", (long long)interval);
> +
> +	ndp_ra(c, &in6addr_ll_all_nodes);
> +
> +	next_ra = now->tv_sec + interval;
> +}
> diff --git a/ndp.h b/ndp.h
> index abe6d02..41c2000 100644
> --- a/ndp.h
> +++ b/ndp.h
> @@ -6,7 +6,10 @@
>  #ifndef NDP_H
>  #define NDP_H
>  
> +struct icmp6hdr;
> +
>  int ndp(const struct ctx *c, const struct icmp6hdr *ih,
>  	const struct in6_addr *saddr, const struct pool *p);
> +void ndp_timer(const struct ctx *c, const struct timespec *now);
>  
>  #endif /* NDP_H */
> diff --git a/passt.c b/passt.c
> index fac6101..454ac8e 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -52,6 +52,7 @@
>  #include "arch.h"
>  #include "log.h"
>  #include "tcp_splice.h"
> +#include "ndp.h"
>  
>  #define EPOLL_EVENTS		8
>  
> @@ -110,6 +111,8 @@ static void post_handler(struct ctx *c, const struct timespec *now)
>  
>  	flow_defer_handler(c, now);
>  #undef CALL_PROTO_HANDLER
> +
> +	ndp_timer(c, now);
>  }
>  
>  /**

-- 
Stefano