From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202410 header.b=WSH2At1X; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 45B8E5A061F for ; Wed, 13 Nov 2024 04:37:28 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202410; t=1731469038; bh=wI5YvTfxgGT76TXVMH3D0VhXhm8qM4HBNqCOlesHKfs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=WSH2At1XzPiPOHpoaGiaaTt6TtIAcIGXiZ2DDtzBYJsU6QZmL9SRM8eNEzlgWuwGU jYf7Dxk9fP1GumvmAx1Z9aeOuM/qNvGrf+ZO3xh0WlI911cvCZ/KQR4VgQLj9BBXhD rjQUbGjc9k8d1KhIxuuEFWi/e2oxcycPn7gZr5XQOs0ayeNCyyvWfOu/26uw0fuNLD 1yR6u2m3PQpu1wWPO1HD09kq9I2Zd3386futNWs4AjRR40Aiyu3aGzpEAUh8e7gswq IKbRovvZ2AC7Zgv1KDPGWAnNLe1MuI5qBLFjEjuF8BfLCjFWVvO0OoM5xMvhmi9ra2 pWvJKXONNRrgQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Xp89V0Gngz4x3q; Wed, 13 Nov 2024 14:37:18 +1100 (AEDT) Date: Wed, 13 Nov 2024 14:01:16 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 6/6] ndp: Send unsolicited Router Advertisements Message-ID: References: <20241112040618.1804081-1-david@gibson.dropbear.id.au> <20241112040618.1804081-7-david@gibson.dropbear.id.au> <20241113021416.418a338c@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="lgsc5AFo5OZ2iOpj" Content-Disposition: inline In-Reply-To: <20241113021416.418a338c@elisabeth> Message-ID-Hash: 6J7NFVBR7NMBMO3JX56VG2CU3QIS6FLN X-Message-ID-Hash: 6J7NFVBR7NMBMO3JX56VG2CU3QIS6FLN X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --lgsc5AFo5OZ2iOpj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Nov 13, 2024 at 02:14:16AM +0100, Stefano Brivio wrote: > Kind of nits only, the whole series looks good to me otherwise: >=20 > On Tue, 12 Nov 2024 15:06:18 +1100 > David Gibson wrote: >=20 > > 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. > >=20 > > Linux as a guest also requires this: it will send an RS when a link fir= st > > 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. > >=20 > > 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 >=20 > s/not// Fixed. > > 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 t= han > > a large broadcast domain. > >=20 > > Link: https://github.com/kubevirt/kubevirt/issues/13191 > > Signed-off-by: David Gibson > > --- > > ip.h | 9 +++++++++ > > ndp.c | 41 +++++++++++++++++++++++++++++++++++++++++ > > ndp.h | 3 +++ > > passt.c | 3 +++ > > 4 files changed, 56 insertions(+) > >=20 > > 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 { > > =20 > > 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 =3D { > > + .s6_addr =3D { > > + 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, > > =20 > > 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 */ >=20 > 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. Ah, thanks for checking. I didn't think to look for a superseding RFC. > > + > > +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 =3D 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 =3D MIN(max_rtr_adv_interval, RT_LIFETIME - 1); > > + > > + /* But we must not go smaller than the minimum delay */ > > + max_rtr_adv_interval =3D MAX(max_rtr_adv_interval, MIN_DELAY_BETWEEN_= RAS); > > + > > + /* RFC 4861, 6.2.1 */ > > + min_rtr_adv_interval =3D MAX(max_rtr_adv_interval / 3, > > + MIN_DELAY_BETWEEN_RAS); > > + > > + interval =3D min_rtr_adv_interval + > > + random() % (max_rtr_adv_interval - min_rtr_adv_interval); >=20 > 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. Good point, I'll add that. > But Coverity is more annoying than I am and reports: >=20 > /home/sbrivio/passt/ndp.c:408:3: > Type: Calling risky function (DC.WEAK_CRYPTO) >=20 > /home/sbrivio/passt/ndp.c:408:3: > dont_call: "random" should not be used for security-related application= s, 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/ran= dom" or "/dev/urandom" on Unix-like systems, and CNG (Cryptography API: Nex= t Generation) on Windows. >=20 > 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. I don't think the latter approach really works. Using the time as the basic input means its still subject to two passt instances (say) starting at the same time on the same link picking the same values and keeping on sending their announcements in lockstep. Picking a random interval once is better. Still, I don't particularly like deviating from the RFC's recommendations just to keep a fussy tool happy. > 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. Also a waste of /dev/random entropy, IMO. Surely there must be some way we can suppress the Coverity whinge. --=20 David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson --lgsc5AFo5OZ2iOpj Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmc0FnsACgkQzQJF27ox 2GeV4Q//XK1IAr1NfafyEMfw224ue8+91ywm8YJHozvAJrRSTM0OKCl+/i3oUOEJ Lc2pKTqViMtuZ2AMPo5TLeVS2gJKPzES4mMADH0yCf3wR7CfKQCCtbbwzL5PJDvf CGIX3sKxg4bknUe7bqiEhWv9Psw51VM4zj0b5HAYIgnsufpZcJsZoys+H+DgMKhf PgX83v7WSMQk/JmYU0tO6Sk3FYMZOLCZb2alG/pgLZHbczDgI5fEIC4GrVayWkMH jZ+XqoHMUynIAyUO11pH1TwCji4rhNMZNP2iOuLv2y0XurVRn/acCRNZlk4xdnLq 0ei26EVd+6GXaapG/l8X+3NMnPjZHIAiCXez3K/l+vfEcOMkLl+uE0KyJnvJNKu4 L+3z4L053ejVp+ftcrsNy0sPQOMCdfncBx0i27cSqNlPXcXjgbjsViIGPBNVkEaU SURZSvY5ujTBaMIeTmuxso2ltK+sCFB1yF8GYoVdyB2jsEvywzV1kOSFov+j8U03 BuPC+ry4MCoyijQsLhKNNLxCqEMgLjkPNl25v72do74zJKznYJE1ukosZwYtQ8SC XACdQ0uB2qR5GA+FhYnFbU15XFkdhJjPMkxsFhPUxyKtyaXJXTSgTYdCxBXaFncW LhhV7g9j7SRPZiNFBokz88xGlvcH87E6xc7AZ581ERlSY+MwAgY= =iuE/ -----END PGP SIGNATURE----- --lgsc5AFo5OZ2iOpj--