[PATCH v4 0/3] dhcp: Add support for Rapid Commit, broadcast replies

[PATCH v4 0/3] dhcp: Add support for Rapid Commit, broadcast replies

[Stefano Brivio]

These are all changes coming from pending muvm experiments where I'm
trying to speed up and simplify the networking setup in the guest.

v4: In 1/3, don't swap the "BOOTP" and "DHCP" labels: we want to print
    "BOOTP" if option 53 isn't there or has zero length, not the other
    way around. Further, check that clen for option 53 is > 0 before
    checking if it's a DHCPDISCOVER. Given that DHCPDISCOVER is 1 (not
    0), this happened to already work for BOOTP, but it worked by
    mistake

v3: In 1/3, set 'slen' for options we won't send to -1 in dhcp_init(),
    and reset it for option 80 every time we handle a DHCP request. Add
    a comment about DHCP needing a valid message type (option 53)

v2: In 3/3, add in4addr_broadcast to ip.h, instead of open-coding it in
    dhcp()

Stefano Brivio (3):
  dhcp: Use -1 as "missing option" length instead of 0
  dhcp: Introduce support for Rapid Commit (option 80, RFC 4039)
  dhcp: Honour broadcast flag (RFC 2131, 4.1)

 dhcp.c | 53 +++++++++++++++++++++++++++++++++++++++++------------
 ip.h   |  3 +++
 2 files changed, 44 insertions(+), 12 deletions(-)

-- 
2.43.0

[PATCH v4 1/3] dhcp: Use -1 as "missing option" length instead of 0

[Stefano Brivio]

We want to add support for option 80 (Rapid Commit, RFC 4039), whose
length is 0.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 dhcp.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/dhcp.c b/dhcp.c
index a06f143..387aee3 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -36,9 +36,9 @@
 /**
  * struct opt - DHCP option
  * @sent:	Convenience flag, set while filling replies
- * @slen:	Length of option defined for server
+ * @slen:	Length of option defined for server, -1 if not going to be sent
  * @s:		Option payload from server
- * @clen:	Length of option received from client
+ * @clen:	Length of option received from client, -1 if not received
  * @c:		Option payload from client
  */
 struct opt {
@@ -68,6 +68,11 @@ static struct opt opts[255];
  */
 void dhcp_init(void)
 {
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(opts); i++)
+		opts[i].slen = -1;
+
 	opts[1]  = (struct opt) { 0, 4, {     0 }, 0, { 0 }, };	/* Mask */
 	opts[3]  = (struct opt) { 0, 4, {     0 }, 0, { 0 }, };	/* Router */
 	opts[51] = (struct opt) { 0, 4, {  0xff,
@@ -154,17 +159,17 @@ static int fill(struct msg *m)
 	 * option 53 at the beginning of the list.
 	 * Put it there explicitly, unless requested via option 55.
 	 */
-	if (!memchr(opts[55].c, 53, opts[55].clen))
+	if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen))
 		fill_one(m, 53, &offset);
 
 	for (i = 0; i < opts[55].clen; i++) {
 		o = opts[55].c[i];
-		if (opts[o].slen)
+		if (opts[o].slen != -1)
 			fill_one(m, o, &offset);
 	}
 
 	for (o = 0; o < 255; o++) {
-		if (opts[o].slen && !opts[o].sent)
+		if (opts[o].slen != -1 && !opts[o].sent)
 			fill_one(m, o, &offset);
 	}
 
@@ -264,6 +269,9 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
 						 ".\xc0");
 		}
 	}
+
+	if (!opts[119].slen)
+		opts[119].slen = -1;
 }
 
 /**
@@ -313,6 +321,9 @@ int dhcp(const struct ctx *c, const struct pool *p)
 
 	offset += offsetof(struct msg, o);
 
+	for (i = 0; i < ARRAY_SIZE(opts); i++)
+		opts[i].clen = -1;
+
 	while (opt_off + 2 < opt_len) {
 		const uint8_t *olen, *val;
 		uint8_t *type;
@@ -331,11 +342,12 @@ int dhcp(const struct ctx *c, const struct pool *p)
 		opt_off += *olen + 2;
 	}
 
-	if (opts[53].c[0] == DHCPDISCOVER) {
+	if (opts[53].clen > 0 && opts[53].c[0] == DHCPDISCOVER) {
 		info("DHCP: offer to discover");
 		opts[53].s[0] = DHCPOFFER;
-	} else if (opts[53].c[0] == DHCPREQUEST || !opts[53].clen) {
-		info("%s: ack to request", opts[53].clen ? "DHCP" : "BOOTP");
+	} else if (opts[53].clen <= 0 || opts[53].c[0] == DHCPREQUEST) {
+		info("%s: ack to request", /* DHCP needs a valid message type */
+		     (opts[53].clen <= 0) ? "BOOTP" : "DHCP");
 		opts[53].s[0] = DHCPACK;
 	} else {
 		return -1;
@@ -374,6 +386,8 @@ int dhcp(const struct ctx *c, const struct pool *p)
 		((struct in_addr *)opts[6].s)[i] = c->ip4.dns[i];
 		opts[6].slen += sizeof(uint32_t);
 	}
+	if (!opts[6].slen)
+		opts[6].slen = -1;
 
 	if (!c->no_dhcp_dns_search)
 		opt_set_dns_search(c, sizeof(m->o));
-- 
@@ -36,9 +36,9 @@
 /**
  * struct opt - DHCP option
  * @sent:	Convenience flag, set while filling replies
- * @slen:	Length of option defined for server
+ * @slen:	Length of option defined for server, -1 if not going to be sent
  * @s:		Option payload from server
- * @clen:	Length of option received from client
+ * @clen:	Length of option received from client, -1 if not received
  * @c:		Option payload from client
  */
 struct opt {
@@ -68,6 +68,11 @@ static struct opt opts[255];
  */
 void dhcp_init(void)
 {
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(opts); i++)
+		opts[i].slen = -1;
+
 	opts[1]  = (struct opt) { 0, 4, {     0 }, 0, { 0 }, };	/* Mask */
 	opts[3]  = (struct opt) { 0, 4, {     0 }, 0, { 0 }, };	/* Router */
 	opts[51] = (struct opt) { 0, 4, {  0xff,
@@ -154,17 +159,17 @@ static int fill(struct msg *m)
 	 * option 53 at the beginning of the list.
 	 * Put it there explicitly, unless requested via option 55.
 	 */
-	if (!memchr(opts[55].c, 53, opts[55].clen))
+	if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen))
 		fill_one(m, 53, &offset);
 
 	for (i = 0; i < opts[55].clen; i++) {
 		o = opts[55].c[i];
-		if (opts[o].slen)
+		if (opts[o].slen != -1)
 			fill_one(m, o, &offset);
 	}
 
 	for (o = 0; o < 255; o++) {
-		if (opts[o].slen && !opts[o].sent)
+		if (opts[o].slen != -1 && !opts[o].sent)
 			fill_one(m, o, &offset);
 	}
 
@@ -264,6 +269,9 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
 						 ".\xc0");
 		}
 	}
+
+	if (!opts[119].slen)
+		opts[119].slen = -1;
 }
 
 /**
@@ -313,6 +321,9 @@ int dhcp(const struct ctx *c, const struct pool *p)
 
 	offset += offsetof(struct msg, o);
 
+	for (i = 0; i < ARRAY_SIZE(opts); i++)
+		opts[i].clen = -1;
+
 	while (opt_off + 2 < opt_len) {
 		const uint8_t *olen, *val;
 		uint8_t *type;
@@ -331,11 +342,12 @@ int dhcp(const struct ctx *c, const struct pool *p)
 		opt_off += *olen + 2;
 	}
 
-	if (opts[53].c[0] == DHCPDISCOVER) {
+	if (opts[53].clen > 0 && opts[53].c[0] == DHCPDISCOVER) {
 		info("DHCP: offer to discover");
 		opts[53].s[0] = DHCPOFFER;
-	} else if (opts[53].c[0] == DHCPREQUEST || !opts[53].clen) {
-		info("%s: ack to request", opts[53].clen ? "DHCP" : "BOOTP");
+	} else if (opts[53].clen <= 0 || opts[53].c[0] == DHCPREQUEST) {
+		info("%s: ack to request", /* DHCP needs a valid message type */
+		     (opts[53].clen <= 0) ? "BOOTP" : "DHCP");
 		opts[53].s[0] = DHCPACK;
 	} else {
 		return -1;
@@ -374,6 +386,8 @@ int dhcp(const struct ctx *c, const struct pool *p)
 		((struct in_addr *)opts[6].s)[i] = c->ip4.dns[i];
 		opts[6].slen += sizeof(uint32_t);
 	}
+	if (!opts[6].slen)
+		opts[6].slen = -1;
 
 	if (!c->no_dhcp_dns_search)
 		opt_set_dns_search(c, sizeof(m->o));
-- 
2.43.0

[PATCH v4 2/3] dhcp: Introduce support for Rapid Commit (option 80, RFC 4039)

[Stefano Brivio]

I'm trying to speed up and simplify IP address acquisition in muvm.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 dhcp.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/dhcp.c b/dhcp.c
index 387aee3..732eb06 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -342,10 +342,17 @@ int dhcp(const struct ctx *c, const struct pool *p)
 		opt_off += *olen + 2;
 	}
 
+	opts[80].slen = -1;
 	if (opts[53].clen > 0 && opts[53].c[0] == DHCPDISCOVER) {
-		info("DHCP: offer to discover");
-		opts[53].s[0] = DHCPOFFER;
-	} else if (opts[53].clen <= 0 || opts[53].c[0] == DHCPREQUEST) {
+		if (opts[80].clen == -1) {
+			info("DHCP: offer to discover");
+			opts[53].s[0] = DHCPOFFER;
+		} else {
+			info("DHCP: ack to discover (Rapid Commit)");
+			opts[53].s[0] = DHCPACK;
+			opts[80].slen = 0;
+		}
+	} else if (opts[53].clen <= 0 && opts[53].c[0] == DHCPREQUEST) {
 		info("%s: ack to request", /* DHCP needs a valid message type */
 		     (opts[53].clen <= 0) ? "BOOTP" : "DHCP");
 		opts[53].s[0] = DHCPACK;
-- 
@@ -342,10 +342,17 @@ int dhcp(const struct ctx *c, const struct pool *p)
 		opt_off += *olen + 2;
 	}
 
+	opts[80].slen = -1;
 	if (opts[53].clen > 0 && opts[53].c[0] == DHCPDISCOVER) {
-		info("DHCP: offer to discover");
-		opts[53].s[0] = DHCPOFFER;
-	} else if (opts[53].clen <= 0 || opts[53].c[0] == DHCPREQUEST) {
+		if (opts[80].clen == -1) {
+			info("DHCP: offer to discover");
+			opts[53].s[0] = DHCPOFFER;
+		} else {
+			info("DHCP: ack to discover (Rapid Commit)");
+			opts[53].s[0] = DHCPACK;
+			opts[80].slen = 0;
+		}
+	} else if (opts[53].clen <= 0 && opts[53].c[0] == DHCPREQUEST) {
 		info("%s: ack to request", /* DHCP needs a valid message type */
 		     (opts[53].clen <= 0) ? "BOOTP" : "DHCP");
 		opts[53].s[0] = DHCPACK;
-- 
2.43.0

[PATCH v4 3/3] dhcp: Honour broadcast flag (RFC 2131, 4.1)

[Stefano Brivio]

It's widely considered a legacy option nowadays, and I've haven't seen
clients setting it since Windows 95, but it's convenient for a minimal
DHCP client not using raw IP sockets such as what I'm playing with for
muvm.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 dhcp.c | 12 ++++++++++--
 ip.h   |  3 +++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/dhcp.c b/dhcp.c
index 732eb06..665a325 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -112,6 +112,8 @@ struct msg {
 	uint32_t xid;
 	uint16_t secs;
 	uint16_t flags;
+#define FLAG_BROADCAST	htons_constant(0x8000)
+
 	uint32_t ciaddr;
 	struct in_addr yiaddr;
 	uint32_t siaddr;
@@ -285,10 +287,10 @@ int dhcp(const struct ctx *c, const struct pool *p)
 {
 	size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
 	char macstr[ETH_ADDRSTRLEN];
+	struct in_addr mask, dst;
 	const struct ethhdr *eh;
 	const struct iphdr *iph;
 	const struct udphdr *uh;
-	struct in_addr mask;
 	unsigned int i;
 	struct msg *m;
 
@@ -400,7 +402,13 @@ int dhcp(const struct ctx *c, const struct pool *p)
 		opt_set_dns_search(c, sizeof(m->o));
 
 	dlen = offsetof(struct msg, o) + fill(m);
-	tap_udp4_send(c, c->ip4.our_tap_addr, 67, c->ip4.addr, 68, m, dlen);
+
+	if (m->flags & FLAG_BROADCAST)
+		dst = in4addr_broadcast;
+	else
+		dst = c->ip4.addr;
+
+	tap_udp4_send(c, c->ip4.our_tap_addr, 67, dst, 68, m, dlen);
 
 	return 1;
 }
diff --git a/ip.h b/ip.h
index 0742612..1544dbf 100644
--- a/ip.h
+++ b/ip.h
@@ -101,4 +101,7 @@ static const struct in6_addr in6addr_ll_all_nodes = {
 	},
 };
 
+/* IPv4 Limited Broadcast (RFC 919, Section 7), 255.255.255.255 */
+static const struct in_addr in4addr_broadcast = { 0xffffffff };
+
 #endif /* IP_H */
-- 
@@ -101,4 +101,7 @@ static const struct in6_addr in6addr_ll_all_nodes = {
 	},
 };
 
+/* IPv4 Limited Broadcast (RFC 919, Section 7), 255.255.255.255 */
+static const struct in_addr in4addr_broadcast = { 0xffffffff };
+
 #endif /* IP_H */
-- 
2.43.0

Re: [PATCH v4 1/3] dhcp: Use -1 as "missing option" length instead of 0

[Stefano Brivio]

On Tue, 26 Nov 2024 07:34:08 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:

> We want to add support for option 80 (Rapid Commit, RFC 4039), whose
> length is 0.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  dhcp.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index a06f143..387aee3 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -36,9 +36,9 @@
>  /**
>   * struct opt - DHCP option
>   * @sent:	Convenience flag, set while filling replies
> - * @slen:	Length of option defined for server
> + * @slen:	Length of option defined for server, -1 if not going to be sent
>   * @s:		Option payload from server
> - * @clen:	Length of option received from client
> + * @clen:	Length of option received from client, -1 if not received
>   * @c:		Option payload from client
>   */
>  struct opt {
> @@ -68,6 +68,11 @@ static struct opt opts[255];
>   */
>  void dhcp_init(void)
>  {
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(opts); i++)
> +		opts[i].slen = -1;
> +
>  	opts[1]  = (struct opt) { 0, 4, {     0 }, 0, { 0 }, };	/* Mask */
>  	opts[3]  = (struct opt) { 0, 4, {     0 }, 0, { 0 }, };	/* Router */
>  	opts[51] = (struct opt) { 0, 4, {  0xff,
> @@ -154,17 +159,17 @@ static int fill(struct msg *m)
>  	 * option 53 at the beginning of the list.
>  	 * Put it there explicitly, unless requested via option 55.
>  	 */
> -	if (!memchr(opts[55].c, 53, opts[55].clen))
> +	if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen))
>  		fill_one(m, 53, &offset);
>  
>  	for (i = 0; i < opts[55].clen; i++) {
>  		o = opts[55].c[i];
> -		if (opts[o].slen)
> +		if (opts[o].slen != -1)
>  			fill_one(m, o, &offset);
>  	}
>  
>  	for (o = 0; o < 255; o++) {
> -		if (opts[o].slen && !opts[o].sent)
> +		if (opts[o].slen != -1 && !opts[o].sent)
>  			fill_one(m, o, &offset);
>  	}
>  
> @@ -264,6 +269,9 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
>  						 ".\xc0");
>  		}
>  	}
> +
> +	if (!opts[119].slen)
> +		opts[119].slen = -1;
>  }
>  
>  /**
> @@ -313,6 +321,9 @@ int dhcp(const struct ctx *c, const struct pool *p)
>  
>  	offset += offsetof(struct msg, o);
>  
> +	for (i = 0; i < ARRAY_SIZE(opts); i++)
> +		opts[i].clen = -1;
> +
>  	while (opt_off + 2 < opt_len) {
>  		const uint8_t *olen, *val;
>  		uint8_t *type;
> @@ -331,11 +342,12 @@ int dhcp(const struct ctx *c, const struct pool *p)
>  		opt_off += *olen + 2;
>  	}
>  
> -	if (opts[53].c[0] == DHCPDISCOVER) {
> +	if (opts[53].clen > 0 && opts[53].c[0] == DHCPDISCOVER) {
>  		info("DHCP: offer to discover");
>  		opts[53].s[0] = DHCPOFFER;
> -	} else if (opts[53].c[0] == DHCPREQUEST || !opts[53].clen) {
> -		info("%s: ack to request", opts[53].clen ? "DHCP" : "BOOTP");
> +	} else if (opts[53].clen <= 0 || opts[53].c[0] == DHCPREQUEST) {
> +		info("%s: ack to request", /* DHCP needs a valid message type */
> +		     (opts[53].clen <= 0) ? "BOOTP" : "DHCP");

Oops, posted before tests completed, this is broken again...

-- 
Stefano