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

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

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 | 49 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH 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 | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/dhcp.c b/dhcp.c
index a06f143..2fe4a4d 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 {
@@ -154,17 +154,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 +264,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 +316,13 @@ int dhcp(const struct ctx *c, const struct pool *p)
 
 	offset += offsetof(struct msg, o);
 
+	for (i = 0; i < ARRAY_SIZE(opts); i++) {
+		if (!opts[i].slen)
+			opts[i].slen = -1;
+
+		opts[i].clen = -1;
+	}
+
 	while (opt_off + 2 < opt_len) {
 		const uint8_t *olen, *val;
 		uint8_t *type;
@@ -334,8 +344,9 @@ int dhcp(const struct ctx *c, const struct pool *p)
 	if (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].c[0] == DHCPREQUEST || opts[53].clen <= 0) {
+		info("%s: ack to request",
+		     (opts[53].clen <= 0) ? "DHCP" : "BOOTP");
 		opts[53].s[0] = DHCPACK;
 	} else {
 		return -1;
@@ -374,6 +385,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 {
@@ -154,17 +154,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 +264,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 +316,13 @@ int dhcp(const struct ctx *c, const struct pool *p)
 
 	offset += offsetof(struct msg, o);
 
+	for (i = 0; i < ARRAY_SIZE(opts); i++) {
+		if (!opts[i].slen)
+			opts[i].slen = -1;
+
+		opts[i].clen = -1;
+	}
+
 	while (opt_off + 2 < opt_len) {
 		const uint8_t *olen, *val;
 		uint8_t *type;
@@ -334,8 +344,9 @@ int dhcp(const struct ctx *c, const struct pool *p)
 	if (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].c[0] == DHCPREQUEST || opts[53].clen <= 0) {
+		info("%s: ack to request",
+		     (opts[53].clen <= 0) ? "DHCP" : "BOOTP");
 		opts[53].s[0] = DHCPACK;
 	} else {
 		return -1;
@@ -374,6 +385,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 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 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/dhcp.c b/dhcp.c
index 2fe4a4d..aa0ad96 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -342,8 +342,14 @@ int dhcp(const struct ctx *c, const struct pool *p)
 	}
 
 	if (opts[53].c[0] == DHCPDISCOVER) {
-		info("DHCP: offer to discover");
-		opts[53].s[0] = DHCPOFFER;
+		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].c[0] == DHCPREQUEST || opts[53].clen <= 0) {
 		info("%s: ack to request",
 		     (opts[53].clen <= 0) ? "DHCP" : "BOOTP");
-- 
@@ -342,8 +342,14 @@ int dhcp(const struct ctx *c, const struct pool *p)
 	}
 
 	if (opts[53].c[0] == DHCPDISCOVER) {
-		info("DHCP: offer to discover");
-		opts[53].s[0] = DHCPOFFER;
+		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].c[0] == DHCPREQUEST || opts[53].clen <= 0) {
 		info("%s: ack to request",
 		     (opts[53].clen <= 0) ? "DHCP" : "BOOTP");
-- 
2.43.0

[PATCH 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>
---
 dhcp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/dhcp.c b/dhcp.c
index aa0ad96..f1416ee 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -107,6 +107,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;
@@ -280,10 +282,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;
 
@@ -398,7 +400,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 = (struct in_addr){ 0xffffffff };
+	else
+		dst = c->ip4.addr;
+
+	tap_udp4_send(c, c->ip4.our_tap_addr, 67, dst, 68, m, dlen);
 
 	return 1;
 }
-- 
@@ -107,6 +107,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;
@@ -280,10 +282,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;
 
@@ -398,7 +400,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 = (struct in_addr){ 0xffffffff };
+	else
+		dst = c->ip4.addr;
+
+	tap_udp4_send(c, c->ip4.our_tap_addr, 67, dst, 68, m, dlen);
 
 	return 1;
 }
-- 
2.43.0

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

[David Gibson]

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

On Mon, Nov 25, 2024 at 01:04:21AM +0100, Stefano Brivio 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 | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index a06f143..2fe4a4d 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 {
> @@ -154,17 +154,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 +264,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 +316,13 @@ int dhcp(const struct ctx *c, const struct pool *p)
>  
>  	offset += offsetof(struct msg, o);
>  
> +	for (i = 0; i < ARRAY_SIZE(opts); i++) {
> +		if (!opts[i].slen)
> +			opts[i].slen = -1;
> +
> +		opts[i].clen = -1;
> +	}

Could this move to dhcp_init()?  I think there you wouldn't need test
and could unconditionally initialize all the lengths to -1 before
initializing the options we actually use.

>  	while (opt_off + 2 < opt_len) {
>  		const uint8_t *olen, *val;
>  		uint8_t *type;
> @@ -334,8 +344,9 @@ int dhcp(const struct ctx *c, const struct pool *p)
>  	if (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].c[0] == DHCPREQUEST || opts[53].clen <= 0) {
> +		info("%s: ack to request",
> +		     (opts[53].clen <= 0) ? "DHCP" : "BOOTP");

Should this be <= 0, or < 0?  i.e. Wouldn't even an empty option 53
indicate we're dealing with DHCP rather than BOOTP?

>  		opts[53].s[0] = DHCPACK;
>  	} else {
>  		return -1;
> @@ -374,6 +385,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));

-- 
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 2/3] dhcp: Introduce support for Rapid Commit (option 80, RFC 4039)

[David Gibson]

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

On Mon, Nov 25, 2024 at 01:04:22AM +0100, Stefano Brivio wrote:
> I'm trying to speed up and simplify IP address acquisition in muvm.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  dhcp.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index 2fe4a4d..aa0ad96 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -342,8 +342,14 @@ int dhcp(const struct ctx *c, const struct pool *p)
>  	}
>  
>  	if (opts[53].c[0] == DHCPDISCOVER) {
> -		info("DHCP: offer to discover");
> -		opts[53].s[0] = DHCPOFFER;
> +		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].c[0] == DHCPREQUEST || opts[53].clen <= 0) {
>  		info("%s: ack to request",
>  		     (opts[53].clen <= 0) ? "DHCP" : "BOOTP");

-- 
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 3/3] dhcp: Honour broadcast flag (RFC 2131, 4.1)

[David Gibson]

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

On Mon, Nov 25, 2024 at 01:04:23AM +0100, Stefano Brivio wrote:
> 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>
> ---
>  dhcp.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index aa0ad96..f1416ee 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -107,6 +107,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;
> @@ -280,10 +282,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;
>  
> @@ -398,7 +400,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 = (struct in_addr){ 0xffffffff };

It would be nice to add a symbolic constant ("in4addr_broadcast"?) to
ip.h for this.

> +	else
> +		dst = c->ip4.addr;
> +
> +	tap_udp4_send(c, c->ip4.our_tap_addr, 67, dst, 68, m, dlen);
>  
>  	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 1/3] dhcp: Use -1 as "missing option" length instead of 0

[Stefano Brivio]

On Mon, 25 Nov 2024 12:08:35 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Nov 25, 2024 at 01:04:21AM +0100, Stefano Brivio 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 | 27 ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/dhcp.c b/dhcp.c
> > index a06f143..2fe4a4d 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 {
> > @@ -154,17 +154,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 +264,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 +316,13 @@ int dhcp(const struct ctx *c, const struct pool *p)
> >  
> >  	offset += offsetof(struct msg, o);
> >  
> > +	for (i = 0; i < ARRAY_SIZE(opts); i++) {
> > +		if (!opts[i].slen)
> > +			opts[i].slen = -1;
> > +
> > +		opts[i].clen = -1;
> > +	}  
> 
> Could this move to dhcp_init()?  I think there you wouldn't need test
> and could unconditionally initialize all the lengths to -1 before
> initializing the options we actually use.

No, because dhcp_init() is run only once, and 'opts' at this point
represents the status from the previous run, so:

- we need to unconditionally reset all the 'clen' attributes which were
  set in the previous run

- we need to reset the 'slen' attributes for zero-length options (it's
  just option 80 at the moment) because we need to re-evaluate their
  inclusion. Sure, I could also clean things up at the end of any run,
  but this is more practical and robust

> >  	while (opt_off + 2 < opt_len) {
> >  		const uint8_t *olen, *val;
> >  		uint8_t *type;
> > @@ -334,8 +344,9 @@ int dhcp(const struct ctx *c, const struct pool *p)
> >  	if (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].c[0] == DHCPREQUEST || opts[53].clen <= 0) {
> > +		info("%s: ack to request",
> > +		     (opts[53].clen <= 0) ? "DHCP" : "BOOTP");  
> 
> Should this be <= 0, or < 0?  i.e. Wouldn't even an empty option 53
> indicate we're dealing with DHCP rather than BOOTP?

It should really be <= 0, preserving the existing behaviour, because if
option 53 is empty, we don't know what kind of DHCP message that is. We
know for sure that it's not a valid DHCP message, but it probably is a
valid BOOTP message (with a vendor extension).

This might look like speculation, but there are some half-DHCP
implementations from the 1990s which we can happily handle as BOOTP
clients, but not really as DHCP. After all the fun we had with wattcp32
and mTCP I would say it's not unlikely.

> >  		opts[53].s[0] = DHCPACK;
> >  	} else {
> >  		return -1;
> > @@ -374,6 +385,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));  

-- 
Stefano

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

[Stefano Brivio]

On Mon, 25 Nov 2024 12:11:34 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Nov 25, 2024 at 01:04:23AM +0100, Stefano Brivio wrote:
> > 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>
> > ---
> >  dhcp.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/dhcp.c b/dhcp.c
> > index aa0ad96..f1416ee 100644
> > --- a/dhcp.c
> > +++ b/dhcp.c
> > @@ -107,6 +107,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;
> > @@ -280,10 +282,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;
> >  
> > @@ -398,7 +400,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 = (struct in_addr){ 0xffffffff };  
> 
> It would be nice to add a symbolic constant ("in4addr_broadcast"?) to
> ip.h for this.

Sure, added as in4addr_broadcast.

> > +	else
> > +		dst = c->ip4.addr;
> > +
> > +	tap_udp4_send(c, c->ip4.our_tap_addr, 67, dst, 68, m, dlen);
> >  
> >  	return 1;
> >  }  

-- 
Stefano

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

[David Gibson]

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

On Mon, Nov 25, 2024 at 09:30:10AM +0100, Stefano Brivio wrote:
> On Mon, 25 Nov 2024 12:08:35 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Nov 25, 2024 at 01:04:21AM +0100, Stefano Brivio 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 | 27 ++++++++++++++++++++-------
> > >  1 file changed, 20 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/dhcp.c b/dhcp.c
> > > index a06f143..2fe4a4d 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 {
> > > @@ -154,17 +154,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 +264,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 +316,13 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > >  
> > >  	offset += offsetof(struct msg, o);
> > >  
> > > +	for (i = 0; i < ARRAY_SIZE(opts); i++) {
> > > +		if (!opts[i].slen)
> > > +			opts[i].slen = -1;
> > > +
> > > +		opts[i].clen = -1;
> > > +	}  
> > 
> > Could this move to dhcp_init()?  I think there you wouldn't need test
> > and could unconditionally initialize all the lengths to -1 before
> > initializing the options we actually use.
> 
> No, because dhcp_init() is run only once, and 'opts' at this point
> represents the status from the previous run, so:
> 
> - we need to unconditionally reset all the 'clen' attributes which were
>   set in the previous run

Ah, yes of course.

> - we need to reset the 'slen' attributes for zero-length options (it's
>   just option 80 at the moment) because we need to re-evaluate their
>   inclusion. Sure, I could also clean things up at the end of any run,
>   but this is more practical and robust

Hrm... I see that you need to do _something_ here, but the zero length
check still seems weird to me.  Ideally, this change would mean that
"no option" is always represented by -1 - but in a few places 0 sort
of still does as well.  There's no nice way in C to statically
initialise all the entries to -1, so I can see why we have to set
things to -1 with code in dhcp_init(), but I don't much like having
ambiguous representation past that point.

Shouldn't whether we reset a server side option be dependent only on
which option it is, not whether it was zero-length the last time?  The
fact that this only affects option 80, which happens to be zero-length
seems only correct by accident.

> 
> > >  	while (opt_off + 2 < opt_len) {
> > >  		const uint8_t *olen, *val;
> > >  		uint8_t *type;
> > > @@ -334,8 +344,9 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > >  	if (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].c[0] == DHCPREQUEST || opts[53].clen <= 0) {
> > > +		info("%s: ack to request",
> > > +		     (opts[53].clen <= 0) ? "DHCP" : "BOOTP");  
> > 
> > Should this be <= 0, or < 0?  i.e. Wouldn't even an empty option 53
> > indicate we're dealing with DHCP rather than BOOTP?
> 
> It should really be <= 0, preserving the existing behaviour, because if
> option 53 is empty, we don't know what kind of DHCP message that is. We
> know for sure that it's not a valid DHCP message, but it probably is a
> valid BOOTP message (with a vendor extension).

Ah, that makes sense.  A comment to that effect might be useful.

> This might look like speculation, but there are some half-DHCP
> implementations from the 1990s which we can happily handle as BOOTP
> clients, but not really as DHCP. After all the fun we had with wattcp32
> and mTCP I would say it's not unlikely.
> 
> > >  		opts[53].s[0] = DHCPACK;
> > >  	} else {
> > >  		return -1;
> > > @@ -374,6 +385,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));  
> 

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