ell.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH 01/15] dhcp-server: Add "authoritative" mode
@ 2021-08-02 14:04 Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 02/15] dhcp-server: Handle DHCPDECLINE for active leases Andrew Zaborowski
                   ` (14 more replies)
  0 siblings, 15 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

An authoritative DHCP server basically assumes that there are no other
DHCP servers on the network and replies to DHCPREQUEST messages
requesting IPs from outside of its subnet or with a wrong server ID
value.  This speeds up connections from clients who try to use IP
leases they have cached from other networks and who start their IP
configuration by trying DHCPREQUESTs for those leases.

The term is not defined in the DHCP standard but is used by various
implementations.  Default to authoritative being true.  When set to
false, handle a DHCPREQUEST message directed at other servers by taking
it as meaning the client is declining our own lease offer as mandated
by RFC 2131 on page 16.

Refactor the checks in DHCPREQUEST handling quoting parts of the RFC.
The new checks are stricter and I'm not preserving some of the checks
that didn't seem justified, for example in the
"if (server_id_opt || !lease) ... send_nak()" block it didn't seem to
make sense to only reply to clients who have no lease (offered or
active) and ignore those who do have a lease with a different IP
address, the opposite would make more sense.
---
 ell/dhcp-server.c | 117 +++++++++++++++++++++++++++++++++++++++-------
 ell/dhcp.h        |   2 +
 ell/ell.sym       |   1 +
 3 files changed, 103 insertions(+), 17 deletions(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index bae0f10..4c12327 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -87,6 +87,8 @@ struct l_dhcp_server {
 	struct dhcp_transport *transport;
 
 	struct l_acd *acd;
+
+	bool authoritative : 1;
 };
 
 #define MAC "%02x:%02x:%02x:%02x:%02x:%02x"
@@ -596,7 +598,8 @@ static void listener_event(const void *data, size_t len, void *user_data)
 	const void *v;
 	struct l_dhcp_lease *lease;
 	uint8_t type = 0;
-	uint32_t server_id_opt = 0;
+	bool server_id_opt = false;
+	bool server_id_match = true;
 	uint32_t requested_ip_opt = 0;
 
 	SERVER_DEBUG("");
@@ -612,12 +615,11 @@ static void listener_event(const void *data, size_t len, void *user_data)
 
 			break;
 		case L_DHCP_OPTION_SERVER_IDENTIFIER:
-			if (l == 4)
-				server_id_opt = l_get_u32(v);
-
-			if (server->address != server_id_opt)
-				return;
+			if (l != 4)
+				break;
 
+			server_id_opt = true;
+			server_id_match = (l_get_u32(v) == server->address);
 			break;
 		case L_DHCP_OPTION_REQUESTED_IP_ADDRESS:
 			if (l == 4)
@@ -640,6 +642,9 @@ static void listener_event(const void *data, size_t len, void *user_data)
 		SERVER_DEBUG("Received DISCOVER, requested IP "NIPQUAD_FMT,
 					NIPQUAD(requested_ip_opt));
 
+		if (!server_id_match)
+			break;
+
 		send_offer(server, message, lease, requested_ip_opt);
 
 		break;
@@ -647,27 +652,91 @@ static void listener_event(const void *data, size_t len, void *user_data)
 		SERVER_DEBUG("Received REQUEST, requested IP "NIPQUAD_FMT,
 				NIPQUAD(requested_ip_opt));
 
-		if (requested_ip_opt == 0) {
-			requested_ip_opt = message->ciaddr;
-			if (requested_ip_opt == 0)
+		/*
+		 * RFC2131 Section 3.5: "Those servers not selected by the
+		 * DHCPREQUEST message use the message as notification that
+		 * the client has declined that server's offer."
+		 */
+		if (!server_id_match) {
+			if (server->authoritative) {
+				send_nak(server, message);
+				break;
+			}
+
+			if (!lease || !lease->offering)
 				break;
-		}
 
-		if (lease && requested_ip_opt == lease->address) {
-			send_ack(server, message, lease->address);
+			remove_lease(server, lease);
 			break;
 		}
 
-		if (server_id_opt || !lease) {
-			send_nak(server, message);
+		/*
+		 * RFC2131 Section 3.5: "If the selected server is unable to
+		 * satisfy the DHCPREQUEST message (...), the server SHOULD
+		 * respond with a DHCPNAK message."
+		 *
+		 * But:
+		 * 4.3.2: "If the DHCP server has no record of this client,
+		 * then it MUST remain silent (...)"
+		 */
+		if (!lease) {
+			if (server_id_opt || server->authoritative)
+				send_nak(server, message);
+
 			break;
 		}
+
+		/*
+		 * 4.3.2: "If the DHCPREQUEST message contains a 'server
+		 * identifier' option, the message is in response to a
+		 * DHCPOFFER message.  Otherwise, the message is a request
+		 * to verify or extend an existing lease."
+		 */
+		if (server_id_opt && server_id_match) {
+			/*
+			 * Allow either no 'requested IP address' option or
+			 * a value identical with the one we offered because
+			 * the spec is unclear on whether it is to be
+			 * included:
+			 *
+			 * Section 4.3.2: "DHCPREQUEST generated during
+			 * SELECTING state: (...) 'requested IP address' MUST
+			 * be filled in with the yiaddr value from the chosen
+			 * DHCPOFFER."
+			 *
+			 * but section 3.5 suggests only in the INIT-REBOOT
+			 * state: "The 'requested IP address' option is to be
+			 * filled in only in a DHCPREQUEST message when the
+			 * client is verifying network parameters obtained
+			 * previously."
+			 */
+			if (!lease->offering ||
+					(requested_ip_opt &&
+					 requested_ip_opt != lease->address)) {
+				send_nak(server, message);
+				break;
+			}
+		} else {
+			/*
+			 * 3.5: "If a server receives a DHCPREQUEST message
+			 * with an invalid 'requested IP address', the server
+			 * SHOULD respond to the client with a DHCPNAK message"
+			 */
+			if (lease->offering ||
+					(requested_ip_opt &&
+					 requested_ip_opt != lease->address)) {
+				send_nak(server, message);
+				break;
+			}
+		}
+
+		send_ack(server, message, lease->address);
 		break;
 	case DHCP_MESSAGE_TYPE_DECLINE:
 		SERVER_DEBUG("Received DECLINE");
 
-		if (!server_id_opt || !requested_ip_opt || !lease ||
-				!lease->offering)
+		if (!server_id_opt || !server_id_match || !requested_ip_opt ||
+				!lease || !lease->offering)
 			break;
 
 		if (requested_ip_opt == lease->address)
@@ -677,7 +746,8 @@ static void listener_event(const void *data, size_t len, void *user_data)
 	case DHCP_MESSAGE_TYPE_RELEASE:
 		SERVER_DEBUG("Received RELEASE");
 
-		if (!server_id_opt || !lease || lease->offering)
+		if (!server_id_opt || !server_id_match || !lease ||
+				lease->offering)
 			break;
 
 		if (message->ciaddr == lease->address)
@@ -687,6 +757,9 @@ static void listener_event(const void *data, size_t len, void *user_data)
 	case DHCP_MESSAGE_TYPE_INFORM:
 		SERVER_DEBUG("Received INFORM");
 
+		if (!server_id_match)
+			break;
+
 		send_inform(server, message);
 		break;
 	}
@@ -732,6 +805,7 @@ LIB_EXPORT struct l_dhcp_server *l_dhcp_server_new(int ifindex)
 	server->expired_list = l_queue_new();
 
 	server->started = false;
+	server->authoritative = true;
 
 	server->lease_seconds = DEFAULT_DHCP_LEASE_SEC;
 	server->max_expired = MAX_EXPIRED_LEASES;
@@ -1053,6 +1127,15 @@ failed:
 	return false;
 }
 
+LIB_EXPORT void l_dhcp_server_set_authoritative(struct l_dhcp_server *server,
+						bool authoritative)
+{
+	if (unlikely(!server))
+		return;
+
+	server->authoritative = authoritative;
+}
+
 LIB_EXPORT struct l_dhcp_lease *l_dhcp_server_discover(
 						struct l_dhcp_server *server,
 						uint32_t requested_ip_opt,
diff --git a/ell/dhcp.h b/ell/dhcp.h
index 2804c78..830cbb1 100644
--- a/ell/dhcp.h
+++ b/ell/dhcp.h
@@ -141,6 +141,8 @@ bool l_dhcp_server_set_ip_address(struct l_dhcp_server *server,
 bool l_dhcp_server_set_netmask(struct l_dhcp_server *server, const char *mask);
 bool l_dhcp_server_set_gateway(struct l_dhcp_server *server, const char *ip);
 bool l_dhcp_server_set_dns(struct l_dhcp_server *server, char **dns);
+void l_dhcp_server_set_authoritative(struct l_dhcp_server *server,
+					bool authoritative);
 
 struct l_dhcp_lease *l_dhcp_server_discover(struct l_dhcp_server *server,
 						uint32_t requested_ip_opt,
diff --git a/ell/ell.sym b/ell/ell.sym
index 414c6d4..a781dea 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -259,6 +259,7 @@ global:
 	l_dhcp_server_set_netmask;
 	l_dhcp_server_set_gateway;
 	l_dhcp_server_set_dns;
+	l_dhcp_server_set_authoritative;
 	l_dhcp_server_discover;
 	l_dhcp_server_request;
 	l_dhcp_server_decline;
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 02/15] dhcp-server: Handle DHCPDECLINE for active leases
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
@ 2021-08-02 14:04 ` Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 03/15] dhcp-server: Respect client's broadcast flag Andrew Zaborowski
                   ` (13 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

RFC2131 Section 3.1 mentions a scenario where the client is supposed to
drop its lease using DHCPDECLINE after sending the DHCPREQUEST and
receiving the DHCPACK meaning that lease is no longer in the offering
state:

"5. The client receives the DHCPACK message with configuration
    parameters.  The client SHOULD perform a final check on the
    parameters [...].  If the client detects that the address is already
    in use (e.g., through the use of ARP), the client MUST send a
    DHCPDECLINE message..."
---
 ell/dhcp-server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 4c12327..0e18cf8 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -736,7 +736,7 @@ static void listener_event(const void *data, size_t len, void *user_data)
 		SERVER_DEBUG("Received DECLINE");
 
 		if (!server_id_opt || !server_id_match || !requested_ip_opt ||
-				!lease || !lease->offering)
+				!lease)
 			break;
 
 		if (requested_ip_opt == lease->address)
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 03/15] dhcp-server: Respect client's broadcast flag
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 02/15] dhcp-server: Handle DHCPDECLINE for active leases Andrew Zaborowski
@ 2021-08-02 14:04 ` Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 04/15] dhcp-server: Look up leases by client identifier option Andrew Zaborowski
                   ` (12 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

Follow notes in RFC2131 4.1, 4.3.2 and 4.4.4 when deciding whether we
can use unicast for a given message.  We want to prefer unicast when
possible but we should follow the spec on the broadcast flag and the
destination address.
---
 ell/dhcp-private.h |  3 ++
 ell/dhcp-server.c  | 90 +++++++++++++++++++++++++++++++++++-----------
 2 files changed, 72 insertions(+), 21 deletions(-)

diff --git a/ell/dhcp-private.h b/ell/dhcp-private.h
index c422322..f761162 100644
--- a/ell/dhcp-private.h
+++ b/ell/dhcp-private.h
@@ -66,6 +66,9 @@ enum dhcp_option_overload {
 #define DHCP_OPTION_MAXIMUM_MESSAGE_SIZE 57 /* Section 9.10 */
 #define DHCP_OPTION_CLIENT_IDENTIFIER 61 /* Section 9.14 */
 
+/* RFC 2131, Figure 2 */
+#define DHCP_FLAG_BROADCAST (1 << 15)
+
 /* RFC 2131, Figure 1 */
 struct dhcp_message {
 	uint8_t op;
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 0e18cf8..910aed7 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -420,6 +420,71 @@ static void server_message_init(struct l_dhcp_server *server,
 	reply->ciaddr = client_msg->ciaddr;
 }
 
+static bool server_message_send(struct l_dhcp_server *server,
+				struct dhcp_message *reply, size_t len,
+				uint8_t type)
+{
+	uint32_t daddr;
+	uint16_t dport;
+	const uint8_t *dest_mac;
+
+	/*
+	 * RFC2131 Section 4.1: "If the 'giaddr' field in a DHCP message from
+	 * a client is non-zero, the server sends any return messages to the
+	 * 'DHCP server' port on the BOOTP relay agent whose address appears
+	 * in 'giaddr'. If the 'giaddr' field is zero and the 'ciaddr' field
+	 * is nonzero, then the server unicasts DHCPOFFER and DHCPACK messages
+	 * to the address in 'ciaddr'.  If 'giaddr' is zero and 'ciaddr' is
+	 * zero, and the broadcast bit is set, then the server broadcasts
+	 * DHCPOFFER and DHCPACK messages to 0xffffffff. If the broadcast bit
+	 * is not set and 'giaddr' is zero and 'ciaddr' is zero, then the
+	 * server unicasts DHCPOFFER and DHCPACK messages to the client's
+	 * hardware address and 'yiaddr' address.  In all cases, when 'giaddr'
+	 * is zero, the server broadcasts any DHCPNAK messages to 0xffffffff."
+	 *
+	 * 4.3.2: "If 'giaddr' is set in the DHCPREQUEST message, the client
+	 * is on a different subnet.  The server MUST set the broadcast bit in
+	 * the DHCPNAK, so that the relay agent will broadcast the DHCPNAK to
+	 * the client, because the client may not have a correct network
+	 * address or subnet mask, and the client may not be answering ARP
+	 * requests."
+	 */
+	if (reply->giaddr) {
+		dport = DHCP_PORT_SERVER;
+		daddr = reply->giaddr;
+		dest_mac = reply->chaddr;
+
+		if (type == DHCP_MESSAGE_TYPE_NAK)
+			reply->flags |= L_CPU_TO_BE16(DHCP_FLAG_BROADCAST);
+	} else {
+		dport = DHCP_PORT_CLIENT;
+
+		if (type == DHCP_MESSAGE_TYPE_NAK) {
+			daddr = 0xffffffff;
+			dest_mac = MAC_BCAST_ADDR;
+		} else if (reply->ciaddr) {
+			daddr = reply->ciaddr;
+			dest_mac = reply->chaddr;
+		} else if (L_BE16_TO_CPU(reply->flags) & DHCP_FLAG_BROADCAST) {
+			daddr = 0xffffffff;
+			dest_mac = MAC_BCAST_ADDR;
+		} else {
+			daddr = reply->yiaddr;
+			dest_mac = reply->chaddr;
+		}
+	}
+
+	if (server->transport->l2_send(server->transport, server->address,
+					DHCP_PORT_SERVER, daddr, dport,
+					dest_mac, reply, len) < 0) {
+		SERVER_DEBUG("Failed to send %s",
+				_dhcp_message_type_to_string(type));
+		return false;
+	}
+
+	return true;
+}
+
 static void add_server_options(struct l_dhcp_server *server,
 				struct dhcp_message_builder *builder)
 {
@@ -491,11 +556,7 @@ static void send_offer(struct l_dhcp_server *server,
 	SERVER_DEBUG("Sending OFFER of "NIPQUAD_FMT " to "MAC,
 			NIPQUAD(reply->yiaddr),	MAC_STR(reply->chaddr));
 
-	if (server->transport->l2_send(server->transport, server->address,
-					DHCP_PORT_SERVER,
-					reply->ciaddr, DHCP_PORT_CLIENT,
-					reply->chaddr, reply, len) < 0)
-		SERVER_DEBUG("Failed to send OFFER");
+	server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_OFFER);
 }
 
 static void send_inform(struct l_dhcp_server *server,
@@ -515,11 +576,7 @@ static void send_inform(struct l_dhcp_server *server,
 
 	_dhcp_message_builder_finalize(&builder, &len);
 
-	if (server->transport->l2_send(server->transport, server->address,
-					DHCP_PORT_SERVER, reply->ciaddr,
-					DHCP_PORT_CLIENT, reply->chaddr,
-					reply, len) < 0)
-		SERVER_DEBUG("Failed to send INFORM");
+	server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_INFORM);
 }
 
 static void send_nak(struct l_dhcp_server *server,
@@ -537,11 +594,7 @@ static void send_nak(struct l_dhcp_server *server,
 
 	_dhcp_message_builder_finalize(&builder, &len);
 
-	if (server->transport->l2_send(server->transport, server->address,
-					DHCP_PORT_SERVER, reply->ciaddr,
-					DHCP_PORT_CLIENT, MAC_BCAST_ADDR,
-					reply, len) < 0)
-		SERVER_DEBUG("Failed to send NACK");
+	server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_NAK);
 }
 
 static void send_ack(struct l_dhcp_server *server,
@@ -574,13 +627,8 @@ static void send_ack(struct l_dhcp_server *server,
 
 	SERVER_DEBUG("Sending ACK to "NIPQUAD_FMT, NIPQUAD(reply->yiaddr));
 
-	if (server->transport->l2_send(server->transport, server->address,
-					DHCP_PORT_SERVER, reply->ciaddr,
-					DHCP_PORT_CLIENT,
-					reply->chaddr, reply, len) < 0) {
-		SERVER_DEBUG("Failed to send ACK");
+	if (!server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_ACK))
 		return;
-	}
 
 	lease = add_lease(server, false, reply->chaddr, reply->yiaddr);
 
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 04/15] dhcp-server: Look up leases by client identifier option
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 02/15] dhcp-server: Handle DHCPDECLINE for active leases Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 03/15] dhcp-server: Respect client's broadcast flag Andrew Zaborowski
@ 2021-08-02 14:04 ` Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 05/15] dhcp-server: Copy client identifier from the client message Andrew Zaborowski
                   ` (11 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

If the client identifier option is present in the client message, that
should be used for lease lookups, overriding the MAC value, according to
RFC2131.  Parse that option and save the value for lookups.
---
 ell/dhcp-lease.c   |  9 ++++++
 ell/dhcp-private.h |  1 +
 ell/dhcp-server.c  | 74 +++++++++++++++++++++++++++++++++-------------
 ell/dhcp.h         |  1 +
 4 files changed, 64 insertions(+), 21 deletions(-)

diff --git a/ell/dhcp-lease.c b/ell/dhcp-lease.c
index ffed6d6..c596c6b 100644
--- a/ell/dhcp-lease.c
+++ b/ell/dhcp-lease.c
@@ -49,6 +49,7 @@ void _dhcp_lease_free(struct l_dhcp_lease *lease)
 
 	l_free(lease->dns);
 	l_free(lease->domain_name);
+	l_free(lease->client_id);
 
 	l_free(lease);
 }
@@ -137,6 +138,14 @@ struct l_dhcp_lease *_dhcp_lease_parse_options(struct dhcp_message_iter *iter)
 			if (l_net_hostname_is_localhost(lease->domain_name))
 				goto error;
 
+			break;
+		case DHCP_OPTION_CLIENT_IDENTIFIER:
+			if (l < 1 || l > 253 || lease->client_id)
+				goto error;
+
+			lease->client_id = l_malloc(l + 1);
+			lease->client_id[0] = l;
+			memcpy(lease->client_id + 1, v, l);
 			break;
 		default:
 			break;
diff --git a/ell/dhcp-private.h b/ell/dhcp-private.h
index f761162..f9c0c6f 100644
--- a/ell/dhcp-private.h
+++ b/ell/dhcp-private.h
@@ -167,6 +167,7 @@ struct l_dhcp_lease {
 	char *domain_name;
 	/* for server */
 	uint8_t mac[6];
+	uint8_t *client_id;
 
 	/* set for an offered lease, but not ACK'ed */
 	bool offering : 1;
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 910aed7..e4d9d72 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -114,6 +114,15 @@ static bool is_expired_lease(const struct l_dhcp_lease *lease)
 	return !l_time_after(get_lease_expiry_time(lease), l_time_now());
 }
 
+static bool match_lease_client_id(const void *data, const void *user_data)
+{
+	const struct l_dhcp_lease *lease = data;
+	const uint8_t *client_id = user_data;
+
+	return lease->client_id &&
+		!memcmp(lease->client_id, client_id, client_id[0] + 1);
+}
+
 static bool match_lease_mac(const void *data, const void *user_data)
 {
 	const struct l_dhcp_lease *lease = data;
@@ -122,16 +131,21 @@ static bool match_lease_mac(const void *data, const void *user_data)
 	return !memcmp(lease->mac, mac, 6);
 }
 
-static struct l_dhcp_lease *find_lease_by_mac(struct l_dhcp_server *server,
+static struct l_dhcp_lease *find_lease_by_id(struct l_dhcp_server *server,
+						const uint8_t *client_id,
 						const uint8_t *mac)
 {
+	if (client_id)
+		return l_queue_find(server->lease_list, match_lease_client_id,
+					client_id);
+
 	return l_queue_find(server->lease_list, match_lease_mac, mac);
 }
 
 /* Clear the old lease and create the new one */
 static int get_lease(struct l_dhcp_server *server, uint32_t yiaddr,
-				const uint8_t *mac,
-				struct l_dhcp_lease **lease_out)
+			const uint8_t *client_id, const uint8_t *mac,
+			struct l_dhcp_lease **lease_out)
 {
 	struct l_dhcp_lease *lease;
 
@@ -150,7 +164,7 @@ static int get_lease(struct l_dhcp_server *server, uint32_t yiaddr,
 	if (l_memeqzero(mac, ETH_ALEN))
 		return -ENXIO;
 
-	lease = find_lease_by_mac(server, mac);
+	lease = find_lease_by_id(server, client_id, mac);
 
 	if (lease) {
 		l_queue_remove(server->lease_list, lease);
@@ -245,16 +259,17 @@ static void lease_expired_cb(struct l_timeout *timeout, void *user_data)
 }
 
 static struct l_dhcp_lease *add_lease(struct l_dhcp_server *server,
-					bool offering, const uint8_t *chaddr,
-					uint32_t yiaddr)
+					bool offering, const uint8_t *client_id,
+					const uint8_t *chaddr, uint32_t yiaddr)
 {
 	struct l_dhcp_lease *lease = NULL;
 	int ret;
 
-	ret = get_lease(server, yiaddr, chaddr, &lease);
+	ret = get_lease(server, yiaddr, client_id, chaddr, &lease);
 	if (ret != 0)
 		return NULL;
 
+	l_free(lease->client_id);
 	memset(lease, 0, sizeof(*lease));
 
 	memcpy(lease->mac, chaddr, ETH_ALEN);
@@ -262,6 +277,9 @@ static struct l_dhcp_lease *add_lease(struct l_dhcp_server *server,
 	lease->subnet_mask = server->netmask;
 	lease->router = server->gateway;
 
+	if (client_id)
+		lease->client_id = l_memdup(client_id, client_id[0] + 1);
+
 	lease->offering = offering;
 	lease->bound_time = l_time_now();
 
@@ -509,7 +527,8 @@ static void add_server_options(struct l_dhcp_server *server,
 
 static void send_offer(struct l_dhcp_server *server,
 			const struct dhcp_message *client_msg,
-			struct l_dhcp_lease *lease, uint32_t requested_ip)
+			struct l_dhcp_lease *lease, uint32_t requested_ip,
+			const uint8_t *client_id)
 {
 	struct dhcp_message_builder builder;
 	size_t len = sizeof(struct dhcp_message) + DHCP_MIN_OPTIONS_SIZE;
@@ -531,7 +550,8 @@ static void send_offer(struct l_dhcp_server *server,
 		return;
 	}
 
-	lease = add_lease(server, true, client_msg->chaddr, reply->yiaddr);
+	lease = add_lease(server, true, client_id, client_msg->chaddr,
+				reply->yiaddr);
 	if (!lease) {
 		SERVER_DEBUG("add_lease() failed");
 		return;
@@ -598,13 +618,14 @@ static void send_nak(struct l_dhcp_server *server,
 }
 
 static void send_ack(struct l_dhcp_server *server,
-			const struct dhcp_message *client_msg, uint32_t dest)
+			const struct dhcp_message *client_msg,
+			struct l_dhcp_lease *lease)
 {
 	struct dhcp_message_builder builder;
 	size_t len = sizeof(struct dhcp_message) + DHCP_MIN_OPTIONS_SIZE;
 	L_AUTO_FREE_VAR(struct dhcp_message *, reply);
 	uint32_t lease_time = L_CPU_TO_BE32(server->lease_seconds);
-	struct l_dhcp_lease *lease;
+	L_AUTO_FREE_VAR(uint8_t *, client_id) = l_steal_ptr(lease->client_id);
 
 	reply = (struct dhcp_message *) l_new(uint8_t, len);
 
@@ -612,7 +633,7 @@ static void send_ack(struct l_dhcp_server *server,
 
 	_dhcp_message_builder_init(&builder, reply, len, DHCP_MESSAGE_TYPE_ACK);
 
-	reply->yiaddr = dest;
+	reply->yiaddr = lease->address;
 
 	_dhcp_message_builder_append(&builder,
 					L_DHCP_OPTION_IP_ADDRESS_LEASE_TIME,
@@ -630,7 +651,8 @@ static void send_ack(struct l_dhcp_server *server,
 	if (!server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_ACK))
 		return;
 
-	lease = add_lease(server, false, reply->chaddr, reply->yiaddr);
+	lease = add_lease(server, false, client_id, reply->chaddr,
+				reply->yiaddr);
 
 	if (server->event_handler)
 		server->event_handler(server, L_DHCP_SERVER_EVENT_NEW_LEASE,
@@ -649,6 +671,7 @@ static void listener_event(const void *data, size_t len, void *user_data)
 	bool server_id_opt = false;
 	bool server_id_match = true;
 	uint32_t requested_ip_opt = 0;
+	L_AUTO_FREE_VAR(uint8_t *, client_id_opt) = NULL;
 
 	SERVER_DEBUG("");
 
@@ -674,13 +697,21 @@ static void listener_event(const void *data, size_t len, void *user_data)
 				requested_ip_opt = l_get_u32(v);
 
 			break;
+		case DHCP_OPTION_CLIENT_IDENTIFIER:
+			if (l < 1 || l > 253 || client_id_opt)
+				break;
+
+			client_id_opt = l_malloc(l + 1);
+			client_id_opt[0] = l;
+			memcpy(client_id_opt + 1, v, l);
+			break;
 		}
 	}
 
 	if (type == 0)
 		return;
 
-	lease = find_lease_by_mac(server, message->chaddr);
+	lease = find_lease_by_id(server, client_id_opt, message->chaddr);
 	if (!lease)
 		SERVER_DEBUG("No lease found for "MAC,
 					MAC_STR(message->chaddr));
@@ -693,8 +724,8 @@ static void listener_event(const void *data, size_t len, void *user_data)
 		if (!server_id_match)
 			break;
 
-		send_offer(server, message, lease, requested_ip_opt);
-
+		send_offer(server, message, lease, requested_ip_opt,
+				client_id_opt);
 		break;
 	case DHCP_MESSAGE_TYPE_REQUEST:
 		SERVER_DEBUG("Received REQUEST, requested IP "NIPQUAD_FMT,
@@ -778,7 +809,7 @@ static void listener_event(const void *data, size_t len, void *user_data)
 			}
 		}
 
-		send_ack(server, message, lease->address);
+		send_ack(server, message, lease);
 		break;
 	case DHCP_MESSAGE_TYPE_DECLINE:
 		SERVER_DEBUG("Received DECLINE");
@@ -1187,6 +1218,7 @@ LIB_EXPORT void l_dhcp_server_set_authoritative(struct l_dhcp_server *server,
 LIB_EXPORT struct l_dhcp_lease *l_dhcp_server_discover(
 						struct l_dhcp_server *server,
 						uint32_t requested_ip_opt,
+						const uint8_t *client_id,
 						const uint8_t *mac)
 {
 	struct l_dhcp_lease *lease;
@@ -1194,7 +1226,7 @@ LIB_EXPORT struct l_dhcp_lease *l_dhcp_server_discover(
 	SERVER_DEBUG("Requested IP " NIPQUAD_FMT " for " MAC,
 			NIPQUAD(requested_ip_opt), MAC_STR(mac));
 
-	if ((lease = find_lease_by_mac(server, mac)))
+	if ((lease = find_lease_by_id(server, client_id, mac)))
 		requested_ip_opt = lease->address;
 	else if (!check_requested_ip(server, requested_ip_opt)) {
 		requested_ip_opt = find_free_or_expired_ip(server, mac);
@@ -1205,7 +1237,7 @@ LIB_EXPORT struct l_dhcp_lease *l_dhcp_server_discover(
 		}
 	}
 
-	lease = add_lease(server, true, mac, requested_ip_opt);
+	lease = add_lease(server, true, client_id, mac, requested_ip_opt);
 	if (unlikely(!lease)) {
 		SERVER_DEBUG("add_lease() failed");
 		return NULL;
@@ -1225,7 +1257,7 @@ LIB_EXPORT bool l_dhcp_server_request(struct l_dhcp_server *server,
 	SERVER_DEBUG("Requested IP " NIPQUAD_FMT " for " MAC,
 			NIPQUAD(lease->address), MAC_STR(lease->mac));
 
-	lease = add_lease(server, false, lease->mac, lease->address);
+	lease = add_lease(server, false, NULL, lease->mac, lease->address);
 
 	if (server->event_handler)
 		server->event_handler(server, L_DHCP_SERVER_EVENT_NEW_LEASE,
@@ -1278,7 +1310,7 @@ LIB_EXPORT bool l_dhcp_server_lease_remove(struct l_dhcp_server *server,
 LIB_EXPORT void l_dhcp_server_expire_by_mac(struct l_dhcp_server *server,
 						const uint8_t *mac)
 {
-	struct l_dhcp_lease *lease = find_lease_by_mac(server, mac);
+	struct l_dhcp_lease *lease = find_lease_by_id(server, NULL, mac);
 
 	if (likely(lease))
 		lease_release(server, lease);
diff --git a/ell/dhcp.h b/ell/dhcp.h
index 830cbb1..58deb7e 100644
--- a/ell/dhcp.h
+++ b/ell/dhcp.h
@@ -146,6 +146,7 @@ void l_dhcp_server_set_authoritative(struct l_dhcp_server *server,
 
 struct l_dhcp_lease *l_dhcp_server_discover(struct l_dhcp_server *server,
 						uint32_t requested_ip_opt,
+						const uint8_t *client_id,
 						const uint8_t *mac);
 bool l_dhcp_server_request(struct l_dhcp_server *server,
 				struct l_dhcp_lease *lease);
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 05/15] dhcp-server: Copy client identifier from the client message
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
                   ` (2 preceding siblings ...)
  2021-08-02 14:04 ` [PATCH 04/15] dhcp-server: Look up leases by client identifier option Andrew Zaborowski
@ 2021-08-02 14:04 ` Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 06/15] dhcp-server: Save lease mac before calling add_lease Andrew Zaborowski
                   ` (10 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

As mandated by RFC 6842 copy the client identifier option from the
client message we're replying to into the reply message.
---
 ell/dhcp-server.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index e4d9d72..5c511f7 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -525,6 +525,16 @@ static void add_server_options(struct l_dhcp_server *server,
 	}
 }
 
+/* Copy the client identifier option from the client message per RFC6842 */
+static void copy_client_id(struct dhcp_message_builder *builder,
+				const uint8_t *client_id)
+{
+	if (client_id)
+		_dhcp_message_builder_append(builder,
+						DHCP_OPTION_CLIENT_IDENTIFIER,
+						client_id[0], client_id + 1);
+}
+
 static void send_offer(struct l_dhcp_server *server,
 			const struct dhcp_message *client_msg,
 			struct l_dhcp_lease *lease, uint32_t requested_ip,
@@ -570,6 +580,7 @@ static void send_offer(struct l_dhcp_server *server,
 					4, &server->address);
 
 	add_server_options(server, &builder);
+	copy_client_id(&builder, client_id);
 
 	_dhcp_message_builder_finalize(&builder, &len);
 
@@ -580,7 +591,8 @@ static void send_offer(struct l_dhcp_server *server,
 }
 
 static void send_inform(struct l_dhcp_server *server,
-				const struct dhcp_message *client_msg)
+				const struct dhcp_message *client_msg,
+				const uint8_t *client_id)
 {
 	struct dhcp_message_builder builder;
 	size_t len = sizeof(struct dhcp_message) + DHCP_MIN_OPTIONS_SIZE;
@@ -593,6 +605,7 @@ static void send_inform(struct l_dhcp_server *server,
 	_dhcp_message_builder_init(&builder, reply, len, DHCP_MESSAGE_TYPE_ACK);
 
 	add_server_options(server, &builder);
+	copy_client_id(&builder, client_id);
 
 	_dhcp_message_builder_finalize(&builder, &len);
 
@@ -600,7 +613,8 @@ static void send_inform(struct l_dhcp_server *server,
 }
 
 static void send_nak(struct l_dhcp_server *server,
-			const struct dhcp_message *client_msg)
+			const struct dhcp_message *client_msg,
+			const uint8_t *client_id)
 {
 	struct dhcp_message_builder builder;
 	size_t len = sizeof(struct dhcp_message) + DHCP_MIN_OPTIONS_SIZE;
@@ -611,7 +625,7 @@ static void send_nak(struct l_dhcp_server *server,
 	server_message_init(server, client_msg, reply);
 
 	_dhcp_message_builder_init(&builder, reply, len, DHCP_MESSAGE_TYPE_NAK);
-
+	copy_client_id(&builder, client_id);
 	_dhcp_message_builder_finalize(&builder, &len);
 
 	server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_NAK);
@@ -640,6 +654,7 @@ static void send_ack(struct l_dhcp_server *server,
 					4, &lease_time);
 
 	add_server_options(server, &builder);
+	copy_client_id(&builder, client_id);
 
 	_dhcp_message_builder_append(&builder, L_DHCP_OPTION_SERVER_IDENTIFIER,
 					4, &server->address);
@@ -738,7 +753,7 @@ static void listener_event(const void *data, size_t len, void *user_data)
 		 */
 		if (!server_id_match) {
 			if (server->authoritative) {
-				send_nak(server, message);
+				send_nak(server, message, client_id_opt);
 				break;
 			}
 
@@ -760,7 +775,7 @@ static void listener_event(const void *data, size_t len, void *user_data)
 		 */
 		if (!lease) {
 			if (server_id_opt || server->authoritative)
-				send_nak(server, message);
+				send_nak(server, message, client_id_opt);
 
 			break;
 		}
@@ -792,7 +807,7 @@ static void listener_event(const void *data, size_t len, void *user_data)
 			if (!lease->offering ||
 					(requested_ip_opt &&
 					 requested_ip_opt != lease->address)) {
-				send_nak(server, message);
+				send_nak(server, message, client_id_opt);
 				break;
 			}
 		} else {
@@ -804,7 +819,7 @@ static void listener_event(const void *data, size_t len, void *user_data)
 			if (lease->offering ||
 					(requested_ip_opt &&
 					 requested_ip_opt != lease->address)) {
-				send_nak(server, message);
+				send_nak(server, message, client_id_opt);
 				break;
 			}
 		}
@@ -839,7 +854,7 @@ static void listener_event(const void *data, size_t len, void *user_data)
 		if (!server_id_match)
 			break;
 
-		send_inform(server, message);
+		send_inform(server, message, client_id_opt);
 		break;
 	}
 }
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 06/15] dhcp-server: Save lease mac before calling add_lease
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
                   ` (3 preceding siblings ...)
  2021-08-02 14:04 ` [PATCH 05/15] dhcp-server: Copy client identifier from the client message Andrew Zaborowski
@ 2021-08-02 14:04 ` Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 07/15] dhcp-server: Ensure broadcast address is not selected Andrew Zaborowski
                   ` (9 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

add_lease will re-use the existing lease struct but will memzero it and
the fill in the data.  We can't pass lease->mac to add_lease() because
the mac will be zeroed together with the rest of the structure and will
be all-zeros when add_lease() tries to copy the new value from the
pointer we passed.
---
 ell/dhcp-server.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 5c511f7..bdf43d7 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -1266,13 +1266,16 @@ LIB_EXPORT struct l_dhcp_lease *l_dhcp_server_discover(
 LIB_EXPORT bool l_dhcp_server_request(struct l_dhcp_server *server,
 					struct l_dhcp_lease *lease)
 {
+	uint8_t mac[ETH_ALEN];
+
 	if (unlikely(!lease))
 		return false;
 
 	SERVER_DEBUG("Requested IP " NIPQUAD_FMT " for " MAC,
 			NIPQUAD(lease->address), MAC_STR(lease->mac));
 
-	lease = add_lease(server, false, NULL, lease->mac, lease->address);
+	memcpy(mac, lease->mac, ETH_ALEN);
+	lease = add_lease(server, false, NULL, mac, lease->address);
 
 	if (server->event_handler)
 		server->event_handler(server, L_DHCP_SERVER_EVENT_NEW_LEASE,
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 07/15] dhcp-server: Ensure broadcast address is not selected
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
                   ` (4 preceding siblings ...)
  2021-08-02 14:04 ` [PATCH 06/15] dhcp-server: Save lease mac before calling add_lease Andrew Zaborowski
@ 2021-08-02 14:04 ` Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 08/15] dhcp-server: Reuse leases in find_free_or_expired_ip Andrew Zaborowski
                   ` (8 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

In find_free_or_expired_ip, get_lease and check_requested_ip we'd allow
addresses up to and including server->end_ip meaning that in some
situations the broadcast address might have been allowed as a valid
client address.  find_free_or_expired_ip would check that the address
didn't end in .255 but that is only correct for a 24-bit netmask.  Now
set start_ip to be at least one address above the subnet address and
end_ip to be at lesat one address below the broadcast address.
---
 ell/dhcp-server.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index bdf43d7..a4e7002 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -971,8 +971,8 @@ LIB_EXPORT bool l_dhcp_server_start(struct l_dhcp_server *server)
 	 */
 	if (!server->start_ip) {
 		server->start_ip = ntohl(server->address) + 1;
-		server->end_ip = ntohl(server->address) |
-			(~ntohl(server->netmask));
+		server->end_ip = (ntohl(server->address) |
+			(~ntohl(server->netmask))) - 1;
 	} else {
 		if ((server->start_ip ^ ntohl(server->address)) &
 				ntohl(server->netmask))
@@ -981,17 +981,29 @@ LIB_EXPORT bool l_dhcp_server_start(struct l_dhcp_server *server)
 		if ((server->end_ip ^ ntohl(server->address)) &
 				ntohl(server->netmask))
 			return false;
-	}
 
-	/*
-	 * We skip over IPs ending in 0 or 255 when selecting a free address
-	 * later on but make sure end_ip is not 0xffffffff so we can use
-	 * "<= server->end_ip" safely in the loop condition.
-	 */
-	if ((server->end_ip & 0xff) == 255)
-		server->end_ip--;
+		/*
+		 * Directly ensure the [start_ip, end_ip] range doesn't
+		 * include the subnet address or the broadcast address so that
+		 * we have fewer checks to make when selecting a free address
+		 * from that range.  Additionally this ensures end_ip is not
+		 * 0xffffffff so we can use the condition "<= server->end_ip"
+		 * safely on uint32_t values.
+		 * In find_free_or_expired_ip we skip over IPs ending in .0 or
+		 * .255 even for netmasks other than 24-bit just to avoid
+		 * generating addresses that could look suspicious even if
+		 * they're legal.  We don't exclude these addresses when
+		 * explicitly requested by the client, i.e. in
+		 * check_requested_ip.
+		 */
+		if ((server->start_ip & (~ntohl(server->netmask))) == 0)
+			server->start_ip++;
+
+		if ((server->end_ip | ntohl(server->netmask)) == 0xffffffff)
+			server->end_ip--;
+	}
 
-	if (server->start_ip > server->end_ip)
+	if (server->start_ip >= server->end_ip)
 		return false;
 
 	if (!server->ifname) {
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 08/15] dhcp-server: Reuse leases in find_free_or_expired_ip
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
                   ` (5 preceding siblings ...)
  2021-08-02 14:04 ` [PATCH 07/15] dhcp-server: Ensure broadcast address is not selected Andrew Zaborowski
@ 2021-08-02 14:04 ` Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 09/15] dhcp-server: Refactor lease lookup Andrew Zaborowski
                   ` (7 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

If a 'safe_mac' parameter is passed to find_free_or_expired_ip and a
lease matching that MAC is found in expired_list, return that lease's
address over the oldest lease's or an unused address.
---
 ell/dhcp-server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index a4e7002..eb8fcd7 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -411,7 +411,7 @@ static uint32_t find_free_or_expired_ip(struct l_dhcp_server *server,
 			continue;
 
 		lease = find_lease_by_ip(server->expired_list, ip_nl);
-		if (lease)
+		if (lease && memcmp(lease->mac, safe_mac, ETH_ALEN))
 			continue;
 
 		if (arp_check(ip_nl, safe_mac))
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 09/15] dhcp-server: Refactor lease lookup
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
                   ` (6 preceding siblings ...)
  2021-08-02 14:04 ` [PATCH 08/15] dhcp-server: Reuse leases in find_free_or_expired_ip Andrew Zaborowski
@ 2021-08-02 14:04 ` Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 10/15] dhcp-server: Allow reactivating expired leases Andrew Zaborowski
                   ` (6 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

A few places in the code assume that there can be only one lease per
client MAC address (or recently client ID).  The spec doesn't actually
say this and says that a lease is identified by the client-id/mac plus
the IP:

"The combination of 'client identifier' or 'chaddr' and assigned network
address constitute a unique identifier for the client's lease and are
used by both the client and server to identify a lease referred to in
any DHCP messages."

Start identifying leases this way.  Don't actually add any way for a
client to obtain a second lease -- we still ignore the 'requested IP'
option in DHCPDISCOVER if the client already has a lease.

This currently solves an unrelated problem: there was no upper bound
on server->expired_list size.  When handling DHCPDISCOVER we'd free the
oldest lease in server->expired_list only under some conditions,
otherwise we might have been allocating an IP that was already in the
expired list and once that new lease expired there would be duplicate IP
addresses in server->expired_list and it could keep growing.  Now when
we try to re-use a struct l_dhcp_lease in get_lease() we look up by IP
instead of by the MAC and we look at both server->lease_list and
server->expired_list.  This guarantees IPs are unique across the two
lists and their sizes are bounded by the pool size.
---
 ell/dhcp-server.c | 98 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 74 insertions(+), 24 deletions(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index eb8fcd7..0183c20 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -131,15 +131,49 @@ static bool match_lease_mac(const void *data, const void *user_data)
 	return !memcmp(lease->mac, mac, 6);
 }
 
-static struct l_dhcp_lease *find_lease_by_id(struct l_dhcp_server *server,
+static bool match_lease_ip(const void *data, const void *user_data)
+{
+	const struct l_dhcp_lease *lease = data;
+
+	return lease->address == L_PTR_TO_UINT(user_data);
+}
+
+static struct l_dhcp_lease *find_lease_by_ip(struct l_queue *lease_list,
+						uint32_t nip)
+{
+	return l_queue_find(lease_list, match_lease_ip, L_UINT_TO_PTR(nip));
+}
+
+static struct l_dhcp_lease *find_lease_by_id(struct l_queue *lease_list,
 						const uint8_t *client_id,
 						const uint8_t *mac)
 {
 	if (client_id)
-		return l_queue_find(server->lease_list, match_lease_client_id,
+		return l_queue_find(lease_list, match_lease_client_id,
 					client_id);
 
-	return l_queue_find(server->lease_list, match_lease_mac, mac);
+	return l_queue_find(lease_list, match_lease_mac, mac);
+}
+
+static struct l_dhcp_lease *find_lease_by_id_and_ip(struct l_queue *lease_list,
+						const uint8_t *client_id,
+						const uint8_t *mac,
+						uint32_t ip)
+{
+	struct l_dhcp_lease *lease = find_lease_by_ip(lease_list, ip);
+
+	if (!lease)
+		return NULL;
+
+	if (client_id) {
+		if (!match_lease_client_id(lease, client_id))
+			return NULL;
+	} else {
+		if (!match_lease_mac(lease, mac))
+			return NULL;
+	}
+
+	return lease;
 }
 
 /* Clear the old lease and create the new one */
@@ -164,13 +198,17 @@ static int get_lease(struct l_dhcp_server *server, uint32_t yiaddr,
 	if (l_memeqzero(mac, ETH_ALEN))
 		return -ENXIO;
 
-	lease = find_lease_by_id(server, client_id, mac);
-
+	lease = find_lease_by_ip(server->lease_list, yiaddr);
 	if (lease) {
 		l_queue_remove(server->lease_list, lease);
-
 		*lease_out = lease;
+		return 0;
+	}
 
+	lease = find_lease_by_ip(server->expired_list, yiaddr);
+	if (lease && lease->address == yiaddr) {
+		l_queue_remove(server->expired_list, lease);
+		*lease_out = lease;
 		return 0;
 	}
 
@@ -332,19 +370,6 @@ static void lease_release(struct l_dhcp_server *server,
 	set_next_expire_timer(server, lease);
 }
 
-static bool match_lease_ip(const void *data, const void *user_data)
-{
-	const struct l_dhcp_lease *lease = data;
-
-	return lease->address == L_PTR_TO_UINT(user_data);
-}
-
-static struct l_dhcp_lease *find_lease_by_ip(struct l_queue *lease_list,
-						uint32_t nip)
-{
-	return l_queue_find(lease_list, match_lease_ip, L_UINT_TO_PTR(nip));
-}
-
 static bool check_requested_ip(struct l_dhcp_server *server,
 				uint32_t requested_nip)
 {
@@ -726,7 +751,15 @@ static void listener_event(const void *data, size_t len, void *user_data)
 	if (type == 0)
 		return;
 
-	lease = find_lease_by_id(server, client_id_opt, message->chaddr);
+	if (requested_ip_opt)
+		lease = find_lease_by_id_and_ip(server->lease_list,
+						client_id_opt, message->chaddr,
+						requested_ip_opt);
+
+	if (!requested_ip_opt || !lease)
+		lease = find_lease_by_id(server->lease_list, client_id_opt,
+						message->chaddr);
+
 	if (!lease)
 		SERVER_DEBUG("No lease found for "MAC,
 					MAC_STR(message->chaddr));
@@ -1253,7 +1286,7 @@ LIB_EXPORT struct l_dhcp_lease *l_dhcp_server_discover(
 	SERVER_DEBUG("Requested IP " NIPQUAD_FMT " for " MAC,
 			NIPQUAD(requested_ip_opt), MAC_STR(mac));
 
-	if ((lease = find_lease_by_id(server, client_id, mac)))
+	if ((lease = find_lease_by_id(server->lease_list, client_id, mac)))
 		requested_ip_opt = lease->address;
 	else if (!check_requested_ip(server, requested_ip_opt)) {
 		requested_ip_opt = find_free_or_expired_ip(server, mac);
@@ -1337,11 +1370,28 @@ LIB_EXPORT bool l_dhcp_server_lease_remove(struct l_dhcp_server *server,
 	return true;
 }
 
+struct dhcp_expire_by_mac_data {
+	struct l_dhcp_server *server;
+	const uint8_t *mac;
+};
+
+static bool dhcp_expire_by_mac(void *data, void *user_data)
+{
+	struct l_dhcp_lease *lease = data;
+	struct dhcp_expire_by_mac_data *expire_data = user_data;
+
+	if (!match_lease_mac(lease, expire_data->mac))
+		return false;
+
+	lease_release(expire_data->server, lease);
+	return true;
+}
+
 LIB_EXPORT void l_dhcp_server_expire_by_mac(struct l_dhcp_server *server,
 						const uint8_t *mac)
 {
-	struct l_dhcp_lease *lease = find_lease_by_id(server, NULL, mac);
+	struct dhcp_expire_by_mac_data expire_data = { server, mac };
 
-	if (likely(lease))
-		lease_release(server, lease);
+	l_queue_foreach_remove(server->lease_list, dhcp_expire_by_mac,
+				&expire_data);
 }
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 10/15] dhcp-server: Allow reactivating expired leases
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
                   ` (7 preceding siblings ...)
  2021-08-02 14:04 ` [PATCH 09/15] dhcp-server: Refactor lease lookup Andrew Zaborowski
@ 2021-08-02 14:04 ` Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 11/15] dhcp: Support RFC4039 Rapid Commit Andrew Zaborowski
                   ` (5 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

Allow clients to reactivate leases from server->expired_list by using a
DHCPREQUEST.  This will allow users of the API to mark addressses owned
by disconnecting clients as okay to be recycled by using
l_dhcp_server_expire_by_mac().  If the client that saves its leases
reconnects before we run out of address space, they will reactivate
their address be going through the standard "reusing a previously
allocated network address" procedure (DHCPREQUEST/DHCPACK).  This may
make sense for wireless access points where we know exactly when a
client disconnects, and where a combination of poor signal strength and
MAC randomization (and no Client ID support) could lead to a lot of
addresses being allocated quickly.  Combined with IP allocation during
the 802.11 4-Way Handshake, unused IPs could be handed out even faster.
---
 ell/dhcp-server.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 0183c20..506bb7d 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -797,6 +797,17 @@ static void listener_event(const void *data, size_t len, void *user_data)
 			break;
 		}
 
+		/*
+		 * As an extension, check if we have an expired lease matching
+		 * the requested IP and the client ID/mac and if so, allow the
+		 * lease to be re-activated.
+		 */
+		if (!lease && requested_ip_opt)
+			lease = find_lease_by_id_and_ip(server->expired_list,
+							client_id_opt,
+							message->chaddr,
+							requested_ip_opt);
+
 		/*
 		 * RFC2131 Section 3.5: "If the selected server is unable to
 		 * satisfy the DHCPREQUEST message (...), the server SHOULD
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 11/15] dhcp: Support RFC4039 Rapid Commit
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
                   ` (8 preceding siblings ...)
  2021-08-02 14:04 ` [PATCH 10/15] dhcp-server: Allow reactivating expired leases Andrew Zaborowski
@ 2021-08-02 14:04 ` Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 12/15] dhcp-server: " Andrew Zaborowski
                   ` (4 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

Use the RFC4039 Rapid Commit option in the DHCPDISCOVERY to attempt to
get the server to directly reply with a DHCPACK, i.e. skipping the
DHCPOFFER and DHCPREQUEST roundtrip.

Currently enabled unconditionally, the support can be made optional if
needed in the future.
---
 ell/dhcp-private.h |  1 +
 ell/dhcp-util.c    |  4 +++-
 ell/dhcp.c         | 14 ++++++++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/ell/dhcp-private.h b/ell/dhcp-private.h
index f9c0c6f..5a09f5e 100644
--- a/ell/dhcp-private.h
+++ b/ell/dhcp-private.h
@@ -65,6 +65,7 @@ enum dhcp_option_overload {
 #define DHCP_OPTION_PARAMETER_REQUEST_LIST 55 /* Section 9.8 */
 #define DHCP_OPTION_MAXIMUM_MESSAGE_SIZE 57 /* Section 9.10 */
 #define DHCP_OPTION_CLIENT_IDENTIFIER 61 /* Section 9.14 */
+#define DHCP_OPTION_RAPID_COMMIT 80 /* RFC 4039 Section 4 */
 
 /* RFC 2131, Figure 2 */
 #define DHCP_FLAG_BROADCAST (1 << 15)
diff --git a/ell/dhcp-util.c b/ell/dhcp-util.c
index d4d715a..e5c88b2 100644
--- a/ell/dhcp-util.c
+++ b/ell/dhcp-util.c
@@ -73,7 +73,9 @@ bool _dhcp_message_builder_append(struct dhcp_message_builder *builder,
 
 		builder->pos[0] = code;
 		builder->pos[1] = optlen;
-		memcpy(builder->pos + 2, optval, optlen);
+
+		if (optlen)
+			memcpy(builder->pos + 2, optval, optlen);
 
 		builder->pos += optlen + 2;
 		break;
diff --git a/ell/dhcp.c b/ell/dhcp.c
index 115325e..2efbf39 100644
--- a/ell/dhcp.c
+++ b/ell/dhcp.c
@@ -361,6 +361,8 @@ static int dhcp_client_send_discover(struct l_dhcp_client *client)
 						client->hostname))
 			return -EINVAL;
 
+	_dhcp_message_builder_append(&builder, DHCP_OPTION_RAPID_COMMIT,
+					0, "");
 	_dhcp_message_builder_finalize(&builder, &len);
 
 	return client->transport->l2_send(client->transport,
@@ -815,6 +817,17 @@ static void dhcp_client_rx_message(const void *data, size_t len, void *userdata)
 	case DHCP_STATE_INIT:
 		return;
 	case DHCP_STATE_SELECTING:
+		if (msg_type == DHCP_MESSAGE_TYPE_ACK) {
+			_dhcp_message_iter_init(&iter, message, len);
+
+			while (_dhcp_message_iter_next(&iter, &t, &l, &v))
+				if (t == DHCP_OPTION_RAPID_COMMIT) {
+					CLIENT_ENTER_STATE(
+							DHCP_STATE_REQUESTING);
+					goto receive_rapid_commit;
+				}
+		}
+
 		if (msg_type != DHCP_MESSAGE_TYPE_OFFER)
 			return;
 
@@ -835,6 +848,7 @@ static void dhcp_client_rx_message(const void *data, size_t len, void *userdata)
 	case DHCP_STATE_REQUESTING:
 	case DHCP_STATE_RENEWING:
 	case DHCP_STATE_REBINDING:
+	receive_rapid_commit:
 		if (msg_type == DHCP_MESSAGE_TYPE_NAK) {
 			CLIENT_DEBUG("Received NAK, Stopping...");
 			l_dhcp_client_stop(client);
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 12/15] dhcp-server: Support RFC4039 Rapid Commit
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
                   ` (9 preceding siblings ...)
  2021-08-02 14:04 ` [PATCH 11/15] dhcp: Support RFC4039 Rapid Commit Andrew Zaborowski
@ 2021-08-02 14:04 ` Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 13/15] dhcp-server: Rapid commit enable/disable setter Andrew Zaborowski
                   ` (3 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

Handle the RFC4039 Rapid Commit option in a DHCPDISCOVERY by directly
creating an active lease and replying with a DHCPACK, i.e. skipping the
DHCPOFFER and DHCPREQUEST roundtrip.
---
 ell/dhcp-server.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 506bb7d..971bc54 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -658,7 +658,8 @@ static void send_nak(struct l_dhcp_server *server,
 
 static void send_ack(struct l_dhcp_server *server,
 			const struct dhcp_message *client_msg,
-			struct l_dhcp_lease *lease)
+			struct l_dhcp_lease *lease,
+			bool rapid_commit)
 {
 	struct dhcp_message_builder builder;
 	size_t len = sizeof(struct dhcp_message) + DHCP_MIN_OPTIONS_SIZE;
@@ -684,6 +685,10 @@ static void send_ack(struct l_dhcp_server *server,
 	_dhcp_message_builder_append(&builder, L_DHCP_OPTION_SERVER_IDENTIFIER,
 					4, &server->address);
 
+	if (rapid_commit)
+		_dhcp_message_builder_append(&builder, DHCP_OPTION_RAPID_COMMIT,
+						0, "");
+
 	_dhcp_message_builder_finalize(&builder, &len);
 
 	SERVER_DEBUG("Sending ACK to "NIPQUAD_FMT, NIPQUAD(reply->yiaddr));
@@ -712,6 +717,7 @@ static void listener_event(const void *data, size_t len, void *user_data)
 	bool server_id_match = true;
 	uint32_t requested_ip_opt = 0;
 	L_AUTO_FREE_VAR(uint8_t *, client_id_opt) = NULL;
+	bool rapid_commit_opt = false;
 
 	SERVER_DEBUG("");
 
@@ -745,6 +751,9 @@ static void listener_event(const void *data, size_t len, void *user_data)
 			client_id_opt[0] = l;
 			memcpy(client_id_opt + 1, v, l);
 			break;
+		case DHCP_OPTION_RAPID_COMMIT:
+			rapid_commit_opt = true;
+			break;
 		}
 	}
 
@@ -772,6 +781,19 @@ static void listener_event(const void *data, size_t len, void *user_data)
 		if (!server_id_match)
 			break;
 
+		if (rapid_commit_opt) {
+			lease = l_dhcp_server_discover(server, requested_ip_opt,
+							client_id_opt,
+							message->chaddr);
+			if (!lease) {
+				send_nak(server, message, client_id_opt);
+				break;
+			}
+
+			send_ack(server, message, lease, rapid_commit_opt);
+			break;
+		}
+
 		send_offer(server, message, lease, requested_ip_opt,
 				client_id_opt);
 		break;
@@ -868,7 +890,7 @@ static void listener_event(const void *data, size_t len, void *user_data)
 			}
 		}
 
-		send_ack(server, message, lease);
+		send_ack(server, message, lease, false);
 		break;
 	case DHCP_MESSAGE_TYPE_DECLINE:
 		SERVER_DEBUG("Received DECLINE");
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 13/15] dhcp-server: Rapid commit enable/disable setter
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
                   ` (10 preceding siblings ...)
  2021-08-02 14:04 ` [PATCH 12/15] dhcp-server: " Andrew Zaborowski
@ 2021-08-02 14:04 ` Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 14/15] unit: Stricter checks in unit-dhcp Andrew Zaborowski
                   ` (2 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

---
 ell/dhcp-server.c | 14 +++++++++++++-
 ell/dhcp.h        |  2 ++
 ell/ell.sym       |  1 +
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 971bc54..f92807a 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -89,6 +89,7 @@ struct l_dhcp_server {
 	struct l_acd *acd;
 
 	bool authoritative : 1;
+	bool rapid_commit : 1;
 };
 
 #define MAC "%02x:%02x:%02x:%02x:%02x:%02x"
@@ -781,7 +782,7 @@ static void listener_event(const void *data, size_t len, void *user_data)
 		if (!server_id_match)
 			break;
 
-		if (rapid_commit_opt) {
+		if (rapid_commit_opt && server->rapid_commit) {
 			lease = l_dhcp_server_discover(server, requested_ip_opt,
 							client_id_opt,
 							message->chaddr);
@@ -966,6 +967,7 @@ LIB_EXPORT struct l_dhcp_server *l_dhcp_server_new(int ifindex)
 
 	server->started = false;
 	server->authoritative = true;
+	server->rapid_commit = true;
 
 	server->lease_seconds = DEFAULT_DHCP_LEASE_SEC;
 	server->max_expired = MAX_EXPIRED_LEASES;
@@ -1308,6 +1310,16 @@ LIB_EXPORT void l_dhcp_server_set_authoritative(struct l_dhcp_server *server,
 	server->authoritative = authoritative;
 }
 
+LIB_EXPORT void l_dhcp_server_set_enable_rapid_commit(
+						struct l_dhcp_server *server,
+						bool enable)
+{
+	if (unlikely(!server))
+		return;
+
+	server->rapid_commit = enable;
+}
+
 LIB_EXPORT struct l_dhcp_lease *l_dhcp_server_discover(
 						struct l_dhcp_server *server,
 						uint32_t requested_ip_opt,
diff --git a/ell/dhcp.h b/ell/dhcp.h
index 58deb7e..5873c90 100644
--- a/ell/dhcp.h
+++ b/ell/dhcp.h
@@ -143,6 +143,8 @@ bool l_dhcp_server_set_gateway(struct l_dhcp_server *server, const char *ip);
 bool l_dhcp_server_set_dns(struct l_dhcp_server *server, char **dns);
 void l_dhcp_server_set_authoritative(struct l_dhcp_server *server,
 					bool authoritative);
+void l_dhcp_server_set_enable_rapid_commit(struct l_dhcp_server *server,
+						bool enable);
 
 struct l_dhcp_lease *l_dhcp_server_discover(struct l_dhcp_server *server,
 						uint32_t requested_ip_opt,
diff --git a/ell/ell.sym b/ell/ell.sym
index a781dea..b29d98d 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -260,6 +260,7 @@ global:
 	l_dhcp_server_set_gateway;
 	l_dhcp_server_set_dns;
 	l_dhcp_server_set_authoritative;
+	l_dhcp_server_set_enable_rapid_commit;
 	l_dhcp_server_discover;
 	l_dhcp_server_request;
 	l_dhcp_server_decline;
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 14/15] unit: Stricter checks in unit-dhcp
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
                   ` (11 preceding siblings ...)
  2021-08-02 14:04 ` [PATCH 13/15] dhcp-server: Rapid commit enable/disable setter Andrew Zaborowski
@ 2021-08-02 14:04 ` Andrew Zaborowski
  2021-08-02 14:04 ` [PATCH 15/15] unit: Test DHCP rapid commit Andrew Zaborowski
  2021-08-02 18:15 ` [PATCH 01/15] dhcp-server: Add "authoritative" mode Denis Kenzior
  14 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

When the client state machine is expected to have generated a packet and
the transport l2_send callback is expected to have filled in
client_packet/client_packet_len, make sure this has actually happened.
Without this the test passes with rapid-commit enabled even though the
code looks like it's requiring 4 messages -- the DHCPDISCOVER is being
re-sent and the server generates a second DHCPACK.
---
 unit/test-dhcp.c | 40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/unit/test-dhcp.c b/unit/test-dhcp.c
index e03eaca..079f037 100644
--- a/unit/test-dhcp.c
+++ b/unit/test-dhcp.c
@@ -655,13 +655,17 @@ static bool dhcp_message_compare(const uint8_t *expected, size_t expected_len,
 	return r;
 }
 
+static bool client_send_called = false;
+
 static int fake_transport_send(struct dhcp_transport *transport,
 				const struct sockaddr_in *dest,
 				const void *data, size_t len)
 {
 	assert(len <= sizeof(client_packet));
+	assert(!client_send_called);
 	memcpy(client_packet, data, len);
 	client_packet_len = len;
+	client_send_called = true;
 
 	return len;
 }
@@ -673,8 +677,10 @@ static int fake_transport_l2_send(struct dhcp_transport *transprot,
 					const void *data, size_t len)
 {
 	assert(len <= sizeof(client_packet));
+	assert(!client_send_called);
 	memcpy(client_packet, data, len);
 	client_packet_len = len;
+	client_send_called = true;
 
 	return len;
 }
@@ -710,16 +716,21 @@ static void test_discover(const void *data)
 
 	assert(l_dhcp_client_start(client));
 
+	assert(client_send_called);
+	client_send_called = false;
 	assert(dhcp_message_compare(discover_data_1, sizeof(discover_data_1),
 					client_packet, client_packet_len));
 
 	transport->rx_cb(offer_data_1, sizeof(offer_data_1), client);
 
+	assert(client_send_called);
+	client_send_called = false;
 	assert(dhcp_message_compare(request_data_1, sizeof(request_data_1),
 					client_packet, client_packet_len));
 
 	event_handler_called = false;
 	transport->rx_cb(ack_data_1, sizeof(ack_data_1), client);
+	assert(!client_send_called);
 	assert(event_handler_called);
 
 	lease = l_dhcp_client_get_lease(client);
@@ -736,6 +747,8 @@ static void test_discover(const void *data)
 	assert(lease->t2 == 0x00012750);
 
 	l_dhcp_client_destroy(client);
+	assert(client_send_called);
+	client_send_called = false;
 }
 
 static bool l2_send_called = false;
@@ -749,6 +762,7 @@ static int fake_transport_server_l2_send(struct dhcp_transport *s,
 					const void *data, size_t len)
 {
 	assert(len <= sizeof(server_packet));
+	assert(!l2_send_called);
 	memcpy(server_packet, data, len);
 	server_packet_len = len;
 
@@ -774,9 +788,11 @@ static void server_event(struct l_dhcp_server *server,
 {
 	switch (event) {
 	case L_DHCP_SERVER_EVENT_NEW_LEASE:
+		assert(!new_client);
 		new_client = l_dhcp_lease_get_address(lease);
 		break;
 	case L_DHCP_SERVER_EVENT_LEASE_EXPIRED:
+		assert(!expired_client);
 		expired_client = l_dhcp_lease_get_address(lease);
 		break;
 	}
@@ -806,15 +822,17 @@ static struct l_dhcp_client *client_init(const uint8_t *mac)
 	return client;
 }
 
-static bool client_connect(struct l_dhcp_client *client,
+static void client_connect(struct l_dhcp_client *client,
 				struct l_dhcp_server *server)
 {
 	struct dhcp_transport *srv_transport =
 					_dhcp_server_get_transport(server);
 	struct dhcp_transport *cli_transport =
 					_dhcp_client_get_transport(client);
-	if (!l_dhcp_client_start(client))
-		return false;
+
+	assert(l_dhcp_client_start(client));
+	assert(client_send_called);
+	client_send_called = false;
 
 	/* RX DISCOVER */
 	srv_transport->rx_cb(client_packet, client_packet_len, server);
@@ -823,6 +841,8 @@ static bool client_connect(struct l_dhcp_client *client,
 
 	/* RX OFFER */
 	cli_transport->rx_cb(server_packet, server_packet_len, client);
+	assert(client_send_called);
+	client_send_called = false;
 
 	/* RX REQUEST */
 	srv_transport->rx_cb(client_packet, client_packet_len, server);
@@ -831,10 +851,9 @@ static bool client_connect(struct l_dhcp_client *client,
 
 	/* RX ACK */
 	cli_transport->rx_cb(server_packet, server_packet_len, client);
+	assert(!client_send_called);
 
 	assert(event_handler_called);
-
-	return true;
 }
 
 static struct l_dhcp_server *server_init()
@@ -941,7 +960,8 @@ static void test_complete_run(const void *data)
 	l_free(cli_addr);
 
 	l_dhcp_client_stop(client1);
-
+	assert(client_send_called);
+	client_send_called = false;
 	srv_transport->rx_cb(client_packet, client_packet_len, server);
 	assert(expired_client);
 	assert(!strcmp(expired_client, "192.168.1.2"));
@@ -949,6 +969,8 @@ static void test_complete_run(const void *data)
 	expired_client = NULL;
 
 	l_dhcp_client_stop(client2);
+	assert(client_send_called);
+	client_send_called = false;
 	srv_transport->rx_cb(client_packet, client_packet_len, server);
 	assert(expired_client);
 	assert(!strcmp(expired_client, "192.168.1.3"));
@@ -956,7 +978,9 @@ static void test_complete_run(const void *data)
 	expired_client = NULL;
 
 	l_dhcp_client_destroy(client1);
+	assert(!client_send_called);
 	l_dhcp_client_destroy(client2);
+	assert(!client_send_called);
 
 	l_dhcp_server_stop(server);
 	l_dhcp_server_destroy(server);
@@ -992,6 +1016,8 @@ static void test_expired_ip_reuse(const void *data)
 		new_client = NULL;
 
 		l_dhcp_client_destroy(client);
+		assert(client_send_called);
+		client_send_called = false;
 		srv_transport->rx_cb(client_packet, client_packet_len, server);
 		l_free(expired_client);
 		expired_client = NULL;
@@ -1010,6 +1036,8 @@ static void test_expired_ip_reuse(const void *data)
 	l_free(cli_addr);
 
 	l_dhcp_client_destroy(client_new);
+	assert(client_send_called);
+	client_send_called = false;
 	l_dhcp_server_destroy(server);
 }
 
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 15/15] unit: Test DHCP rapid commit
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
                   ` (12 preceding siblings ...)
  2021-08-02 14:04 ` [PATCH 14/15] unit: Stricter checks in unit-dhcp Andrew Zaborowski
@ 2021-08-02 14:04 ` Andrew Zaborowski
  2021-08-02 18:15 ` [PATCH 01/15] dhcp-server: Add "authoritative" mode Denis Kenzior
  14 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2021-08-02 14:04 UTC (permalink / raw)
  To: ell

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

---
 unit/test-dhcp.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/unit/test-dhcp.c b/unit/test-dhcp.c
index 079f037..11ee10e 100644
--- a/unit/test-dhcp.c
+++ b/unit/test-dhcp.c
@@ -823,7 +823,7 @@ static struct l_dhcp_client *client_init(const uint8_t *mac)
 }
 
 static void client_connect(struct l_dhcp_client *client,
-				struct l_dhcp_server *server)
+				struct l_dhcp_server *server, bool rapid_commit)
 {
 	struct dhcp_transport *srv_transport =
 					_dhcp_server_get_transport(server);
@@ -839,15 +839,17 @@ static void client_connect(struct l_dhcp_client *client,
 	assert(l2_send_called);
 	l2_send_called = false;
 
-	/* RX OFFER */
-	cli_transport->rx_cb(server_packet, server_packet_len, client);
-	assert(client_send_called);
-	client_send_called = false;
+	if (!rapid_commit) {
+		/* RX OFFER */
+		cli_transport->rx_cb(server_packet, server_packet_len, client);
+		assert(client_send_called);
+		client_send_called = false;
 
-	/* RX REQUEST */
-	srv_transport->rx_cb(client_packet, client_packet_len, server);
-	assert(l2_send_called);
-	l2_send_called = false;
+		/* RX REQUEST */
+		srv_transport->rx_cb(client_packet, client_packet_len, server);
+		assert(l2_send_called);
+		l2_send_called = false;
+	}
 
 	/* RX ACK */
 	cli_transport->rx_cb(server_packet, server_packet_len, client);
@@ -885,6 +887,7 @@ static struct l_dhcp_server *server_init()
 
 static void test_complete_run(const void *data)
 {
+	bool rapid_commit = L_PTR_TO_UINT(data);
 	static const uint8_t addr1[6] = { 0x01, 0x02, 0x03, 0x04, 0x05, 0x06 };
 	static const uint8_t addr2[6] = { 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c };
 	struct dhcp_transport *srv_transport;
@@ -902,6 +905,7 @@ static void test_complete_run(const void *data)
 	char **dns_list;
 
 	server = server_init();
+	l_dhcp_server_set_enable_rapid_commit(server, rapid_commit);
 
 	assert(l_dhcp_server_set_ip_range(server, "192.168.1.2",
 						"192.168.1.100"));
@@ -910,7 +914,7 @@ static void test_complete_run(const void *data)
 
 	client1 = client_init(addr1);
 
-	client_connect(client1, server);
+	client_connect(client1, server, rapid_commit);
 
 	cli_lease = l_dhcp_client_get_lease(client1);
 	assert(cli_lease);
@@ -943,7 +947,7 @@ static void test_complete_run(const void *data)
 
 	client2 = client_init(addr2);
 
-	client_connect(client2, server);
+	client_connect(client2, server, rapid_commit);
 
 	cli_lease = l_dhcp_client_get_lease(client2);
 	assert(cli_lease);
@@ -999,6 +1003,7 @@ static void test_expired_ip_reuse(const void *data)
 
 	l_dhcp_server_set_ip_range(server, "192.168.1.2", "192.168.1.11");
 	_dhcp_server_set_max_expired_clients(server, 10);
+	l_dhcp_server_set_enable_rapid_commit(server, false);
 
 	/*
 	 * Connect and release 10 clients, this should max out the expired
@@ -1011,7 +1016,7 @@ static void test_expired_ip_reuse(const void *data)
 
 		addr[5] = i;
 		client = client_init(addr);
-		client_connect(client, server);
+		client_connect(client, server, false);
 		l_free(new_client);
 		new_client = NULL;
 
@@ -1025,7 +1030,7 @@ static void test_expired_ip_reuse(const void *data)
 
 	addr[5] = i + 1;
 	client_new = client_init(addr);
-	client_connect(client_new, server);
+	client_connect(client_new, server, false);
 	l_free(new_client);
 	new_client = NULL;
 
@@ -1064,7 +1069,8 @@ int main(int argc, char *argv[])
 
 	l_test_add("discover", test_discover, NULL);
 
-	l_test_add("complete run", test_complete_run, NULL);
+	l_test_add("complete run", test_complete_run, L_UINT_TO_PTR(false));
+	l_test_add("rapid commit", test_complete_run, L_UINT_TO_PTR(true));
 	l_test_add("expired IP reuse", test_expired_ip_reuse, NULL);
 
 	return l_test_run();
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 01/15] dhcp-server: Add "authoritative" mode
  2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
                   ` (13 preceding siblings ...)
  2021-08-02 14:04 ` [PATCH 15/15] unit: Test DHCP rapid commit Andrew Zaborowski
@ 2021-08-02 18:15 ` Denis Kenzior
  14 siblings, 0 replies; 16+ messages in thread
From: Denis Kenzior @ 2021-08-02 18:15 UTC (permalink / raw)
  To: ell

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

Hi Andrew,

On 8/2/21 9:04 AM, Andrew Zaborowski wrote:
> An authoritative DHCP server basically assumes that there are no other
> DHCP servers on the network and replies to DHCPREQUEST messages
> requesting IPs from outside of its subnet or with a wrong server ID
> value.  This speeds up connections from clients who try to use IP
> leases they have cached from other networks and who start their IP
> configuration by trying DHCPREQUESTs for those leases.
> 
> The term is not defined in the DHCP standard but is used by various
> implementations.  Default to authoritative being true.  When set to
> false, handle a DHCPREQUEST message directed at other servers by taking
> it as meaning the client is declining our own lease offer as mandated
> by RFC 2131 on page 16.
> 
> Refactor the checks in DHCPREQUEST handling quoting parts of the RFC.
> The new checks are stricter and I'm not preserving some of the checks
> that didn't seem justified, for example in the
> "if (server_id_opt || !lease) ... send_nak()" block it didn't seem to
> make sense to only reply to clients who have no lease (offered or
> active) and ignore those who do have a lease with a different IP
> address, the opposite would make more sense.
> ---
>   ell/dhcp-server.c | 117 +++++++++++++++++++++++++++++++++++++++-------
>   ell/dhcp.h        |   2 +
>   ell/ell.sym       |   1 +
>   3 files changed, 103 insertions(+), 17 deletions(-)
> 

All applied, thanks.

Regards,
-Denis

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-08-02 18:15 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-02 14:04 [PATCH 01/15] dhcp-server: Add "authoritative" mode Andrew Zaborowski
2021-08-02 14:04 ` [PATCH 02/15] dhcp-server: Handle DHCPDECLINE for active leases Andrew Zaborowski
2021-08-02 14:04 ` [PATCH 03/15] dhcp-server: Respect client's broadcast flag Andrew Zaborowski
2021-08-02 14:04 ` [PATCH 04/15] dhcp-server: Look up leases by client identifier option Andrew Zaborowski
2021-08-02 14:04 ` [PATCH 05/15] dhcp-server: Copy client identifier from the client message Andrew Zaborowski
2021-08-02 14:04 ` [PATCH 06/15] dhcp-server: Save lease mac before calling add_lease Andrew Zaborowski
2021-08-02 14:04 ` [PATCH 07/15] dhcp-server: Ensure broadcast address is not selected Andrew Zaborowski
2021-08-02 14:04 ` [PATCH 08/15] dhcp-server: Reuse leases in find_free_or_expired_ip Andrew Zaborowski
2021-08-02 14:04 ` [PATCH 09/15] dhcp-server: Refactor lease lookup Andrew Zaborowski
2021-08-02 14:04 ` [PATCH 10/15] dhcp-server: Allow reactivating expired leases Andrew Zaborowski
2021-08-02 14:04 ` [PATCH 11/15] dhcp: Support RFC4039 Rapid Commit Andrew Zaborowski
2021-08-02 14:04 ` [PATCH 12/15] dhcp-server: " Andrew Zaborowski
2021-08-02 14:04 ` [PATCH 13/15] dhcp-server: Rapid commit enable/disable setter Andrew Zaborowski
2021-08-02 14:04 ` [PATCH 14/15] unit: Stricter checks in unit-dhcp Andrew Zaborowski
2021-08-02 14:04 ` [PATCH 15/15] unit: Test DHCP rapid commit Andrew Zaborowski
2021-08-02 18:15 ` [PATCH 01/15] dhcp-server: Add "authoritative" mode Denis Kenzior

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).