[systemd-devel] [PATCH 14/17] sd-dhcp-server: track bound leases
Zbigniew Jędrzejewski-Szmek
zbyszek at in.waw.pl
Tue May 27 09:20:35 PDT 2014
On Mon, May 26, 2014 at 09:39:43PM +0200, Tom Gundersen wrote:
> Make sure we don't hand out the same IP twice. We still don't
> handle lease expiry.
> ---
> src/libsystemd-network/dhcp-server-internal.h | 25 ++++-
> src/libsystemd-network/sd-dhcp-server.c | 136 ++++++++++++++++++++++++--
> src/libsystemd-network/test-dhcp-server.c | 59 +++++++++++
> 3 files changed, 206 insertions(+), 14 deletions(-)
>
> diff --git a/src/libsystemd-network/dhcp-server-internal.h b/src/libsystemd-network/dhcp-server-internal.h
> index ce2e260..7fe7253 100644
> --- a/src/libsystemd-network/dhcp-server-internal.h
> +++ b/src/libsystemd-network/dhcp-server-internal.h
> @@ -23,12 +23,25 @@
> #include "sd-event.h"
> #include "sd-dhcp-server.h"
>
> +#include "hashmap.h"
> #include "refcnt.h"
> #include "util.h"
> #include "log.h"
>
> #include "dhcp-internal.h"
>
> +typedef struct DHCPClientId {
> + size_t length;
> + uint8_t *data;
> +} DHCPClientId;
> +
> +typedef struct DHCPLease {
> + DHCPClientId client_id;
> +
> + be32_t address;
> + usec_t expiration;
> +} DHCPLease;
> +
> struct sd_dhcp_server {
> RefCount n_ref;
>
> @@ -42,12 +55,11 @@ struct sd_dhcp_server {
> be32_t address;
> be32_t pool_start;
> size_t pool_size;
> -};
> + size_t next_offer;
>
> -typedef struct DHCPClientId {
> - size_t length;
> - uint8_t *data;
> -} DHCPClientId;
> + Hashmap *leases_by_client_id;
> + DHCPLease **bound_leases;
> +};
>
> typedef struct DHCPRequest {
> /* received message */
> @@ -71,3 +83,6 @@ int dhcp_server_handle_message(sd_dhcp_server *server, DHCPMessage *message,
> int dhcp_server_send_packet(sd_dhcp_server *server,
> DHCPRequest *req, DHCPPacket *packet,
> int type, size_t optoffset);
> +
> +unsigned long client_id_hash_func(const void *p, const uint8_t hash_key[HASH_KEY_SIZE]);
> +int client_id_compare_func(const void *_a, const void *_b);
> diff --git a/src/libsystemd-network/sd-dhcp-server.c b/src/libsystemd-network/sd-dhcp-server.c
> index 3de61f7..44ca645 100644
> --- a/src/libsystemd-network/sd-dhcp-server.c
> +++ b/src/libsystemd-network/sd-dhcp-server.c
> @@ -23,6 +23,8 @@
> #include <sys/ioctl.h>
> #include <netinet/if_ether.h>
>
> +#include "siphash24.h"
> +
> #include "sd-dhcp-server.h"
> #include "dhcp-server-internal.h"
> #include "dhcp-internal.h"
> @@ -37,6 +39,11 @@ int sd_dhcp_server_set_lease_pool(sd_dhcp_server *server, struct in_addr *addres
> assert_return(size, -EINVAL);
> assert_return(server->pool_start == htobe32(INADDR_ANY), -EBUSY);
> assert_return(!server->pool_size, -EBUSY);
> + assert_return(!server->bound_leases, -EBUSY);
> +
> + server->bound_leases = new0(DHCPLease*, size);
> + if (!server->bound_leases)
> + return -ENOMEM;
>
> server->pool_start = address->s_addr;
> server->pool_size = size;
> @@ -62,13 +69,63 @@ sd_dhcp_server *sd_dhcp_server_ref(sd_dhcp_server *server) {
> return server;
> }
>
> +unsigned long client_id_hash_func(const void *p, const uint8_t hash_key[HASH_KEY_SIZE]) {
> + uint64_t u;
> + const DHCPClientId *id = p;
> +
> + assert(id);
> + assert(id->length);
> + assert(id->data);
> +
> + siphash24((uint8_t*) &u, id->data, id->length, hash_key);
> +
> + return (unsigned long) u;
> +}
> +
> +int client_id_compare_func(const void *_a, const void *_b) {
> + const DHCPClientId *a, *b;
> +
> + a = _a;
> + b = _b;
> +
> + assert(!a->length || a->data);
> + assert(!b->length || b->data);
> +
> + if (a->length != b->length)
> + return a->length < b->length ? -1 : 1;
> +
> + return memcmp(a->data, b->data, a->length);
> +}
> +
> +static void dhcp_lease_free(DHCPLease *lease) {
> + if (!lease)
> + return;
> +
> + free(lease->client_id.data);
> + free(lease);
> +}
> +
> +DEFINE_TRIVIAL_CLEANUP_FUNC(DHCPLease*, dhcp_lease_free);
> +#define _cleanup_dhcp_lease_free_ _cleanup_(dhcp_lease_freep)
> +
> sd_dhcp_server *sd_dhcp_server_unref(sd_dhcp_server *server) {
> if (server && REFCNT_DEC(server->n_ref) <= 0) {
> + DHCPLease *lease;
> + Iterator i;
> +
> log_dhcp_server(server, "UNREF");
>
> sd_dhcp_server_stop(server);
>
> sd_event_unref(server->event);
> +
> + HASHMAP_FOREACH(lease, server->leases_by_client_id, i) {
> + hashmap_remove(server->leases_by_client_id, lease);
> + dhcp_lease_free(lease);
> + }
> +
> + hashmap_free(server->leases_by_client_id);
> + free(server->bound_leases);
> free(server);
> }
>
> @@ -90,6 +147,7 @@ int sd_dhcp_server_new(sd_dhcp_server **ret, int ifindex) {
> server->fd = -1;
> server->address = htobe32(INADDR_ANY);
> server->index = ifindex;
> + server->leases_by_client_id = hashmap_new(client_id_hash_func, client_id_compare_func);
>
> *ret = server;
> server = NULL;
> @@ -478,9 +536,25 @@ static int ensure_sane_request(DHCPRequest *req, DHCPMessage *message) {
> return 0;
> }
>
> +static int get_pool_offset(sd_dhcp_server *server, be32_t requested_ip) {
> + assert(server);
> +
> + if (!server->pool_size)
> + return -EINVAL;
> +
> + if (be32toh(requested_ip) < be32toh(server->pool_start) ||
> + be32toh(requested_ip) >= be32toh(server->pool_start) +
> + + server->pool_size)
> + return -EINVAL;
> +
> + return (be32toh(requested_ip) -
> + - be32toh(server->pool_start)) % server->pool_size;
This looks wrong, double minus comes out to a plus, and the modulo should
not be necessary because of the check above.
> +}
> +
> int dhcp_server_handle_message(sd_dhcp_server *server, DHCPMessage *message,
> size_t length) {
> _cleanup_dhcp_request_free_ DHCPRequest *req = NULL;
> + DHCPLease *existing_lease;
> int type, r;
>
> assert(server);
> @@ -504,10 +578,13 @@ int dhcp_server_handle_message(sd_dhcp_server *server, DHCPMessage *message,
> /* this only fails on critical errors */
> return r;
>
> + existing_lease = hashmap_get(server->leases_by_client_id, &req->client_id);
> +
> switch(type) {
> case DHCP_DISCOVER:
> {
> - be32_t address;
> + be32_t address = INADDR_ANY;
> + unsigned i;
>
> log_dhcp_server(server, "DISCOVER (0x%x)",
> be32toh(req->message->xid));
> @@ -516,9 +593,22 @@ int dhcp_server_handle_message(sd_dhcp_server *server, DHCPMessage *message,
> /* no pool allocated */
> return 0;
>
> - /* for now pick a random address from the pool */
> - address = htobe32(be32toh(server->pool_start) +
> - (random_u32() % server->pool_size));
Parentheses are not be necessary, modulo has the same precedence as multiplication.
> + /* for now pick a random free address from the pool */
> + if (existing_lease)
> + address = existing_lease->address;
> + else {
> + for (i = 0; i < server->pool_size; i++, server->next_offer++) {
> + if (!server->bound_leases[(server->next_offer) % server->pool_size]) {
Misleading parentheses here.
> + address = htobe32(be32toh(server->pool_start) +
> + + (server->next_offer) % server->pool_size);
And here.
> + break;
Hm, maybe it would be nicer to apply the modulo already when incrementing the variable? Might
be more readable that way:
for (i = 0; i < server->pool_size; i++)
if (!server->bound_leases[server->next_offer]) {
address = htobe32(be32toh(server->pool_start) + server->next_offer);
break;
} else
server->next_offer = (server->next_offer + 1) % server->pool_size;
> + }
> + }
> + }
> +
> + if (address == INADDR_ANY)
> + /* no free addresses left */
> + return 0;
>
> r = server_send_offer(server, req, address);
> if (r < 0) {
> @@ -537,6 +627,7 @@ int dhcp_server_handle_message(sd_dhcp_server *server, DHCPMessage *message,
> {
> be32_t address;
> bool init_reboot = false;
> + int pool_offset;
>
> /* see RFC 2131, section 4.3.2 */
>
> @@ -583,19 +674,46 @@ int dhcp_server_handle_message(sd_dhcp_server *server, DHCPMessage *message,
> address = req->message->ciaddr;
> }
>
> - /* for now we just verify that the address is from the pool, not
> - whether or not it is taken */
> - if (htobe32(req->requested_ip) >= htobe32(server->pool_start) &&
> - htobe32(req->requested_ip) < htobe32(server->pool_start) +
> - + server->pool_size) {
> + pool_offset = get_pool_offset(server, address);
> +
> + /* verify that the requested address is from the pool, and either
> + owned by the current client or free */
> + if (pool_offset >= 0 &&
> + server->bound_leases[pool_offset] == existing_lease) {
> + DHCPLease *lease;
> + usec_t time_now;
> +
> + if (!existing_lease) {
> + lease = new0(DHCPLease, 1);
> + lease->address = req->requested_ip;
> + lease->client_id.data = memdup(req->client_id.data,
> + req->client_id.length);
Indentation.
> + if (!lease->client_id.data)
> + return -ENOMEM;
> + lease->client_id.length = req->client_id.length;
> + } else
> + lease = existing_lease;
> +
> + r = sd_event_now(server->event, CLOCK_MONOTONIC, &time_now);
> + if (r < 0)
> + time_now = now(CLOCK_MONOTONIC);
> + lease->expiration = req->lifetime * USEC_PER_SEC + time_now;
> +
> r = server_send_ack(server, req, address);
> if (r < 0) {
> log_dhcp_server(server, "could not send ack: %s",
> strerror(-r));
> + if (!existing_lease)
> + dhcp_lease_free(lease);
> +
> return 0;
> } else {
> log_dhcp_server(server, "ACK (0x%x)",
> be32toh(req->message->xid));
> +
> + server->bound_leases[pool_offset] = lease;
> + hashmap_put(server->leases_by_client_id, &lease->client_id, lease);
> +
> return DHCP_ACK;
> }
> } else if (init_reboot) {
> diff --git a/src/libsystemd-network/test-dhcp-server.c b/src/libsystemd-network/test-dhcp-server.c
> index ea256b9..369e59f 100644
> --- a/src/libsystemd-network/test-dhcp-server.c
> +++ b/src/libsystemd-network/test-dhcp-server.c
> @@ -90,6 +90,11 @@ static void test_message_handler(void) {
> uint8_t length;
> be32_t address;
> } _packed_ option_server_id;
> + struct {
> + uint8_t code;
> + uint8_t length;
> + uint8_t id[7];
> + } _packed_ option_client_id;
> uint8_t end;
> } _packed_ test = {
> .message.op = BOOTREQUEST,
> @@ -156,14 +161,67 @@ static void test_message_handler(void) {
> test.option_server_id.address = htobe32(INADDR_LOOPBACK);
> test.option_requested_ip.address = htobe32(INADDR_LOOPBACK + 3);
> assert_se(dhcp_server_handle_message(server, (DHCPMessage*)&test, sizeof(test)) == DHCP_ACK);
> +
> test.option_server_id.address = htobe32(0x12345678);
> test.option_requested_ip.address = htobe32(INADDR_LOOPBACK + 3);
> assert_se(dhcp_server_handle_message(server, (DHCPMessage*)&test, sizeof(test)) == 0);
> test.option_server_id.address = htobe32(INADDR_LOOPBACK);
> + test.option_requested_ip.address = htobe32(INADDR_LOOPBACK + 4);
> + assert_se(dhcp_server_handle_message(server, (DHCPMessage*)&test, sizeof(test)) == 0);
> + test.option_requested_ip.address = htobe32(INADDR_LOOPBACK + 3);
> + assert_se(dhcp_server_handle_message(server, (DHCPMessage*)&test, sizeof(test)) == DHCP_ACK);
> +
> + test.option_client_id.code = DHCP_OPTION_CLIENT_IDENTIFIER;
> + test.option_client_id.length = 7;
> + test.option_client_id.id[0] = 0x01;
> + test.option_client_id.id[1] = 'A';
> + test.option_client_id.id[2] = 'B';
> + test.option_client_id.id[3] = 'C';
> + test.option_client_id.id[4] = 'D';
> + test.option_client_id.id[5] = 'E';
> + test.option_client_id.id[6] = 'F';
> + assert_se(dhcp_server_handle_message(server, (DHCPMessage*)&test, sizeof(test)) == DHCP_ACK);
> +
> test.option_requested_ip.address = htobe32(INADDR_LOOPBACK + 30);
> assert_se(dhcp_server_handle_message(server, (DHCPMessage*)&test, sizeof(test)) == 0);
> }
>
> +static void test_client_id_hash(void) {
> + DHCPClientId a = {
> + .length = 4,
> + }, b = {
> + .length = 4,
> + };
> + uint8_t hash_key[HASH_KEY_SIZE] = {
> + '0', '1', '2', '3', '4', '5', '6', '7',
> + '8', '9', 'A', 'B', 'C', 'D', 'E', 'F',
> + };
> +
> + a.data = (uint8_t*)strdup("abcd");
> + b.data = (uint8_t*)strdup("abcd");
> +
> + assert_se(client_id_compare_func(&a, &b) == 0);
> + assert_se(client_id_hash_func(&a, hash_key) == client_id_hash_func(&b, hash_key));
> + a.length = 3;
> + assert_se(client_id_compare_func(&a, &b) != 0);
> + a.length = 4;
> + assert_se(client_id_compare_func(&a, &b) == 0);
> + assert_se(client_id_hash_func(&a, hash_key) == client_id_hash_func(&b, hash_key));
> +
> + b.length = 3;
> + assert_se(client_id_compare_func(&a, &b) != 0);
> + b.length = 4;
> + assert_se(client_id_compare_func(&a, &b) == 0);
> + assert_se(client_id_hash_func(&a, hash_key) == client_id_hash_func(&b, hash_key));
> +
> + free(b.data);
> + b.data = (uint8_t*)strdup("abce");
> + assert_se(client_id_compare_func(&a, &b) != 0);
> +
> + free(a.data);
> + free(b.data);
> +}
> +
> int main(int argc, char *argv[]) {
> _cleanup_event_unref_ sd_event *e;
>
> @@ -175,6 +233,7 @@ int main(int argc, char *argv[]) {
>
> test_basic(e);
> test_message_handler();
> + test_client_id_hash();
>
> return 0;
> }
Zbyszek
More information about the systemd-devel
mailing list