[PATCH 0/6] ndp: Unsolicited RAs

[PATCH 0/6] ndp: Unsolicited RAs

[David Gibson]

The default route a guest obtains via NDP has an expiry.  We set this
to the maximum allowed, 65535s (about 18 hours).  We missed, however,
that after that expiry, the guest won't send a new Router
Solicitation.  Instead it expects an unsolicited Router Advertisement
to have come at some point before the expiry.  This means that on an
IPv6 setup, the default will disappear after 18 hours.

Correct this by sending unsolicited Router Advertisements as required
by RFC 4861.  Along the way we make a number of small cleanups to the
NDP code.

Link: https://github.com/kubevirt/kubevirt/issues/13191

David Gibson (6):
  ndp: Remove redundant update to addr_seen
  ndp: Add ndp_send() helper
  ndp: Split out helpers for sending specific NDP message types
  ndp: Use struct assignment in preference to memcpy() for IPv6
    addresses
  ndp: Make route lifetime a #define
  ndp: Send unsolicited Router Advertisements

 ip.h    |   9 +++
 ndp.c   | 210 ++++++++++++++++++++++++++++++++++++--------------------
 ndp.h   |   7 +-
 passt.c |   3 +
 4 files changed, 152 insertions(+), 77 deletions(-)

-- 
2.47.0

[PATCH 1/6] ndp: Remove redundant update to addr_seen

[David Gibson]

ndp() updates addr_seen or addr_ll_seen based on the source address of the
received packet.  This is redundant since tap6_handler() has already
updated addr_seen for any type of packet, not just NDP.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ndp.c | 9 ++-------
 ndp.h | 4 ++--
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/ndp.c b/ndp.c
index faae408..ab80898 100644
--- a/ndp.c
+++ b/ndp.c
@@ -179,8 +179,8 @@ struct ndp_ns {
  *
  * Return: 0 if not handled here, 1 if handled, -1 on failure
  */
-int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr,
-	const struct pool *p)
+int ndp(const struct ctx *c, const struct icmp6hdr *ih,
+	const struct in6_addr *saddr, const struct pool *p)
 {
 	struct ndp_na na = {
 		.ih = {
@@ -336,11 +336,6 @@ dns_done:
 		return 1;
 	}
 
-	if (IN6_IS_ADDR_LINKLOCAL(saddr))
-		c->ip6.addr_ll_seen = *saddr;
-	else
-		c->ip6.addr_seen = *saddr;
-
 	rsaddr = &c->ip6.our_tap_ll;
 
 	if (ih->icmp6_type == NS) {
diff --git a/ndp.h b/ndp.h
index a786441..abe6d02 100644
--- a/ndp.h
+++ b/ndp.h
@@ -6,7 +6,7 @@
 #ifndef NDP_H
 #define NDP_H
 
-int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr,
-	const struct pool *p);
+int ndp(const struct ctx *c, const struct icmp6hdr *ih,
+	const struct in6_addr *saddr, const struct pool *p);
 
 #endif /* NDP_H */
-- 
@@ -6,7 +6,7 @@
 #ifndef NDP_H
 #define NDP_H
 
-int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr,
-	const struct pool *p);
+int ndp(const struct ctx *c, const struct icmp6hdr *ih,
+	const struct in6_addr *saddr, const struct pool *p);
 
 #endif /* NDP_H */
-- 
2.47.0

[PATCH 2/6] ndp: Add ndp_send() helper

[David Gibson]

ndp() has a conditional on message type generating the reply message, then
a tiny amount of common code, then another conditional to send the reply
with slightly different parameters.  We can make this a bit neater by
making a helper function for sending the reply, and call it from each of
the different message type paths.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ndp.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/ndp.c b/ndp.c
index ab80898..fa1b67a 100644
--- a/ndp.c
+++ b/ndp.c
@@ -170,6 +170,21 @@ struct ndp_ns {
 	struct in6_addr target_addr;
 } __attribute__((packed));
 
+/**
+ * ndp_send() - Send an NDP message
+ * @c:		Execution context
+ * @dst:	IPv6 address to send the message to
+ * @buf:	ICMPv6 header + message payload
+ * @l4len:	Length of message, including ICMPv6 header
+ */
+static void ndp_send(const struct ctx *c, const struct in6_addr *dst,
+		     const void *buf, size_t l4len)
+{
+	const struct in6_addr *src = &c->ip6.our_tap_ll;
+
+	tap_icmp6_send(c, src, dst, buf, l4len);
+}
+
 /**
  * ndp() - Check for NDP solicitations, reply as needed
  * @c:		Execution context
@@ -223,9 +238,6 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 			},
 		},
 	};
-	const struct in6_addr *rsaddr; /* src addr for reply */
-	unsigned char *ptr = NULL;
-	size_t dlen;
 
 	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
 		return 0;
@@ -249,7 +261,9 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 		       sizeof(na.target_addr));
 		memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
 
+		ndp_send(c, saddr, &na, sizeof(struct ndp_na));
 	} else if (ih->icmp6_type == RS) {
+		unsigned char *ptr = NULL;
 		size_t dns_s_len = 0;
 		int i, n;
 
@@ -332,18 +346,8 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 
 dns_done:
 		memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
-	} else {
-		return 1;
-	}
 
-	rsaddr = &c->ip6.our_tap_ll;
-
-	if (ih->icmp6_type == NS) {
-		dlen = sizeof(struct ndp_na);
-		tap_icmp6_send(c, rsaddr, saddr, &na, dlen);
-	} else if (ih->icmp6_type == RS) {
-		dlen = ptr - (unsigned char *)&ra;
-		tap_icmp6_send(c, rsaddr, saddr, &ra, dlen);
+		ndp_send(c, saddr, &ra, ptr - (unsigned char *)&ra);
 	}
 
 	return 1;
-- 
@@ -170,6 +170,21 @@ struct ndp_ns {
 	struct in6_addr target_addr;
 } __attribute__((packed));
 
+/**
+ * ndp_send() - Send an NDP message
+ * @c:		Execution context
+ * @dst:	IPv6 address to send the message to
+ * @buf:	ICMPv6 header + message payload
+ * @l4len:	Length of message, including ICMPv6 header
+ */
+static void ndp_send(const struct ctx *c, const struct in6_addr *dst,
+		     const void *buf, size_t l4len)
+{
+	const struct in6_addr *src = &c->ip6.our_tap_ll;
+
+	tap_icmp6_send(c, src, dst, buf, l4len);
+}
+
 /**
  * ndp() - Check for NDP solicitations, reply as needed
  * @c:		Execution context
@@ -223,9 +238,6 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 			},
 		},
 	};
-	const struct in6_addr *rsaddr; /* src addr for reply */
-	unsigned char *ptr = NULL;
-	size_t dlen;
 
 	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
 		return 0;
@@ -249,7 +261,9 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 		       sizeof(na.target_addr));
 		memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
 
+		ndp_send(c, saddr, &na, sizeof(struct ndp_na));
 	} else if (ih->icmp6_type == RS) {
+		unsigned char *ptr = NULL;
 		size_t dns_s_len = 0;
 		int i, n;
 
@@ -332,18 +346,8 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 
 dns_done:
 		memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
-	} else {
-		return 1;
-	}
 
-	rsaddr = &c->ip6.our_tap_ll;
-
-	if (ih->icmp6_type == NS) {
-		dlen = sizeof(struct ndp_na);
-		tap_icmp6_send(c, rsaddr, saddr, &na, dlen);
-	} else if (ih->icmp6_type == RS) {
-		dlen = ptr - (unsigned char *)&ra;
-		tap_icmp6_send(c, rsaddr, saddr, &ra, dlen);
+		ndp_send(c, saddr, &ra, ptr - (unsigned char *)&ra);
 	}
 
 	return 1;
-- 
2.47.0

[PATCH 3/6] ndp: Split out helpers for sending specific NDP message types

[David Gibson]

Currently the large ndp() function responds to all NDP messages we handle,
both parsing the message as necessary and sending the response.  Split out
the code to construct and send specific message types into ndp_na() (to
send NA messages) and ndp_ra() (to send RA messages).

As well as breaking up an excessively large function, this is a first step
to being able to send unsolicited NDP messages.

While we're there, remove a slighty ugly goto.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ndp.c | 136 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 78 insertions(+), 58 deletions(-)

diff --git a/ndp.c b/ndp.c
index fa1b67a..e876c34 100644
--- a/ndp.c
+++ b/ndp.c
@@ -186,16 +186,13 @@ static void ndp_send(const struct ctx *c, const struct in6_addr *dst,
 }
 
 /**
- * ndp() - Check for NDP solicitations, reply as needed
+ * ndp_na() - Send an NDP Neighbour Advertisement (NA) message
  * @c:		Execution context
- * @ih:		ICMPv6 header
- * @saddr:	Source IPv6 address
- * @p:		Packet pool
- *
- * Return: 0 if not handled here, 1 if handled, -1 on failure
+ * @dst:	IPv6 address to send the NA to
+ * @addr:	IPv6 address to advertise
  */
-int ndp(const struct ctx *c, const struct icmp6hdr *ih,
-	const struct in6_addr *saddr, const struct pool *p)
+static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
+		   const void *addr)
 {
 	struct ndp_na na = {
 		.ih = {
@@ -212,6 +209,20 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 			},
 		}
 	};
+
+	memcpy(&na.target_addr, addr, sizeof(na.target_addr));
+	memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
+
+	ndp_send(c, dst, &na, sizeof(na));
+}
+
+/**
+ * ndp_ra() - Send an NDP Router Advertisement (RA) message
+ * @c:		Execution context
+ * @dst:	IPv6 address to send the RA to
+ */
+static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
+{
 	struct ndp_ra ra = {
 		.ih = {
 			.icmp6_type		= RA,
@@ -238,58 +249,28 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 			},
 		},
 	};
+	unsigned char *ptr = NULL;
 
-	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
-		return 0;
-
-	if (c->no_ndp)
-		return 1;
-
-	if (ih->icmp6_type == NS) {
-		const struct ndp_ns *ns =
-			packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL);
+	memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix));
 
-		if (!ns)
-			return -1;
+	ptr = &ra.var[0];
 
-		if (IN6_IS_ADDR_UNSPECIFIED(saddr))
-			return 1;
-
-		info("NDP: received NS, sending NA");
-
-		memcpy(&na.target_addr, &ns->target_addr,
-		       sizeof(na.target_addr));
-		memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
+	if (c->mtu != -1) {
+		struct opt_mtu *mtu = (struct opt_mtu *)ptr;
+		*mtu = (struct opt_mtu) {
+			.header = {
+				.type		= OPT_MTU,
+				.len		= 1,
+			},
+			.value			= htonl(c->mtu),
+		};
+		ptr += sizeof(struct opt_mtu);
+	}
 
-		ndp_send(c, saddr, &na, sizeof(struct ndp_na));
-	} else if (ih->icmp6_type == RS) {
-		unsigned char *ptr = NULL;
+	if (!c->no_dhcp_dns) {
 		size_t dns_s_len = 0;
 		int i, n;
 
-		if (c->no_ra)
-			return 1;
-
-		info("NDP: received RS, sending RA");
-		memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix));
-
-		ptr = &ra.var[0];
-
-		if (c->mtu != -1) {
-			struct opt_mtu *mtu = (struct opt_mtu *)ptr;
-			*mtu = (struct opt_mtu) {
-				.header = {
-					.type		= OPT_MTU,
-					.len		= 1,
-				},
-				.value			= htonl(c->mtu),
-			};
-			ptr += sizeof(struct opt_mtu);
-		}
-
-		if (c->no_dhcp_dns)
-			goto dns_done;
-
 		for (n = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[n]); n++);
 		if (n) {
 			struct opt_rdnss *rdnss = (struct opt_rdnss *)ptr;
@@ -305,7 +286,7 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 				       sizeof(rdnss->dns[i]));
 			}
 			ptr += offsetof(struct opt_rdnss, dns) +
-			       i * sizeof(rdnss->dns[0]);
+				i * sizeof(rdnss->dns[0]);
 
 			for (n = 0; *c->dns_search[n].n; n++)
 				dns_s_len += strlen(c->dns_search[n].n) + 2;
@@ -329,7 +310,7 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 				*(ptr++) = '.';
 
 				len = sizeof(dnssl->domains) -
-				      (ptr - dnssl->domains);
+					(ptr - dnssl->domains);
 
 				strncpy((char *)ptr, c->dns_search[i].n, len);
 				for (dot = (char *)ptr - 1; *dot; dot++) {
@@ -343,11 +324,50 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 			memset(ptr, 0, 8 - dns_s_len % 8);	/* padding */
 			ptr += 8 - dns_s_len % 8;
 		}
+	}
 
-dns_done:
-		memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
+	memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
 
-		ndp_send(c, saddr, &ra, ptr - (unsigned char *)&ra);
+	ndp_send(c, dst, &ra, ptr - (unsigned char *)&ra);
+}
+
+/**
+ * ndp() - Check for NDP solicitations, reply as needed
+ * @c:		Execution context
+ * @ih:		ICMPv6 header
+ * @saddr:	Source IPv6 address
+ * @p:		Packet pool
+ *
+ * Return: 0 if not handled here, 1 if handled, -1 on failure
+ */
+int ndp(const struct ctx *c, const struct icmp6hdr *ih,
+	const struct in6_addr *saddr, const struct pool *p)
+{
+	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
+		return 0;
+
+	if (c->no_ndp)
+		return 1;
+
+	if (ih->icmp6_type == NS) {
+		const struct ndp_ns *ns =
+			packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL);
+
+		if (!ns)
+			return -1;
+
+		if (IN6_IS_ADDR_UNSPECIFIED(saddr))
+			return 1;
+
+		info("NDP: received NS, sending NA");
+
+		ndp_na(c, saddr, &ns->target_addr);
+	} else if (ih->icmp6_type == RS) {
+		if (c->no_ra)
+			return 1;
+
+		info("NDP: received RS, sending RA");
+		ndp_ra(c, saddr);
 	}
 
 	return 1;
-- 
@@ -186,16 +186,13 @@ static void ndp_send(const struct ctx *c, const struct in6_addr *dst,
 }
 
 /**
- * ndp() - Check for NDP solicitations, reply as needed
+ * ndp_na() - Send an NDP Neighbour Advertisement (NA) message
  * @c:		Execution context
- * @ih:		ICMPv6 header
- * @saddr:	Source IPv6 address
- * @p:		Packet pool
- *
- * Return: 0 if not handled here, 1 if handled, -1 on failure
+ * @dst:	IPv6 address to send the NA to
+ * @addr:	IPv6 address to advertise
  */
-int ndp(const struct ctx *c, const struct icmp6hdr *ih,
-	const struct in6_addr *saddr, const struct pool *p)
+static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
+		   const void *addr)
 {
 	struct ndp_na na = {
 		.ih = {
@@ -212,6 +209,20 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 			},
 		}
 	};
+
+	memcpy(&na.target_addr, addr, sizeof(na.target_addr));
+	memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
+
+	ndp_send(c, dst, &na, sizeof(na));
+}
+
+/**
+ * ndp_ra() - Send an NDP Router Advertisement (RA) message
+ * @c:		Execution context
+ * @dst:	IPv6 address to send the RA to
+ */
+static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
+{
 	struct ndp_ra ra = {
 		.ih = {
 			.icmp6_type		= RA,
@@ -238,58 +249,28 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 			},
 		},
 	};
+	unsigned char *ptr = NULL;
 
-	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
-		return 0;
-
-	if (c->no_ndp)
-		return 1;
-
-	if (ih->icmp6_type == NS) {
-		const struct ndp_ns *ns =
-			packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL);
+	memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix));
 
-		if (!ns)
-			return -1;
+	ptr = &ra.var[0];
 
-		if (IN6_IS_ADDR_UNSPECIFIED(saddr))
-			return 1;
-
-		info("NDP: received NS, sending NA");
-
-		memcpy(&na.target_addr, &ns->target_addr,
-		       sizeof(na.target_addr));
-		memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
+	if (c->mtu != -1) {
+		struct opt_mtu *mtu = (struct opt_mtu *)ptr;
+		*mtu = (struct opt_mtu) {
+			.header = {
+				.type		= OPT_MTU,
+				.len		= 1,
+			},
+			.value			= htonl(c->mtu),
+		};
+		ptr += sizeof(struct opt_mtu);
+	}
 
-		ndp_send(c, saddr, &na, sizeof(struct ndp_na));
-	} else if (ih->icmp6_type == RS) {
-		unsigned char *ptr = NULL;
+	if (!c->no_dhcp_dns) {
 		size_t dns_s_len = 0;
 		int i, n;
 
-		if (c->no_ra)
-			return 1;
-
-		info("NDP: received RS, sending RA");
-		memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix));
-
-		ptr = &ra.var[0];
-
-		if (c->mtu != -1) {
-			struct opt_mtu *mtu = (struct opt_mtu *)ptr;
-			*mtu = (struct opt_mtu) {
-				.header = {
-					.type		= OPT_MTU,
-					.len		= 1,
-				},
-				.value			= htonl(c->mtu),
-			};
-			ptr += sizeof(struct opt_mtu);
-		}
-
-		if (c->no_dhcp_dns)
-			goto dns_done;
-
 		for (n = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[n]); n++);
 		if (n) {
 			struct opt_rdnss *rdnss = (struct opt_rdnss *)ptr;
@@ -305,7 +286,7 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 				       sizeof(rdnss->dns[i]));
 			}
 			ptr += offsetof(struct opt_rdnss, dns) +
-			       i * sizeof(rdnss->dns[0]);
+				i * sizeof(rdnss->dns[0]);
 
 			for (n = 0; *c->dns_search[n].n; n++)
 				dns_s_len += strlen(c->dns_search[n].n) + 2;
@@ -329,7 +310,7 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 				*(ptr++) = '.';
 
 				len = sizeof(dnssl->domains) -
-				      (ptr - dnssl->domains);
+					(ptr - dnssl->domains);
 
 				strncpy((char *)ptr, c->dns_search[i].n, len);
 				for (dot = (char *)ptr - 1; *dot; dot++) {
@@ -343,11 +324,50 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
 			memset(ptr, 0, 8 - dns_s_len % 8);	/* padding */
 			ptr += 8 - dns_s_len % 8;
 		}
+	}
 
-dns_done:
-		memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
+	memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
 
-		ndp_send(c, saddr, &ra, ptr - (unsigned char *)&ra);
+	ndp_send(c, dst, &ra, ptr - (unsigned char *)&ra);
+}
+
+/**
+ * ndp() - Check for NDP solicitations, reply as needed
+ * @c:		Execution context
+ * @ih:		ICMPv6 header
+ * @saddr:	Source IPv6 address
+ * @p:		Packet pool
+ *
+ * Return: 0 if not handled here, 1 if handled, -1 on failure
+ */
+int ndp(const struct ctx *c, const struct icmp6hdr *ih,
+	const struct in6_addr *saddr, const struct pool *p)
+{
+	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
+		return 0;
+
+	if (c->no_ndp)
+		return 1;
+
+	if (ih->icmp6_type == NS) {
+		const struct ndp_ns *ns =
+			packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL);
+
+		if (!ns)
+			return -1;
+
+		if (IN6_IS_ADDR_UNSPECIFIED(saddr))
+			return 1;
+
+		info("NDP: received NS, sending NA");
+
+		ndp_na(c, saddr, &ns->target_addr);
+	} else if (ih->icmp6_type == RS) {
+		if (c->no_ra)
+			return 1;
+
+		info("NDP: received RS, sending RA");
+		ndp_ra(c, saddr);
 	}
 
 	return 1;
-- 
2.47.0

[PATCH 4/6] ndp: Use struct assignment in preference to memcpy() for IPv6 addresses

[David Gibson]

There are a number of places we can simply assign IPv6 addresses about,
rather than the current mildly ugly memcpy().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ndp.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/ndp.c b/ndp.c
index e876c34..5fda0f5 100644
--- a/ndp.c
+++ b/ndp.c
@@ -158,7 +158,7 @@ struct ndp_ra {
 
 	unsigned char var[sizeof(struct opt_mtu) + sizeof(struct opt_rdnss) +
 			  sizeof(struct opt_dnssl)];
-} __attribute__((packed));
+} __attribute__((packed, aligned(__alignof__(struct in6_addr))));
 
 /**
  * struct ndp_ns - NDP Neighbor Solicitation (NS) message
@@ -168,7 +168,7 @@ struct ndp_ra {
 struct ndp_ns {
 	struct icmp6hdr ih;
 	struct in6_addr target_addr;
-} __attribute__((packed));
+} __attribute__((packed, aligned(__alignof__(struct in6_addr))));
 
 /**
  * ndp_send() - Send an NDP message
@@ -192,7 +192,7 @@ static void ndp_send(const struct ctx *c, const struct in6_addr *dst,
  * @addr:	IPv6 address to advertise
  */
 static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
-		   const void *addr)
+	    const struct in6_addr *addr)
 {
 	struct ndp_na na = {
 		.ih = {
@@ -202,6 +202,7 @@ static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
 			.icmp6_solicited	= 1,
 			.icmp6_override		= 1,
 		},
+		.target_addr = *addr,
 		.target_l2_addr = {
 			.header	= {
 				.type		= OPT_TARGET_L2_ADDR,
@@ -210,7 +211,6 @@ static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
 		}
 	};
 
-	memcpy(&na.target_addr, addr, sizeof(na.target_addr));
 	memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
 
 	ndp_send(c, dst, &na, sizeof(na));
@@ -242,6 +242,7 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
 			.valid_lifetime		= ~0U,
 			.pref_lifetime		= ~0U,
 		},
+		.prefix = c->ip6.addr,
 		.source_ll = {
 			.header = {
 				.type		= OPT_SRC_L2_ADDR,
@@ -251,8 +252,6 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
 	};
 	unsigned char *ptr = NULL;
 
-	memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix));
-
 	ptr = &ra.var[0];
 
 	if (c->mtu != -1) {
@@ -282,8 +281,7 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
 				.lifetime		= ~0U,
 			};
 			for (i = 0; i < n; i++) {
-				memcpy(&rdnss->dns[i], &c->ip6.dns[i],
-				       sizeof(rdnss->dns[i]));
+				rdnss->dns[i] = c->ip6.dns[i];
 			}
 			ptr += offsetof(struct opt_rdnss, dns) +
 				i * sizeof(rdnss->dns[0]);
-- 
@@ -158,7 +158,7 @@ struct ndp_ra {
 
 	unsigned char var[sizeof(struct opt_mtu) + sizeof(struct opt_rdnss) +
 			  sizeof(struct opt_dnssl)];
-} __attribute__((packed));
+} __attribute__((packed, aligned(__alignof__(struct in6_addr))));
 
 /**
  * struct ndp_ns - NDP Neighbor Solicitation (NS) message
@@ -168,7 +168,7 @@ struct ndp_ra {
 struct ndp_ns {
 	struct icmp6hdr ih;
 	struct in6_addr target_addr;
-} __attribute__((packed));
+} __attribute__((packed, aligned(__alignof__(struct in6_addr))));
 
 /**
  * ndp_send() - Send an NDP message
@@ -192,7 +192,7 @@ static void ndp_send(const struct ctx *c, const struct in6_addr *dst,
  * @addr:	IPv6 address to advertise
  */
 static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
-		   const void *addr)
+	    const struct in6_addr *addr)
 {
 	struct ndp_na na = {
 		.ih = {
@@ -202,6 +202,7 @@ static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
 			.icmp6_solicited	= 1,
 			.icmp6_override		= 1,
 		},
+		.target_addr = *addr,
 		.target_l2_addr = {
 			.header	= {
 				.type		= OPT_TARGET_L2_ADDR,
@@ -210,7 +211,6 @@ static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
 		}
 	};
 
-	memcpy(&na.target_addr, addr, sizeof(na.target_addr));
 	memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
 
 	ndp_send(c, dst, &na, sizeof(na));
@@ -242,6 +242,7 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
 			.valid_lifetime		= ~0U,
 			.pref_lifetime		= ~0U,
 		},
+		.prefix = c->ip6.addr,
 		.source_ll = {
 			.header = {
 				.type		= OPT_SRC_L2_ADDR,
@@ -251,8 +252,6 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
 	};
 	unsigned char *ptr = NULL;
 
-	memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix));
-
 	ptr = &ra.var[0];
 
 	if (c->mtu != -1) {
@@ -282,8 +281,7 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
 				.lifetime		= ~0U,
 			};
 			for (i = 0; i < n; i++) {
-				memcpy(&rdnss->dns[i], &c->ip6.dns[i],
-				       sizeof(rdnss->dns[i]));
+				rdnss->dns[i] = c->ip6.dns[i];
 			}
 			ptr += offsetof(struct opt_rdnss, dns) +
 				i * sizeof(rdnss->dns[0]);
-- 
2.47.0

[PATCH 5/6] ndp: Make route lifetime a #define

[David Gibson]

Currently we open-code the lifetime of the route we advertise via NDP to be
65535s (the maximum).  Change it to a #define.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ndp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ndp.c b/ndp.c
index 5fda0f5..c5fbccf 100644
--- a/ndp.c
+++ b/ndp.c
@@ -33,6 +33,8 @@
 #include "tap.h"
 #include "log.h"
 
+#define	RT_LIFETIME	65535
+
 #define RS	133
 #define RA	134
 #define NS	135
@@ -229,7 +231,7 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
 			.icmp6_code		= 0,
 			.icmp6_hop_limit	= 255,
 			/* RFC 8319 */
-			.icmp6_rt_lifetime	= htons_constant(65535),
+			.icmp6_rt_lifetime	= htons_constant(RT_LIFETIME),
 			.icmp6_addrconf_managed	= 1,
 		},
 		.prefix_info = {
-- 
@@ -33,6 +33,8 @@
 #include "tap.h"
 #include "log.h"
 
+#define	RT_LIFETIME	65535
+
 #define RS	133
 #define RA	134
 #define NS	135
@@ -229,7 +231,7 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
 			.icmp6_code		= 0,
 			.icmp6_hop_limit	= 255,
 			/* RFC 8319 */
-			.icmp6_rt_lifetime	= htons_constant(65535),
+			.icmp6_rt_lifetime	= htons_constant(RT_LIFETIME),
 			.icmp6_addrconf_managed	= 1,
 		},
 		.prefix_info = {
-- 
2.47.0

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

[David Gibson]

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
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 */
+
+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);
+
+	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);
 }
 
 /**
-- 
@@ -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);
 }
 
 /**
-- 
2.47.0

Re: [PATCH 3/6] ndp: Split out helpers for sending specific NDP message types

[Stefano Brivio]

Nits only:

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

> Currently the large ndp() function responds to all NDP messages we handle,
> both parsing the message as necessary and sending the response.  Split out
> the code to construct and send specific message types into ndp_na() (to
> send NA messages) and ndp_ra() (to send RA messages).
> 
> As well as breaking up an excessively large function, this is a first step
> to being able to send unsolicited NDP messages.
> 
> While we're there, remove a slighty ugly goto.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  ndp.c | 136 +++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 78 insertions(+), 58 deletions(-)
> 
> diff --git a/ndp.c b/ndp.c
> index fa1b67a..e876c34 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -186,16 +186,13 @@ static void ndp_send(const struct ctx *c, const struct in6_addr *dst,
>  }
>  
>  /**
> - * ndp() - Check for NDP solicitations, reply as needed
> + * ndp_na() - Send an NDP Neighbour Advertisement (NA) message
>   * @c:		Execution context
> - * @ih:		ICMPv6 header
> - * @saddr:	Source IPv6 address
> - * @p:		Packet pool
> - *
> - * Return: 0 if not handled here, 1 if handled, -1 on failure
> + * @dst:	IPv6 address to send the NA to
> + * @addr:	IPv6 address to advertise
>   */
> -int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> -	const struct in6_addr *saddr, const struct pool *p)
> +static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
> +		   const void *addr)
>  {
>  	struct ndp_na na = {
>  		.ih = {
> @@ -212,6 +209,20 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
>  			},
>  		}
>  	};
> +
> +	memcpy(&na.target_addr, addr, sizeof(na.target_addr));
> +	memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
> +
> +	ndp_send(c, dst, &na, sizeof(na));
> +}
> +
> +/**
> + * ndp_ra() - Send an NDP Router Advertisement (RA) message
> + * @c:		Execution context
> + * @dst:	IPv6 address to send the RA to
> + */
> +static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
> +{
>  	struct ndp_ra ra = {
>  		.ih = {
>  			.icmp6_type		= RA,
> @@ -238,58 +249,28 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
>  			},
>  		},
>  	};
> +	unsigned char *ptr = NULL;
>  
> -	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
> -		return 0;
> -
> -	if (c->no_ndp)
> -		return 1;
> -
> -	if (ih->icmp6_type == NS) {
> -		const struct ndp_ns *ns =
> -			packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL);
> +	memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix));
>  
> -		if (!ns)
> -			return -1;
> +	ptr = &ra.var[0];
>  
> -		if (IN6_IS_ADDR_UNSPECIFIED(saddr))
> -			return 1;
> -
> -		info("NDP: received NS, sending NA");
> -
> -		memcpy(&na.target_addr, &ns->target_addr,
> -		       sizeof(na.target_addr));
> -		memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
> +	if (c->mtu != -1) {
> +		struct opt_mtu *mtu = (struct opt_mtu *)ptr;
> +		*mtu = (struct opt_mtu) {
> +			.header = {
> +				.type		= OPT_MTU,
> +				.len		= 1,
> +			},
> +			.value			= htonl(c->mtu),
> +		};
> +		ptr += sizeof(struct opt_mtu);
> +	}
>  
> -		ndp_send(c, saddr, &na, sizeof(struct ndp_na));
> -	} else if (ih->icmp6_type == RS) {
> -		unsigned char *ptr = NULL;
> +	if (!c->no_dhcp_dns) {
>  		size_t dns_s_len = 0;
>  		int i, n;
>  
> -		if (c->no_ra)
> -			return 1;
> -
> -		info("NDP: received RS, sending RA");
> -		memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix));
> -
> -		ptr = &ra.var[0];
> -
> -		if (c->mtu != -1) {
> -			struct opt_mtu *mtu = (struct opt_mtu *)ptr;
> -			*mtu = (struct opt_mtu) {
> -				.header = {
> -					.type		= OPT_MTU,
> -					.len		= 1,
> -				},
> -				.value			= htonl(c->mtu),
> -			};
> -			ptr += sizeof(struct opt_mtu);
> -		}
> -
> -		if (c->no_dhcp_dns)
> -			goto dns_done;
> -
>  		for (n = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[n]); n++);
>  		if (n) {
>  			struct opt_rdnss *rdnss = (struct opt_rdnss *)ptr;
> @@ -305,7 +286,7 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
>  				       sizeof(rdnss->dns[i]));
>  			}
>  			ptr += offsetof(struct opt_rdnss, dns) +
> -			       i * sizeof(rdnss->dns[0]);
> +				i * sizeof(rdnss->dns[0]);

Unrelated change, less readable, probably from your new editor / clangd?

>  
>  			for (n = 0; *c->dns_search[n].n; n++)
>  				dns_s_len += strlen(c->dns_search[n].n) + 2;
> @@ -329,7 +310,7 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
>  				*(ptr++) = '.';
>  
>  				len = sizeof(dnssl->domains) -
> -				      (ptr - dnssl->domains);
> +					(ptr - dnssl->domains);

Same here.

>  
>  				strncpy((char *)ptr, c->dns_search[i].n, len);
>  				for (dot = (char *)ptr - 1; *dot; dot++) {
> @@ -343,11 +324,50 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
>  			memset(ptr, 0, 8 - dns_s_len % 8);	/* padding */
>  			ptr += 8 - dns_s_len % 8;
>  		}
> +	}
>  
> -dns_done:
> -		memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
> +	memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
>  
> -		ndp_send(c, saddr, &ra, ptr - (unsigned char *)&ra);
> +	ndp_send(c, dst, &ra, ptr - (unsigned char *)&ra);
> +}
> +
> +/**
> + * ndp() - Check for NDP solicitations, reply as needed
> + * @c:		Execution context
> + * @ih:		ICMPv6 header
> + * @saddr:	Source IPv6 address
> + * @p:		Packet pool
> + *
> + * Return: 0 if not handled here, 1 if handled, -1 on failure
> + */
> +int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> +	const struct in6_addr *saddr, const struct pool *p)
> +{
> +	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
> +		return 0;
> +
> +	if (c->no_ndp)
> +		return 1;
> +
> +	if (ih->icmp6_type == NS) {
> +		const struct ndp_ns *ns =
> +			packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL);

The assignment could go on its line here? We don't save any line this
way.

> +
> +		if (!ns)
> +			return -1;
> +
> +		if (IN6_IS_ADDR_UNSPECIFIED(saddr))
> +			return 1;
> +
> +		info("NDP: received NS, sending NA");
> +
> +		ndp_na(c, saddr, &ns->target_addr);
> +	} else if (ih->icmp6_type == RS) {
> +		if (c->no_ra)
> +			return 1;
> +
> +		info("NDP: received RS, sending RA");
> +		ndp_ra(c, saddr);
>  	}
>  
>  	return 1;

-- 
Stefano

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

[Stefano Brivio]

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

Re: [PATCH 3/6] ndp: Split out helpers for sending specific NDP message types

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7410 bytes --]

On Wed, Nov 13, 2024 at 02:14:12AM +0100, Stefano Brivio wrote:
> Nits only:
> 
> On Tue, 12 Nov 2024 15:06:15 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Currently the large ndp() function responds to all NDP messages we handle,
> > both parsing the message as necessary and sending the response.  Split out
> > the code to construct and send specific message types into ndp_na() (to
> > send NA messages) and ndp_ra() (to send RA messages).
> > 
> > As well as breaking up an excessively large function, this is a first step
> > to being able to send unsolicited NDP messages.
> > 
> > While we're there, remove a slighty ugly goto.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  ndp.c | 136 +++++++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 78 insertions(+), 58 deletions(-)
> > 
> > diff --git a/ndp.c b/ndp.c
> > index fa1b67a..e876c34 100644
> > --- a/ndp.c
> > +++ b/ndp.c
> > @@ -186,16 +186,13 @@ static void ndp_send(const struct ctx *c, const struct in6_addr *dst,
> >  }
> >  
> >  /**
> > - * ndp() - Check for NDP solicitations, reply as needed
> > + * ndp_na() - Send an NDP Neighbour Advertisement (NA) message
> >   * @c:		Execution context
> > - * @ih:		ICMPv6 header
> > - * @saddr:	Source IPv6 address
> > - * @p:		Packet pool
> > - *
> > - * Return: 0 if not handled here, 1 if handled, -1 on failure
> > + * @dst:	IPv6 address to send the NA to
> > + * @addr:	IPv6 address to advertise
> >   */
> > -int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> > -	const struct in6_addr *saddr, const struct pool *p)
> > +static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
> > +		   const void *addr)
> >  {
> >  	struct ndp_na na = {
> >  		.ih = {
> > @@ -212,6 +209,20 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> >  			},
> >  		}
> >  	};
> > +
> > +	memcpy(&na.target_addr, addr, sizeof(na.target_addr));
> > +	memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
> > +
> > +	ndp_send(c, dst, &na, sizeof(na));
> > +}
> > +
> > +/**
> > + * ndp_ra() - Send an NDP Router Advertisement (RA) message
> > + * @c:		Execution context
> > + * @dst:	IPv6 address to send the RA to
> > + */
> > +static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
> > +{
> >  	struct ndp_ra ra = {
> >  		.ih = {
> >  			.icmp6_type		= RA,
> > @@ -238,58 +249,28 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> >  			},
> >  		},
> >  	};
> > +	unsigned char *ptr = NULL;
> >  
> > -	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
> > -		return 0;
> > -
> > -	if (c->no_ndp)
> > -		return 1;
> > -
> > -	if (ih->icmp6_type == NS) {
> > -		const struct ndp_ns *ns =
> > -			packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL);
> > +	memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix));
> >  
> > -		if (!ns)
> > -			return -1;
> > +	ptr = &ra.var[0];
> >  
> > -		if (IN6_IS_ADDR_UNSPECIFIED(saddr))
> > -			return 1;
> > -
> > -		info("NDP: received NS, sending NA");
> > -
> > -		memcpy(&na.target_addr, &ns->target_addr,
> > -		       sizeof(na.target_addr));
> > -		memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
> > +	if (c->mtu != -1) {
> > +		struct opt_mtu *mtu = (struct opt_mtu *)ptr;
> > +		*mtu = (struct opt_mtu) {
> > +			.header = {
> > +				.type		= OPT_MTU,
> > +				.len		= 1,
> > +			},
> > +			.value			= htonl(c->mtu),
> > +		};
> > +		ptr += sizeof(struct opt_mtu);
> > +	}
> >  
> > -		ndp_send(c, saddr, &na, sizeof(struct ndp_na));
> > -	} else if (ih->icmp6_type == RS) {
> > -		unsigned char *ptr = NULL;
> > +	if (!c->no_dhcp_dns) {
> >  		size_t dns_s_len = 0;
> >  		int i, n;
> >  
> > -		if (c->no_ra)
> > -			return 1;
> > -
> > -		info("NDP: received RS, sending RA");
> > -		memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix));
> > -
> > -		ptr = &ra.var[0];
> > -
> > -		if (c->mtu != -1) {
> > -			struct opt_mtu *mtu = (struct opt_mtu *)ptr;
> > -			*mtu = (struct opt_mtu) {
> > -				.header = {
> > -					.type		= OPT_MTU,
> > -					.len		= 1,
> > -				},
> > -				.value			= htonl(c->mtu),
> > -			};
> > -			ptr += sizeof(struct opt_mtu);
> > -		}
> > -
> > -		if (c->no_dhcp_dns)
> > -			goto dns_done;
> > -
> >  		for (n = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[n]); n++);
> >  		if (n) {
> >  			struct opt_rdnss *rdnss = (struct opt_rdnss *)ptr;
> > @@ -305,7 +286,7 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> >  				       sizeof(rdnss->dns[i]));
> >  			}
> >  			ptr += offsetof(struct opt_rdnss, dns) +
> > -			       i * sizeof(rdnss->dns[0]);
> > +				i * sizeof(rdnss->dns[0]);
> 
> Unrelated change, less readable, probably from your new editor / clangd?

Curiously, no, I wasn't using Zed at the time.  That just happened
because I reindented the block a couple of times, and apparently my
emacs indent settings aren't *quite* the same as passt standard.

Anyway, fixed.

> >  
> >  			for (n = 0; *c->dns_search[n].n; n++)
> >  				dns_s_len += strlen(c->dns_search[n].n) + 2;
> > @@ -329,7 +310,7 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> >  				*(ptr++) = '.';
> >  
> >  				len = sizeof(dnssl->domains) -
> > -				      (ptr - dnssl->domains);
> > +					(ptr - dnssl->domains);
> 
> Same here.
> 
> >  
> >  				strncpy((char *)ptr, c->dns_search[i].n, len);
> >  				for (dot = (char *)ptr - 1; *dot; dot++) {
> > @@ -343,11 +324,50 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> >  			memset(ptr, 0, 8 - dns_s_len % 8);	/* padding */
> >  			ptr += 8 - dns_s_len % 8;
> >  		}
> > +	}
> >  
> > -dns_done:
> > -		memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
> > +	memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
> >  
> > -		ndp_send(c, saddr, &ra, ptr - (unsigned char *)&ra);
> > +	ndp_send(c, dst, &ra, ptr - (unsigned char *)&ra);
> > +}
> > +
> > +/**
> > + * ndp() - Check for NDP solicitations, reply as needed
> > + * @c:		Execution context
> > + * @ih:		ICMPv6 header
> > + * @saddr:	Source IPv6 address
> > + * @p:		Packet pool
> > + *
> > + * Return: 0 if not handled here, 1 if handled, -1 on failure
> > + */
> > +int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> > +	const struct in6_addr *saddr, const struct pool *p)
> > +{
> > +	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
> > +		return 0;
> > +
> > +	if (c->no_ndp)
> > +		return 1;
> > +
> > +	if (ih->icmp6_type == NS) {
> > +		const struct ndp_ns *ns =
> > +			packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL);
> 
> The assignment could go on its line here? We don't save any line this
> way.

Good point, done.

> > +
> > +		if (!ns)
> > +			return -1;
> > +
> > +		if (IN6_IS_ADDR_UNSPECIFIED(saddr))
> > +			return 1;
> > +
> > +		info("NDP: received NS, sending NA");
> > +
> > +		ndp_na(c, saddr, &ns->target_addr);
> > +	} else if (ih->icmp6_type == RS) {
> > +		if (c->no_ra)
> > +			return 1;
> > +
> > +		info("NDP: received RS, sending RA");
> > +		ndp_ra(c, saddr);
> >  	}
> >  
> >  	return 1;
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5831 bytes --]

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:
> 
> 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//

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 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.

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 = 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.

Good point, I'll add that.

> 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.

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.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Stefano Brivio]

On Wed, 13 Nov 2024 14:01:16 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> 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:
> > 
> > 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//  
> 
> 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 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.  
> 
> 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 = 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.  
> 
> Good point, I'll add that.
> 
> > 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.  
> 
> 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.

Well, but the seeds would be different (I would still factor
hash_secret in), so we would start out of sync right away, and we would
use a fine-grained time resolution to pick a coarser one, which adds
(enough) to the pseudorandomness.

> 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.

Oh, I thought it would still fit the recommendation, but now I read RFC
4861 6.2.4 again and it definitely doesn't.

> > 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.

That adds more maintenance burden than having a warning at this point,
so I'd rather not try to suppress anything. If the tcp_init_seq()-like
thing works, I'd be happy, otherwise let's leave this like it is.

-- 
Stefano