All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/5] ap: Refactor IP address selection
@ 2021-03-02 12:08 Andrew Zaborowski
  2021-03-02 12:08 ` [PATCH 2/5] ap: Send a specific error message on async AP start failure Andrew Zaborowski
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Andrew Zaborowski @ 2021-03-02 12:08 UTC (permalink / raw)
  To: iwd

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

Refactor how the subnet IP address is selected and change related
settings syntax in the following way:

* [IPv4].Address in an AP profile can now be given as just an IP address
  or a list of prefix notation address spaces.  In the latter case one
  subnet address of the size implied by [IPv4].Netmask is selected
  randomly from that space, defaults to a 28-bit netmask.  In the former
  case [IPv4].Address is now also checked for conflicts with addresses
  already in use.

* main.conf's [IPv4].APAddressPool is now the fallback setting that uses
  the same syntax as profile-local [IPv4].Address and has a default
  value of 192.168.0.0/16.

* main.conf's [General].APRanges is deprecated in favour of
  [IPv4].APAddressPool but this change should be backwards compatible.

ap_setup_dhcp now always prints error messages, it seems it was assumed
that the caller would do this before but it didn't.  It also now always
reads [IPv4].Netmask or the value already set on the interface (when
existing IP is used) and uses that value.  Previously the configured
netmask was only being announced through DHCP but not used locally.
Require that the WPA-PSK passphrase is present in the AP profile,
previously it was treated as optional but the rest of the code was
expecting it to always be present.

Drop the l_net_get_address() usage, replace with the more complicated
l_rtnl_* calls.  This is to avoid also adding an l_net_get_netmask() to
ell since we'd need that and we reportely don't want to keep using the
netdev ioctls.
---
 src/ap.c | 804 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 502 insertions(+), 302 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 2042cde6..d871f973 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -28,6 +28,7 @@
 #include <linux/if_ether.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
+#include <ifaddrs.h>
 
 #include <ell/ell.h>
 
@@ -78,15 +79,16 @@ struct ap_state {
 	uint16_t last_aid;
 	struct l_queue *sta_states;
 
+	struct l_settings *profile;
 	struct l_dhcp_server *server;
+	uint32_t rtnl_dump_cmd;
 	uint32_t rtnl_add_cmd;
 	char *own_ip;
-	unsigned int ip_prefix;
+	uint8_t ip_prefix;
 
 	bool started : 1;
 	bool gtk_set : 1;
 	bool cleanup_ip : 1;
-	bool use_ip_pool : 1;
 };
 
 struct sta_state {
@@ -115,99 +117,11 @@ struct ap_wsc_pbc_probe_record {
 	uint64_t timestamp;
 };
 
-struct ap_ip_pool {
-	uint32_t start;
-	uint32_t end;
-	uint8_t prefix;
-
-	/* Fist/last valid subnet */
-	uint8_t sub_start;
-	uint8_t sub_end;
-
-	struct l_uintset *used;
-};
-
-struct ap_ip_pool pool;
+static bool netconfig_enabled;
+static char **global_addr_strs;
 static uint32_t netdev_watch;
 struct l_netlink *rtnl;
 
-/*
- * Creates pool of IPs which AP intefaces can use. Each call to ip_pool_get
- * will advance the subnet +1 so there are no IP conflicts between AP
- * interfaces
- */
-static bool ip_pool_create(const char *ip_prefix)
-{
-	if (!util_ip_prefix_tohl(ip_prefix, &pool.prefix, &pool.start,
-					&pool.end, NULL))
-		return false;
-
-	if (pool.prefix > 24) {
-		l_error("APRanges prefix must 24 or less (%u used)",
-				pool.prefix);
-		memset(&pool, 0, sizeof(pool));
-		return false;
-	}
-
-	/*
-	 * Find the number of subnets we can use, this will dictate the number
-	 * of AP interfaces that can be created (when using DHCP)
-	 */
-	pool.sub_start = (pool.start & 0x0000ff00) >> 8;
-	pool.sub_end = (pool.end & 0x0000ff00) >> 8;
-
-	pool.used = l_uintset_new_from_range(pool.sub_start, pool.sub_end);
-
-	return true;
-}
-
-static char *ip_pool_get()
-{
-	uint32_t ip;
-	struct in_addr ia;
-	uint8_t next_subnet = (uint8_t)l_uintset_find_unused_min(pool.used);
-
-	/* This shouldn't happen */
-	if (next_subnet < pool.sub_start || next_subnet > pool.sub_end)
-		return NULL;
-
-	l_uintset_put(pool.used, next_subnet);
-
-	ip = pool.start;
-	ip &= 0xffff00ff;
-	ip |= (next_subnet << 8);
-
-	ia.s_addr = htonl(ip);
-	return l_strdup(inet_ntoa(ia));
-}
-
-static bool ip_pool_put(const char *address)
-{
-	struct in_addr ia;
-	uint32_t ip;
-	uint8_t subnet;
-
-	if (inet_aton(address, &ia) < 0)
-		return false;
-
-	ip = ntohl(ia.s_addr);
-
-	subnet = (ip & 0x0000ff00) >> 8;
-
-	if (subnet < pool.sub_start || subnet > pool.sub_end)
-		return false;
-
-	return l_uintset_take(pool.used, subnet);
-}
-
-static void ip_pool_destroy()
-{
-	if (pool.used)
-		l_uintset_free(pool.used);
-
-	memset(&pool, 0, sizeof(pool));
-}
-
 static const char *broadcast_from_ip(const char *ip)
 {
 	struct in_addr ia;
@@ -304,6 +218,13 @@ static void ap_reset(struct ap_state *ap)
 	if (ap->start_stop_cmd_id)
 		l_genl_family_cancel(ap->nl80211, ap->start_stop_cmd_id);
 
+	if (ap->rtnl_dump_cmd) {
+		uint32_t cmd = ap->rtnl_dump_cmd;
+
+		ap->rtnl_dump_cmd = 0;
+		l_netlink_cancel(rtnl, cmd);
+	}
+
 	if (ap->rtnl_add_cmd)
 		l_netlink_cancel(rtnl, ap->rtnl_add_cmd);
 
@@ -326,16 +247,16 @@ static void ap_reset(struct ap_state *ap)
 					broadcast_from_ip(ap->own_ip),
 					NULL, NULL, NULL);
 
-	if (ap->own_ip) {
-		/* Release IP from pool if used */
-		if (ap->use_ip_pool)
-			ip_pool_put(ap->own_ip);
-
+	if (ap->own_ip)
 		l_free(ap->own_ip);
-	}
 
 	if (ap->server)
 		l_dhcp_server_stop(ap->server);
+
+	if (ap->profile) {
+		l_settings_free(ap->profile);
+		ap->profile = NULL;
+	}
 }
 
 static void ap_del_station(struct sta_state *sta, uint16_t reason,
@@ -2172,34 +2093,41 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 	return cmd;
 }
 
-static void ap_ifaddr4_added_cb(int error, uint16_t type, const void *data,
-				uint32_t len, void *user_data)
+static bool ap_start_send(struct ap_state *ap)
 {
-	struct ap_state *ap = user_data;
-	struct l_genl_msg *cmd;
-
-	ap->rtnl_add_cmd = 0;
+	struct l_genl_msg *cmd = ap_build_cmd_start_ap(ap);
 
-	if (error) {
-		l_error("Failed to set IP address");
-		goto error;
+	if (!cmd) {
+		l_error("ap_build_cmd_start_ap failed");
+		return false;
 	}
 
-	cmd = ap_build_cmd_start_ap(ap);
-	if (!cmd)
-		goto error;
-
 	ap->start_stop_cmd_id = l_genl_family_send(ap->nl80211, cmd,
 							ap_start_cb, ap, NULL);
 	if (!ap->start_stop_cmd_id) {
+		l_error("AP_START l_genl_family_send failed");
 		l_genl_msg_unref(cmd);
-		goto error;
+		return false;
 	}
 
-	return;
+	return true;
+}
 
-error:
-	ap_start_failed(ap);
+static void ap_ifaddr4_added_cb(int error, uint16_t type, const void *data,
+				uint32_t len, void *user_data)
+{
+	struct ap_state *ap = user_data;
+
+	ap->rtnl_add_cmd = 0;
+
+	if (error) {
+		l_error("Failed to set IP address");
+		ap_start_failed(ap);
+		return;
+	}
+
+	if (!ap_start_send(ap))
+		ap_start_failed(ap);
 }
 
 static bool ap_parse_new_station_ies(const void *data, uint16_t len,
@@ -2394,23 +2322,21 @@ static void ap_mlme_notify(struct l_genl_msg *msg, void *user_data)
 	}
 }
 
-static bool dhcp_load_settings(struct ap_state *ap, struct l_settings *settings)
+static bool ap_load_dhcp_settings(struct ap_state *ap,
+					uint32_t own_ip, uint8_t prefix)
 {
 	struct l_dhcp_server *server = ap->server;
-	struct in_addr ia;
 
-	L_AUTO_FREE_VAR(char *, netmask) = l_settings_get_string(settings,
-							"IPv4", "Netmask");
-	L_AUTO_FREE_VAR(char *, gateway) = l_settings_get_string(settings,
+	L_AUTO_FREE_VAR(char *, gateway) = l_settings_get_string(ap->profile,
 							"IPv4", "Gateway");
-	char **dns = l_settings_get_string_list(settings, "IPv4",
+	char **dns = l_settings_get_string_list(ap->profile, "IPv4",
 							"DNSList", ',');
-	char **ip_range = l_settings_get_string_list(settings, "IPv4",
+	char **ip_range = l_settings_get_string_list(ap->profile, "IPv4",
 							"IPRange", ',');
 	unsigned int lease_time;
 	bool ret = false;
 
-	if (!l_settings_get_uint(settings, "IPv4", "LeaseTime", &lease_time))
+	if (!l_settings_get_uint(ap->profile, "IPv4", "LeaseTime", &lease_time))
 		lease_time = 0;
 
 	if (gateway && !l_dhcp_server_set_gateway(server, gateway)) {
@@ -2423,17 +2349,34 @@ static bool dhcp_load_settings(struct ap_state *ap, struct l_settings *settings)
 		goto error;
 	}
 
-	if (netmask && !l_dhcp_server_set_netmask(server, netmask)) {
-		l_error("[IPv4].Netmask value error");
-		goto error;
-	}
-
 	if (ip_range) {
+		int i;
+
 		if (l_strv_length(ip_range) != 2) {
 			l_error("Two addresses expected in [IPv4].IPRange");
 			goto error;
 		}
 
+		for (i = 0; i < 2; i++) {
+			struct in_addr range_addr;
+
+			if (inet_aton(ip_range[i], &range_addr) != 1) {
+				l_error("Can't parse address in "
+					"[IPv4].IPRange[%i]", i + 1);
+				goto error;
+			}
+
+			if ((own_ip ^ ntohl(range_addr.s_addr)) &
+					util_netmask_from_prefix(prefix)) {
+				struct in_addr addr = { htonl(own_ip) };
+
+				l_error("[IPv4].IPRange[%i] is not in the "
+					"%s/%i subnet", i + 1,
+					inet_ntoa(addr), prefix);
+				goto error;
+			}
+		}
+
 		if (!l_dhcp_server_set_ip_range(server, ip_range[0],
 							ip_range[1])) {
 			l_error("Error setting IP range from [IPv4].IPRange");
@@ -2446,11 +2389,6 @@ static bool dhcp_load_settings(struct ap_state *ap, struct l_settings *settings)
 		goto error;
 	}
 
-	if (netmask && inet_pton(AF_INET, netmask, &ia) > 0)
-		ap->ip_prefix = __builtin_popcountl(ia.s_addr);
-	else
-		ap->ip_prefix = 24;
-
 	ret = true;
 
 error:
@@ -2459,166 +2397,404 @@ error:
 	return ret;
 }
 
+struct ap_ip_range {
+	uint32_t start;
+	uint32_t end;
+};
+
+static int ap_ip_range_compare(const void *a, const void *b, void *user_data)
+{
+	const struct ap_ip_range *range_a = a;
+	const struct ap_ip_range *range_b = b;
+
+	return range_a->start > range_b->start;
+}
+
 /*
- * This will determine the IP being used for DHCP. The IP will be automatically
- * set to ap->own_ip.
- *
- * The address to set (or keep) is determined in this order:
- * 1. Address defined in provisioning file
- * 2. Address already set on interface
- * 3. Address in IP pool.
- *
- * Returns:  0 if an IP was successfully selected and needs to be set
- *          -EALREADY if an IP was already set on the interface
- *          -EEXIST if the IP pool ran out of IP's
- *          -EINVAL if there was an error.
+ * Append any address ranges within an input start/end range, which contain
+ * at least one full subnet and don't intersect with any subnets already in
+ * use.  This may result in the input range being split into multiple ranges
+ * of different sizes or being skipped altogether.
+ * All inputs must be rounded to the subnet boundary and the @used queue
+ * sorted by the subnet start address.
  */
-static int ap_setup_dhcp(struct ap_state *ap, struct l_settings *settings)
+static void ap_append_range(struct l_queue *to, const struct ap_ip_range *range,
+				struct l_queue *used, const char *str)
+{
+	const struct l_queue_entry *entry = l_queue_get_entries(used);
+	const struct ap_ip_range *used_range = entry ? entry->data : NULL;
+	uint32_t start = range->start;
+	bool print = true;
+
+	while (range->end > start) {
+		while (used_range && used_range->end <= start) {
+			entry = entry->next;
+			used_range = entry ? entry->data : NULL;
+		}
+
+		/* No more used ranges that intersect with @start/@range->end */
+		if (!used_range || range->end <= used_range->start) {
+			struct ap_ip_range *sub = l_new(struct ap_ip_range, 1);
+
+			sub->start = start;
+			sub->end = range->end;
+			l_queue_push_tail(to, sub);
+			l_queue_insert(used, l_memdup(sub, sizeof(*sub)),
+					ap_ip_range_compare, NULL);
+			return;
+		}
+
+		if (print) {
+			l_debug("Address spec %s intersects with at least one "
+				"subnet already in use on the system or "
+				"specified in the settings", str);
+			print = false;
+		}
+
+		/* Now we know @used_range is non-NULL and intersects */
+		if (start < used_range->start) {
+			struct ap_ip_range *sub = l_new(struct ap_ip_range, 1);
+
+			sub->start = start;
+			sub->end = used_range->start;
+			l_queue_push_tail(to, sub);
+			l_queue_insert(used, l_memdup(sub, sizeof(*sub)),
+					ap_ip_range_compare, NULL);
+		}
+
+		/* Skip to the start of the next subnet */
+		start = used_range->end;
+	}
+}
+
+/* Select a subnet and a host address from a defined space */
+static int ap_select_addr(const char **addr_str_list, uint8_t prefix,
+				const char *ifname, uint32_t *out_addr)
 {
-	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
+	uint32_t total = 0;
+	uint32_t selected;
+	unsigned int i;
+	uint32_t addr;
+	struct ifaddrs *ifaddr;
+	const struct ifaddrs *ifa;
+	uint32_t subnet_size = 1 << (32 - prefix);
+	uint32_t host_mask = subnet_size - 1;
+	uint32_t subnet_mask = ~host_mask;
+	uint32_t host_addr = 0;
+	struct l_queue *ranges = l_queue_new();
+	struct l_queue *used = l_queue_new();
 	struct in_addr ia;
-	uint32_t address = 0;
-	int ret = -EINVAL;
+	const struct l_queue_entry *entry;
+	int err = -EINVAL;
 
-	ap->server = l_dhcp_server_new(ifindex);
-	if (!ap->server) {
-		l_error("Failed to create DHCP server on %u", ifindex);
-		return -EINVAL;;
+	/* Build a sorted list of used/unavailable subnets */
+	if (getifaddrs(&ifaddr) != 0)
+		ifaddr = NULL;
+
+	for (ifa = ifaddr; ifa; ifa = ifa->ifa_next) {
+		const struct sockaddr_in *addr_sa;
+		const struct sockaddr_in *netmask_sa;
+		uint32_t used_addr;
+		uint32_t used_prefix;
+		struct ap_ip_range *range;
+
+		if (ifa->ifa_addr->sa_family != AF_INET)
+			continue;
+
+		/*
+		 * Ignore the subnet set on the target interface, we'll be
+		 * overwriting it if everything goes well.
+		 */
+		if (!strcmp(ifa->ifa_name, ifname))
+			continue;
+
+		addr_sa = (struct sockaddr_in *) ifa->ifa_addr;
+		netmask_sa = (struct sockaddr_in *) ifa->ifa_netmask;
+		if (!addr_sa || !netmask_sa)
+			continue;
+
+		used_addr = ntohl(addr_sa->sin_addr.s_addr);
+		used_prefix = __builtin_popcountl(netmask_sa->sin_addr.s_addr);
+
+		range = l_new(struct ap_ip_range, 1);
+		range->start = used_addr & subnet_mask;
+		range->end = (range->start + (1 << (32 - used_prefix)) +
+				subnet_size - 1) & subnet_mask;
+		l_queue_insert(used, range, ap_ip_range_compare, NULL);
 	}
 
-	if (getenv("IWD_DHCP_DEBUG"))
-		l_dhcp_server_set_debug(ap->server, do_debug,
-							"[DHCPv4 SERV] ", NULL);
+	freeifaddrs(ifaddr);
 
-	/* get the current address if there is one */
-	if (l_net_get_address(ifindex, &ia) && ia.s_addr != 0)
-		address = ia.s_addr;
+	/* Build the list of available subnets */
 
-	if (ap->config->profile) {
-		char *addr;
-
-		addr = l_settings_get_string(settings, "IPv4", "Address");
-		if (addr) {
-			if (inet_pton(AF_INET, addr, &ia) < 0)
-				goto free_addr;
-
-			/* Is a matching address already set on interface? */
-			if (ia.s_addr == address)
-				ret = -EALREADY;
-			else
-				ret = 0;
-		} else if (address) {
-			/* No address in config, but interface has one set */
-			addr = l_strdup(inet_ntoa(ia));
-			ret = -EALREADY;
-		} else
-			goto free_addr;
+	/* Check for the static IP syntax: Address=<IP> */
+	if (l_strv_length((char **) addr_str_list) == 1 &&
+			inet_pton(AF_INET, *addr_str_list, &ia) == 1) {
+		struct ap_ip_range range;
 
-		/* Set the remaining DHCP options in config file */
-		if (!dhcp_load_settings(ap, settings)) {
-			ret = -EINVAL;
-			goto free_addr;
+		host_addr = ntohl(ia.s_addr);
+		range.start = host_addr & subnet_mask;
+		range.end = range.start + subnet_size;
+		ap_append_range(ranges, &range, used, *addr_str_list);
+		goto select_addr;
+	}
+
+	for (i = 0; addr_str_list[i]; i++) {
+		struct ap_ip_range range;
+		uint8_t addr_prefix;
+
+		if (!util_ip_prefix_tohl(addr_str_list[i], &addr_prefix, &addr,
+						NULL, NULL)) {
+			l_error("Can't parse %s as a subnet address",
+				addr_str_list[i]);
+			goto done;
 		}
 
-		if (!l_dhcp_server_set_ip_address(ap->server, addr)) {
-			ret = -EINVAL;
-			goto free_addr;
+		if (addr_prefix > prefix) {
+			l_debug("Address spec %s smaller than requested "
+				"subnet (prefix len %i)", addr_str_list[i],
+				prefix);
+			continue;
 		}
 
-		ap->own_ip = l_strdup(addr);
+		range.start = addr & subnet_mask;
+		range.end = range.start + (1 << (32 - addr_prefix));
+		ap_append_range(ranges, &range, used, addr_str_list[i]);
+	}
+
+select_addr:
+	if (l_queue_isempty(ranges)) {
+		l_error("No IP subnets available for new Access Point after "
+			"eliminating those already in use on the system");
+		err = -EEXIST;
+		goto done;
+	}
 
-free_addr:
-		l_free(addr);
+	/* Count available @prefix-sized subnets */
+	for (entry = l_queue_get_entries(ranges); entry; entry = entry->next) {
+		struct ap_ip_range *range = entry->data;
 
-		return ret;
-	} else if (address) {
-		/* No config file and address is already set */
-		ap->own_ip = l_strdup(inet_ntoa(ia));
+		total += (range->end - range->start) >> (32 - prefix);
+	}
 
-		return -EALREADY;
-	} else if (pool.used) {
-		/* No config file, no address set. Use IP pool */
-		ap->own_ip = ip_pool_get();
-		if (!ap->own_ip) {
-			l_error("No more IP's in pool, cannot start AP on %u",
-					ifindex);
-			return -EEXIST;
-		}
+	selected = l_getrandom_uint32() % total;
 
-		ap->use_ip_pool = true;
-		ap->ip_prefix = pool.prefix;
+	/* Find the @selected'th @prefix-sized subnet */
+	for (entry = l_queue_get_entries(ranges);; entry = entry->next) {
+		struct ap_ip_range *range = entry->data;
+		uint32_t count = (range->end - range->start) >> (32 - prefix);
 
-		return 0;
+		if (selected < count) {
+			addr = range->start + (selected << (32 - prefix));
+			break;
+		}
+
+		selected -= count;
 	}
 
-	return -EINVAL;
+	if (!host_addr)
+		host_addr = addr + 1;
+
+	err = 0;
+	*out_addr = host_addr;
+
+done:
+	l_queue_destroy(ranges, l_free);
+	l_queue_destroy(used, l_free);
+	return err;
 }
 
-static int ap_load_profile_and_dhcp(struct ap_state *ap, bool *wait_dhcp)
+#define AP_DEFAULT_IPV4_PREFIX_LEN 28
+
+/*
+ * This will determine the IP to be used for DHCP.  ap->server,
+ * ap->own_ip and ap->ip_prefix will be set.
+ *
+ * The address to set (or keep) is determined in this order:
+ * 1. Select from address pool defined in the provisioning file
+ * 2. Keep address already set on the interface
+ * 3. Select from the global address pool (default 192.168.0.0/16)
+ *
+ * Returns:  0 if an IP was successfully selected and needs to be set,
+ *          -EALREADY if an IP is already set on the interface (success),
+ *          -EEXIST if all available subnet addresses are in use (error),
+ *          -EINVAL if there was a different error.
+ */
+static int ap_setup_dhcp(struct ap_state *ap, uint32_t existing_address,
+				uint8_t existing_prefix)
 {
 	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
-	char *passphrase;
-	struct l_settings *settings = NULL;
-	int err = -EINVAL;
+	struct in_addr if_addr;
+	struct in_addr if_netmask;
+	uint32_t address;
+	uint8_t prefix;
+	int ret = -EINVAL;
+	char **addr_str_list;
+	L_AUTO_FREE_VAR(char *, netmask_str) =
+		l_settings_get_string(ap->profile, "IPv4", "Netmask");
 
-	/* No profile or DHCP settings */
-	if (!ap->config->profile && !pool.used)
-		return 0;
+	ap->server = l_dhcp_server_new(ifindex);
+	if (!ap->server) {
+		l_error("Failed to create DHCP server on %u", ifindex);
+		return -EINVAL;
+	}
 
-	if (ap->config->profile) {
-		settings = l_settings_new();
+	if (getenv("IWD_DHCP_DEBUG"))
+		l_dhcp_server_set_debug(ap->server, do_debug,
+							"[DHCPv4 SERV] ", NULL);
 
-		if (!l_settings_load_from_file(settings, ap->config->profile))
-			goto cleanup;
+	if (ap->profile &&
+			l_settings_get_value(ap->profile, "IPv4", "Address")) {
+		addr_str_list = l_settings_get_string_list(ap->profile, "IPv4",
+								"Address", ',');
+		if (!addr_str_list || !*addr_str_list) {
+			l_error("Can't parse the profile [IPv4].Address "
+				"setting as a string list");
+			goto done;
+		}
+	} else if (existing_address) {
+		addr_str_list = NULL;
+	} else
+		addr_str_list = global_addr_strs;
 
-		passphrase = l_settings_get_string(settings, "Security",
-							"Passphrase");
-		if (passphrase) {
-			if (strlen(passphrase) > 63) {
-				l_error("[Security].Passphrase must not exceed "
-						"63 characters");
-				l_free(passphrase);
-				goto cleanup;
+	if (addr_str_list) {
+		int r;
+
+		if (netmask_str) {
+			uint32_t netmask;
+
+			if (inet_pton(AF_INET, netmask_str, &if_netmask) < 0) {
+				l_error("Can't parse the profile "
+					"[IPv4].Netmask setting");
+				goto done;
 			}
 
-			strcpy(ap->config->passphrase, passphrase);
-			l_free(passphrase);
-		}
+			netmask = if_netmask.s_addr;
+			prefix = __builtin_popcountl(netmask);
+			if ((ntohl(netmask) !=
+					util_netmask_from_prefix(prefix))) {
+				l_error("Invalid netmask %s", netmask_str);
+				goto done;
+			}
 
-		if (!l_settings_has_group(settings, "IPv4")) {
-			*wait_dhcp = false;
-			err = 0;
-			goto cleanup;
+			if (prefix > 30 || prefix < 8) {
+				l_error("Netmasks between 8 and 30 bits "
+					"are allowed");
+				goto done;
+			}
+		} else
+			prefix = AP_DEFAULT_IPV4_PREFIX_LEN;
+
+		r = ap_select_addr((const char **) addr_str_list, prefix,
+					netdev_get_name(ap->netdev), &address);
+		if (r < 0) {
+			ret = r;
+			goto done;
 		}
+	} else {
+		address = existing_address;
+		prefix = existing_prefix;
+	}
+
+	if_addr.s_addr = htonl(address);
+	if (!l_dhcp_server_set_ip_address(ap->server, inet_ntoa(if_addr))) {
+		l_error("l_dhcp_server_set_ip_address failed");
+		goto done;
+	}
+
+	if_netmask.s_addr = htonl(util_netmask_from_prefix(prefix));
+	if (!l_dhcp_server_set_netmask(ap->server, inet_ntoa(if_netmask))) {
+		l_error("l_dhcp_server_set_netmask failed");
+		goto done;
 	}
 
-	err = ap_setup_dhcp(ap, settings);
+	/* Set the remaining DHCP options in config file */
+	if (ap->profile && !ap_load_dhcp_settings(ap, address, prefix))
+		goto done;
+
+	ap->own_ip = l_strdup(inet_ntoa(if_addr));
+	ap->ip_prefix = prefix;
+	ret = (address == existing_address && prefix == existing_prefix) ?
+		-EALREADY : 0;
+
+done:
+	if (addr_str_list != global_addr_strs)
+		l_strv_free(addr_str_list);
+
+	return ret;
+}
+
+static void ap_netconfig_start(struct ap_state *ap, uint32_t existing_addr,
+				uint8_t existing_prefix)
+{
+	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
+	int err;
+
+	err = ap_setup_dhcp(ap, existing_addr, existing_prefix);
 	if (err == 0) {
 		/* Address change required */
 		ap->rtnl_add_cmd = l_rtnl_ifaddr4_add(rtnl, ifindex,
 					ap->ip_prefix, ap->own_ip,
 					broadcast_from_ip(ap->own_ip),
 					ap_ifaddr4_added_cb, ap, NULL);
-
-		if (!ap->rtnl_add_cmd) {
-			l_error("Failed to add IPv4 address");
-			err = -EIO;
-			goto cleanup;
+		if (ap->rtnl_add_cmd) {
+			ap->cleanup_ip = true;
+			return;
 		}
 
-		ap->cleanup_ip = true;
-
-		*wait_dhcp = true;
-		err = 0;
+		l_error("Failed to add IPv4 address");
 	/* Selected address already set, continue normally */
 	} else if (err == -EALREADY) {
-		*wait_dhcp = false;
-		err = 0;
+		if (ap_start_send(ap))
+			return;
 	}
 
-cleanup:
-	l_settings_free(settings);
-	return err;
+	ap_start_failed(ap);
+}
+
+static void ap_ifaddr4_dump_cb(int error,
+				uint16_t type, const void *data,
+				uint32_t len, void *user_data)
+{
+	struct ap_state *ap = user_data;
+	const struct ifaddrmsg *ifa = data;
+	char *ip_str = NULL;
+	struct in_addr ip = {};
+	uint8_t prefix = 0;
+	uint32_t cmd;
+
+	if (!error && ifa->ifa_index != netdev_get_ifindex(ap->netdev))
+		return;
+
+	if (error || type != RTM_NEWADDR ||
+			ifa->ifa_prefixlen > 30 || ifa->ifa_prefixlen < 1) {
+		l_error("Error getting existing IPv4 address on AP iface");
+		goto done;
+	}
+
+	l_rtnl_ifaddr4_extract(ifa, len, NULL, &ip_str, NULL);
+	inet_aton(ip_str, &ip);
+	l_free(ip_str);
+	prefix = ifa->ifa_prefixlen;
+
+done:
+	cmd = ap->rtnl_dump_cmd;
+	ap->rtnl_dump_cmd = 0;
+	l_netlink_cancel(rtnl, cmd);
+
+	ap_netconfig_start(ap, ntohl(ip.s_addr), prefix);
+}
+
+static void ap_ifaddr4_dump_destroy_cb(void *user_data)
+{
+	struct ap_state *ap = user_data;
+
+	if (!ap->rtnl_dump_cmd)
+		return;
+
+	ap->rtnl_dump_cmd = 0;
+	l_debug("No existing IPv4 addresses on AP iface");
+	ap_netconfig_start(ap, 0, 0);
 }
 
 /*
@@ -2639,13 +2815,10 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
 {
 	struct ap_state *ap;
 	struct wiphy *wiphy = netdev_get_wiphy(netdev);
-	struct l_genl_msg *cmd;
 	uint64_t wdev_id = netdev_get_wdev_id(netdev);
-	int err = -EINVAL;
-	bool wait_on_address = false;
 
 	if (err_out)
-		*err_out = err;
+		*err_out = -EINVAL;
 
 	if (L_WARN_ON(!config->ssid))
 		return NULL;
@@ -2661,15 +2834,33 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
 	ap->ops = ops;
 	ap->user_data = user_data;
 
-	/*
-	 * This both loads a profile if required and loads DHCP settings either
-	 * by the profile itself or the IP pool (or does nothing in the case
-	 * of a profile-less configuration). wait_on_address will be set true
-	 * if an address change is required.
-	 */
-	err = ap_load_profile_and_dhcp(ap, &wait_on_address);
-	if (err < 0)
-		goto error;
+	if (ap->config->profile) {
+		char *passphrase;
+
+		ap->profile = l_settings_new();
+
+		if (!l_settings_load_from_file(ap->profile, ap->config->profile))
+			goto error;
+
+		passphrase = l_settings_get_string(ap->profile, "Security",
+							"Passphrase");
+		if (!passphrase) {
+			l_error("[Security].Passphrase not found in "
+				"AP profile");
+			goto error;
+		}
+
+		/* (full checks done later in crypto_passphrase_is_valid) */
+		if (strlen(passphrase) > 63) {
+			l_error("[Security].Passphrase must not exceed "
+				"63 characters");
+			l_free(passphrase);
+			goto error;
+		}
+
+		strcpy(ap->config->passphrase, passphrase);
+		l_free(passphrase);
+	}
 
 	if (!config->channel)
 		/* TODO: Start a Get Survey to decide the channel */
@@ -2752,33 +2943,31 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
 	if (!ap->mlme_watch)
 		l_error("Registering for MLME notification failed");
 
-	if (wait_on_address) {
-		if (err_out)
-			*err_out = 0;
+	if (netconfig_enabled &&
+			(!ap->profile ||
+			 l_settings_has_group(ap->profile, "IPv4"))) {
+		ap->rtnl_dump_cmd = l_rtnl_ifaddr4_dump(rtnl,
+						ap_ifaddr4_dump_cb, ap,
+						ap_ifaddr4_dump_destroy_cb);
+		if (!ap->rtnl_dump_cmd) {
+			if (err_out)
+				*err_out = -EIO;
+
+			l_error("Sending the IPv4 addr dump req failed");
+			goto error;
+		}
 
 		return ap;
 	}
 
-	cmd = ap_build_cmd_start_ap(ap);
-	if (!cmd)
-		goto error;
+	if (ap_start_send(ap)) {
+		if (err_out)
+			*err_out = 0;
 
-	ap->start_stop_cmd_id = l_genl_family_send(ap->nl80211, cmd,
-							ap_start_cb, ap, NULL);
-	if (!ap->start_stop_cmd_id) {
-		l_genl_msg_unref(cmd);
-		goto error;
+		return ap;
 	}
 
-	if (err_out)
-		*err_out = 0;
-
-	return ap;
-
 error:
-	if (err_out)
-		*err_out = err;
-
 	ap->config = NULL;
 	ap_reset(ap);
 	l_genl_family_free(ap->nl80211);
@@ -3316,7 +3505,6 @@ static void ap_netdev_watch(struct netdev *netdev,
 static int ap_init(void)
 {
 	const struct l_settings *settings = iwd_get_config();
-	bool dhcp_enable;
 
 	netdev_watch = netdev_watch_add(ap_netdev_watch, NULL, NULL);
 
@@ -3327,31 +3515,44 @@ static int ap_init(void)
 			ap_diagnostic_interface_destroy, false);
 
 	/*
-	 * Reusing [General].EnableNetworkConfiguration as a switch to enable
-	 * DHCP server. If no value is found or it is false do not create a
-	 * DHCP server.
+	 * Enable network configuration and DHCP only if
+	 * [General].EnableNetworkConfiguration is true.
 	 */
 	if (!l_settings_get_bool(settings, "General",
-				"EnableNetworkConfiguration", &dhcp_enable))
-		dhcp_enable = false;
-
-	if (dhcp_enable) {
-		L_AUTO_FREE_VAR(char *, ip_prefix);
-
-		ip_prefix = l_settings_get_string(settings, "General",
-							"APRanges");
-		/*
-		 * In this case its assumed the user only cares about station
-		 * netconfig so we let ap_init pass but DHCP will not be
-		 * enabled.
-		 */
-		if (!ip_prefix) {
-			l_warn("[General].APRanges must be set for DHCP");
-			return 0;
+					"EnableNetworkConfiguration",
+					&netconfig_enabled))
+		netconfig_enabled = false;
+
+	if (netconfig_enabled) {
+		if (l_settings_get_value(settings, "IPv4", "APAddressPool")) {
+			global_addr_strs = l_settings_get_string_list(settings,
+								"IPv4",
+								"APAddressPool",
+								',');
+			if (!global_addr_strs || !*global_addr_strs) {
+				l_error("Can't parse the [IPv4].APAddressPool "
+					"setting as a string list");
+				l_strv_free(global_addr_strs);
+				global_addr_strs = NULL;
+			}
+		} else if (l_settings_get_value(settings,
+						"General", "APRanges")) {
+			global_addr_strs = l_settings_get_string_list(settings,
+								"General",
+								"APRanges",
+								',');
+			if (!global_addr_strs || !*global_addr_strs) {
+				l_error("Can't parse the [General].APRanges "
+					"setting as a string list");
+				l_strv_free(global_addr_strs);
+				global_addr_strs = NULL;
+			}
 		}
 
-		if (!ip_pool_create(ip_prefix))
-			return -EINVAL;
+		/* Fall back to 192.168.0.0/16 */
+		if (!global_addr_strs)
+			global_addr_strs =
+				l_strv_append(NULL, "192.168.0.0/16");
 	}
 
 	rtnl = iwd_get_rtnl();
@@ -3363,8 +3564,7 @@ static void ap_exit(void)
 {
 	netdev_watch_remove(netdev_watch);
 	l_dbus_unregister_interface(dbus_get_bus(), IWD_AP_INTERFACE);
-
-	ip_pool_destroy();
+	l_strv_free(global_addr_strs);
 }
 
 IWD_MODULE(ap, ap_init, ap_exit)
-- 
2.27.0

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

* [PATCH 2/5] ap: Send a specific error message on async AP start failure
  2021-03-02 12:08 [PATCH 1/5] ap: Refactor IP address selection Andrew Zaborowski
@ 2021-03-02 12:08 ` Andrew Zaborowski
  2021-03-02 12:08 ` [PATCH 3/5] ap: Move the DHCP server freeing to ap_reset Andrew Zaborowski
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Andrew Zaborowski @ 2021-03-02 12:08 UTC (permalink / raw)
  To: iwd

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

We generate the DBus error reply type from the errno only when
ap_start() was failing synchronously, now also send the errno through
the callbacks so that we can also return a specific DBus reply when
failing asynchronously.  Thea AP autotest relies on receiving the
AlreadyExists DBus error.
---
 src/ap.c | 34 +++++++++++++++++++---------------
 src/ap.h |  4 ++++
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index d871f973..c5e6c43e 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -1979,9 +1979,11 @@ static void do_debug(const char *str, void *user_data)
 	l_info("%s%s", prefix, str);
 }
 
-static void ap_start_failed(struct ap_state *ap)
+static void ap_start_failed(struct ap_state *ap, int err)
 {
-	ap->ops->handle_event(AP_EVENT_START_FAILED, NULL, ap->user_data);
+	struct ap_event_start_failed_data data = { err };
+
+	ap->ops->handle_event(AP_EVENT_START_FAILED, &data, ap->user_data);
 	ap_reset(ap);
 	l_genl_family_free(ap->nl80211);
 
@@ -1996,22 +1998,18 @@ static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
 
 	if (l_genl_msg_get_error(msg) < 0) {
 		l_error("START_AP failed: %i", l_genl_msg_get_error(msg));
-
-		goto failed;
+		ap_start_failed(ap, l_genl_msg_get_error(msg));
+		return;
 	}
 
 	if (ap->server && !l_dhcp_server_start(ap->server)) {
 		l_error("DHCP server failed to start");
-		goto failed;
+		ap_start_failed(ap, -EINVAL);
+		return;
 	}
 
 	ap->started = true;
 	ap->ops->handle_event(AP_EVENT_STARTED, NULL, ap->user_data);
-
-	return;
-
-failed:
-	ap_start_failed(ap);
 }
 
 static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
@@ -2122,12 +2120,12 @@ static void ap_ifaddr4_added_cb(int error, uint16_t type, const void *data,
 
 	if (error) {
 		l_error("Failed to set IP address");
-		ap_start_failed(ap);
+		ap_start_failed(ap, error);
 		return;
 	}
 
 	if (!ap_start_send(ap))
-		ap_start_failed(ap);
+		ap_start_failed(ap, -EIO);
 }
 
 static bool ap_parse_new_station_ies(const void *data, uint16_t len,
@@ -2298,10 +2296,12 @@ static void ap_mlme_notify(struct l_genl_msg *msg, void *user_data)
 	switch (l_genl_msg_get_command(msg)) {
 	case NL80211_CMD_STOP_AP:
 		if (ap->start_stop_cmd_id) {
+			struct ap_event_start_failed_data data = { -ECANCELED };
+
 			l_genl_family_cancel(ap->nl80211,
 						ap->start_stop_cmd_id);
 			ap->start_stop_cmd_id = 0;
-			ap->ops->handle_event(AP_EVENT_START_FAILED, NULL,
+			ap->ops->handle_event(AP_EVENT_START_FAILED, &data,
 						ap->user_data);
 		} else if (ap->started) {
 			ap->started = false;
@@ -2749,7 +2749,7 @@ static void ap_netconfig_start(struct ap_state *ap, uint32_t existing_addr,
 			return;
 	}
 
-	ap_start_failed(ap);
+	ap_start_failed(ap, err);
 }
 
 static void ap_ifaddr4_dump_cb(int error,
@@ -3155,13 +3155,17 @@ static void ap_if_event_func(enum ap_event_type type, const void *event_data,
 
 	switch (type) {
 	case AP_EVENT_START_FAILED:
+	{
+		const struct ap_event_start_failed_data *data = event_data;
+
 		if (L_WARN_ON(!ap_if->pending))
 			break;
 
-		reply = dbus_error_failed(ap_if->pending);
+		reply = dbus_error_from_errno(data->error, ap_if->pending);
 		dbus_pending_reply(&ap_if->pending, reply);
 		ap_if->ap = NULL;
 		break;
+	}
 
 	case AP_EVENT_STARTED:
 		if (L_WARN_ON(!ap_if->pending))
diff --git a/src/ap.h b/src/ap.h
index dc57a0bb..40b39bc0 100644
--- a/src/ap.h
+++ b/src/ap.h
@@ -34,6 +34,10 @@ enum ap_event_type {
 	AP_EVENT_PBC_MODE_EXIT,
 };
 
+struct ap_event_start_failed_data {
+	int error;
+};
+
 struct ap_event_station_added_data {
 	const uint8_t *mac;
 	const uint8_t *rsn_ie;
-- 
2.27.0

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

* [PATCH 3/5] ap: Move the DHCP server freeing to ap_reset
  2021-03-02 12:08 [PATCH 1/5] ap: Refactor IP address selection Andrew Zaborowski
  2021-03-02 12:08 ` [PATCH 2/5] ap: Send a specific error message on async AP start failure Andrew Zaborowski
@ 2021-03-02 12:08 ` Andrew Zaborowski
  2021-03-02 12:08 ` [PATCH 4/5] autotests: Update APRanges usage in testAP Andrew Zaborowski
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Andrew Zaborowski @ 2021-03-02 12:08 UTC (permalink / raw)
  To: iwd

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

Some callers of ap_reset(ap)/l_free(ap) would not call
l_dhcp_server_destroy() causing some likely leaks.  It's easier to call
it directly from inside ap_reset().
---
 src/ap.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index c5e6c43e..5425823a 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -250,8 +250,10 @@ static void ap_reset(struct ap_state *ap)
 	if (ap->own_ip)
 		l_free(ap->own_ip);
 
-	if (ap->server)
-		l_dhcp_server_stop(ap->server);
+	if (ap->server) {
+		l_dhcp_server_destroy(ap->server);
+		ap->server = NULL;
+	}
 
 	if (ap->profile) {
 		l_settings_free(ap->profile);
@@ -3075,8 +3077,6 @@ void ap_free(struct ap_state *ap)
 {
 	ap_reset(ap);
 	l_genl_family_free(ap->nl80211);
-	if (ap->server)
-		l_dhcp_server_destroy(ap->server);
 	l_free(ap);
 }
 
-- 
2.27.0

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

* [PATCH 4/5] autotests: Update APRanges usage in testAP
  2021-03-02 12:08 [PATCH 1/5] ap: Refactor IP address selection Andrew Zaborowski
  2021-03-02 12:08 ` [PATCH 2/5] ap: Send a specific error message on async AP start failure Andrew Zaborowski
  2021-03-02 12:08 ` [PATCH 3/5] ap: Move the DHCP server freeing to ap_reset Andrew Zaborowski
@ 2021-03-02 12:08 ` Andrew Zaborowski
  2021-03-02 12:08 ` [PATCH 5/5] doc: Update AP settings in iwd.ap and iwd.config(5) Andrew Zaborowski
  2021-03-03 17:14 ` [PATCH 1/5] ap: Refactor IP address selection Denis Kenzior
  4 siblings, 0 replies; 8+ messages in thread
From: Andrew Zaborowski @ 2021-03-02 12:08 UTC (permalink / raw)
  To: iwd

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

[General].APRange is now [IPv4].APAddressPool and the netmask is changed
from 23 to 27 bits to make the test correctly assert that only two
default-sized subnets are allowed by IWD simultaneously (default has
changed from 24 to 28 bits)
---
 autotests/testAP/dhcp/main.conf |  4 +++-
 autotests/testAP/dhcp_test.py   | 12 +++++++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/autotests/testAP/dhcp/main.conf b/autotests/testAP/dhcp/main.conf
index 667de23c..62ae782d 100644
--- a/autotests/testAP/dhcp/main.conf
+++ b/autotests/testAP/dhcp/main.conf
@@ -3,4 +3,6 @@ DisableMacAddressRandomization=true
 
 [General]
 EnableNetworkConfiguration=true
-APRanges=192.168.80.0/23
+
+[IPv4]
+APAddressPool=192.168.80.0/27
diff --git a/autotests/testAP/dhcp_test.py b/autotests/testAP/dhcp_test.py
index bf52fb28..e69c4465 100644
--- a/autotests/testAP/dhcp_test.py
+++ b/autotests/testAP/dhcp_test.py
@@ -58,8 +58,14 @@ class Test(unittest.TestCase):
             testutil.test_iface_operstate(dev2.name)
             testutil.test_ifaces_connected(dev1.name, dev2.name, group=False)
 
-            testutil.test_ip_address_match(dev1.name, "192.168.80.1")
-            testutil.test_ip_address_match(dev2.name, "192.168.80.2")
+            try:
+                testutil.test_ip_address_match(dev1.name, "192.168.80.1")
+                testutil.test_ip_address_match(dev2.name, "192.168.80.2")
+                ip = "192.168.80.1"
+            except:
+                testutil.test_ip_address_match(dev1.name, "192.168.80.17")
+                testutil.test_ip_address_match(dev2.name, "192.168.80.18")
+                ip = "192.168.80.17"
 
             wd.unregister_psk_agent(psk_agent)
 
@@ -76,7 +82,7 @@ class Test(unittest.TestCase):
             # got initially.
             dev4.start_ap('TestAP4', 'Password4')
 
-            testutil.test_ip_address_match(dev4.name, "192.168.80.1")
+            testutil.test_ip_address_match(dev4.name, ip)
 
         finally:
             dev1.stop_ap()
-- 
2.27.0

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

* [PATCH 5/5] doc: Update AP settings in iwd.ap and iwd.config(5)
  2021-03-02 12:08 [PATCH 1/5] ap: Refactor IP address selection Andrew Zaborowski
                   ` (2 preceding siblings ...)
  2021-03-02 12:08 ` [PATCH 4/5] autotests: Update APRanges usage in testAP Andrew Zaborowski
@ 2021-03-02 12:08 ` Andrew Zaborowski
  2021-03-03 17:14 ` [PATCH 1/5] ap: Refactor IP address selection Denis Kenzior
  4 siblings, 0 replies; 8+ messages in thread
From: Andrew Zaborowski @ 2021-03-02 12:08 UTC (permalink / raw)
  To: iwd

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

Update the AP-mode address logic documentation, change some of the
wording and add some references.
---
 src/iwd.ap.rst     | 69 +++++++++++++++++++++++++++++-----------------
 src/iwd.config.rst | 31 +++++++++++++--------
 2 files changed, 63 insertions(+), 37 deletions(-)

diff --git a/src/iwd.ap.rst b/src/iwd.ap.rst
index 52312855..e2ec01e6 100644
--- a/src/iwd.ap.rst
+++ b/src/iwd.ap.rst
@@ -3,7 +3,7 @@
 ============
 
 --------------------------------------
-Configuration of IWD access point
+Configuration of IWD access points
 --------------------------------------
 
 :Author: James Prestwood <prestwoj@gmail.com>
@@ -25,19 +25,21 @@ Description of access point provisioning files.
 DESCRIPTION
 ===========
 
-An access point provisioning files define the configuration of an IWD access
+An access point provisioning file defines the configuration of an IWD access
 point. These files live in *$STATE_DIRECTORY*/ap (/var/lib/iwd/ap by default).
+They are read when the `net.connman.iwd.AccessPoint.StartProfile(ssid)` DBus
+method is used.
 
 FILE FORMAT
 ===========
 
-See *iwd.network* for details on the file format.
+See *iwd.network* for details on the settings file syntax.
 
 SETTINGS
 ========
 
 The settings are split into several categories.  Each category has a group
-associated with it and described in separate tables below.
+associated with it and is described in the corresponding table below.
 
 Network Authentication Settings
 -------------------------------
@@ -54,13 +56,15 @@ configuration.
    * - Passphrase
      - 8..63 character string
 
-       Passphrase to be used with this access point.
+       WPA PSK Passphrase to be used with this access point.
 
-DHCP Server Settings
---------------------
+IPv4 Network Configuration
+--------------------------
 
-The group ``[IPv4]`` contains settings for IWD's built in DHCP server. All
-settings are optional.
+The group ``[IPv4]`` contains settings for IWD's built-in DHCP server. All
+settings are optional.  They're used if network configuration was enabled as
+described in ``iwd.config(5)``.  Omitting the ``[IPv4]`` group disables
+network configuration and the DHCP server for this access point.
 
 .. list-table::
    :header-rows: 0
@@ -68,41 +72,56 @@ settings are optional.
    :widths: 20 80
 
    * - Address
-     - IP Address of AP
-
-       Optional address for the DHCP server/access point. If provided this
-       address will be set on the AP interface and any other DHCP server options
-       will be derived from this address, unless they are overriden inside the
-       AP profile. If [IPv4].Address is not provided and no IP address is set
-       on the interface prior to calling StartProfile the IP pool will be used.
+     - Local IP address or a comma-separated list of prefix-notation addresses
+
+       Optional local address pool for the access point and the DHCP server.
+       If a single address is provided this address will be set on the AP
+       interface and any other DHCP server options will be derived from it,
+       unless they are overriden by other settings.  If a list of addresses and
+       prefix lengths is given (in the `<IP>/<prefix-len>` format), a single
+       subnet address will be selected from the available space each time this
+       profile is started.  The subnet size will still be based on the
+       ``[IPv4].Netmask`` setting.  If ``[IPv4].Address`` is not provided and
+       no IP address is set on the interface prior to calling `StartProfile`
+       the value of the main.conf ``[IPv4].APAddressPool`` setting will be
+       inherited, which in turn defaults to 192.168.0.0/16.
+
+       For example, if ``[IPv4].Netmask`` is set the 255.255.255.0 and this
+       setting, or the global ``APAddressPool`` fallback, is set to
+       ``192.168.0.0/16, 10.0.0.0/22``, IWD will select one of the 256 subnets
+       with addresses in the 192.168.<0-255>.0/24 range or one of the 4 subnets
+       with addresses in the 10.0.<0-3>.0/24 range, allowing 270 possible
+       subnets.  Defining an address pool larger than the subnet may be
+       desired if other interfaces on the system use dynamically assigned
+       addresses giving IWD a chance to avoid conflicts.
 
    * - Gateway
      - IP Address of gateway
 
-       IP address of gateway. This will inherit from [IPv4].Address if not
-       provided.
+       IP address of the gateway to be advertised by DHCP. This will fall back
+       to the local IP address if not provided.
 
    * - Netmask
-     - Netmask of DHCP server
+     - Local netmask of the AP
 
-       This will be generated from [IPv4].Address if not provided.
+       Defaults to a 28-bit netmask if not provided.
 
    * - DNSList
-     - List of DNS servers
+     - List of DNS servers as a comma-separated IP address list
 
        A list of DNS servers which will be advertised by the DHCP server. If
        not provided no DNS servers will be sent by the DHCP server.
 
    * - LeaseTime
-     - Time limit for DHCP leases
+     - Time limit for DHCP leases in seconds
 
        Override the default lease time.
 
    * - IPRange
-     - Range of IPs to use for the DHCP server
+     - Range of IPs given as two addresses separated by a comma
 
-       If not provided a default range will be chosen which is the DHCP server
-       address + 1 to 254.
+       From and to addresses of the range assigned to clients through DHCP.
+       If not provided the range from local address + 1 to .254 will be used.
 
 SEE ALSO
 ========
diff --git a/src/iwd.config.rst b/src/iwd.config.rst
index 478bd616..e3dc2827 100644
--- a/src/iwd.config.rst
+++ b/src/iwd.config.rst
@@ -67,23 +67,13 @@ The group ``[General]`` contains general settings.
        obtain the dynamic addresses from the network through the built-in
        DHCP client.
 
-       This also enables DHCP server when in AP mode when either
-       [General].APRanges is set or an AP profile is being used.
+       This also enables network configuration and the DHCP server when in AP
+       mode and the AP profile being activated does does not override it.
 
        The network configuration feature is disabled by default.  See
        ``[Network]`` settings for additional settings related to network
        configuration.
 
-   * - APRanges
-     - Values: <IP in prefix notation>
-
-       Sets the range of IP's used for DHCP server (AP mode). The IP should be
-       in prefix notation e.g. 192.168.1.0/24. AP's which are started in a
-       profile-less configuration will use this pool of IP's to set the AP's
-       interface address as well as default DHCP server options. Each AP will
-       get a new subnet from the range and clients will be addressed in that
-       subnet to avoid IP conflicts if multiple AP's are started.
-
    * - UseDefaultInterface
      - Values: true, **false**
 
@@ -312,6 +302,23 @@ No modification from defaults is normally required.
        prevent **iwd** from roaming properly, but can be useful for networks
        operating under extremely low rssi levels where roaming isn't possible.
 
+IPv4
+----
+
+The group ``[IPv4]`` contains settings related to IPv4 address configuration.
+
+   * - APAddressPool
+     - Values: comma-separated list of prefix-notation IPv4 address strings
+
+       Defines the space of IPs used for the Access Point-mode subnet addresses
+       and the DHCP server.  Defaults to 192.168.0.0/16.  The prefix length
+       decides the size of the pool from which an address is selected but the
+       actual subnet size (netmask) is based on the AP profile being activated
+       and defaults to 28 bits.  The AP profile's ``[IPv4].Address`` setting
+       overrides the global value set here.  Setting a too small address space
+       will limit the number of access points that can be running
+       simultaneously on different interfaces.
+
 SEE ALSO
 ========
 
-- 
2.27.0

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

* Re: [PATCH 1/5] ap: Refactor IP address selection
  2021-03-02 12:08 [PATCH 1/5] ap: Refactor IP address selection Andrew Zaborowski
                   ` (3 preceding siblings ...)
  2021-03-02 12:08 ` [PATCH 5/5] doc: Update AP settings in iwd.ap and iwd.config(5) Andrew Zaborowski
@ 2021-03-03 17:14 ` Denis Kenzior
  2021-03-03 19:17   ` Andrew Zaborowski
  4 siblings, 1 reply; 8+ messages in thread
From: Denis Kenzior @ 2021-03-03 17:14 UTC (permalink / raw)
  To: iwd

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

Hi Andrew,

On 3/2/21 6:08 AM, Andrew Zaborowski wrote:
> Refactor how the subnet IP address is selected and change related
> settings syntax in the following way:
> 
> * [IPv4].Address in an AP profile can now be given as just an IP address
>    or a list of prefix notation address spaces.  In the latter case one
>    subnet address of the size implied by [IPv4].Netmask is selected
>    randomly from that space, defaults to a 28-bit netmask.  In the former
>    case [IPv4].Address is now also checked for conflicts with addresses
>    already in use.
> 
> * main.conf's [IPv4].APAddressPool is now the fallback setting that uses
>    the same syntax as profile-local [IPv4].Address and has a default
>    value of 192.168.0.0/16.
> 
> * main.conf's [General].APRanges is deprecated in favour of
>    [IPv4].APAddressPool but this change should be backwards compatible.
> 
> ap_setup_dhcp now always prints error messages, it seems it was assumed
> that the caller would do this before but it didn't.  It also now always
> reads [IPv4].Netmask or the value already set on the interface (when
> existing IP is used) and uses that value.  Previously the configured
> netmask was only being announced through DHCP but not used locally.
> Require that the WPA-PSK passphrase is present in the AP profile,
> previously it was treated as optional but the rest of the code was
> expecting it to always be present.

So I really wish this patch was broken up more.  Reviewing 800 line patchsets is 
already a pain.  And you have a bunch of different things going on here.

> 
> Drop the l_net_get_address() usage, replace with the more complicated
> l_rtnl_* calls.  This is to avoid also adding an l_net_get_netmask() to
> ell since we'd need that and we reportely don't want to keep using the
> netdev ioctls.

This is really metadata / change log.  Does not belong in the description.

> ---
>   src/ap.c | 804 ++++++++++++++++++++++++++++++++++---------------------
>   1 file changed, 502 insertions(+), 302 deletions(-)
> 
> diff --git a/src/ap.c b/src/ap.c
> index 2042cde6..d871f973 100644
> --- a/src/ap.c
> +++ b/src/ap.c
> @@ -28,6 +28,7 @@
>   #include <linux/if_ether.h>
>   #include <netinet/in.h>
>   #include <arpa/inet.h>
> +#include <ifaddrs.h>
>   
>   #include <ell/ell.h>
>   
> @@ -78,15 +79,16 @@ struct ap_state {
>   	uint16_t last_aid;
>   	struct l_queue *sta_states;
>   
> +	struct l_settings *profile;
>   	struct l_dhcp_server *server;
> +	uint32_t rtnl_dump_cmd;
>   	uint32_t rtnl_add_cmd;
>   	char *own_ip;
> -	unsigned int ip_prefix;
> +	uint8_t ip_prefix;
>   
>   	bool started : 1;
>   	bool gtk_set : 1;
>   	bool cleanup_ip : 1;
> -	bool use_ip_pool : 1;
>   };
>   
>   struct sta_state {
> @@ -115,99 +117,11 @@ struct ap_wsc_pbc_probe_record {
>   	uint64_t timestamp;
>   };
>   
> -struct ap_ip_pool {
> -	uint32_t start;
> -	uint32_t end;
> -	uint8_t prefix;
> -
> -	/* Fist/last valid subnet */
> -	uint8_t sub_start;
> -	uint8_t sub_end;
> -
> -	struct l_uintset *used;
> -};
> -
> -struct ap_ip_pool pool;
> +static bool netconfig_enabled;
> +static char **global_addr_strs;
>   static uint32_t netdev_watch;
>   struct l_netlink *rtnl;
>   
> -/*
> - * Creates pool of IPs which AP intefaces can use. Each call to ip_pool_get
> - * will advance the subnet +1 so there are no IP conflicts between AP
> - * interfaces
> - */
> -static bool ip_pool_create(const char *ip_prefix)
> -{
> -	if (!util_ip_prefix_tohl(ip_prefix, &pool.prefix, &pool.start,
> -					&pool.end, NULL))
> -		return false;
> -
> -	if (pool.prefix > 24) {
> -		l_error("APRanges prefix must 24 or less (%u used)",
> -				pool.prefix);
> -		memset(&pool, 0, sizeof(pool));
> -		return false;
> -	}
> -
> -	/*
> -	 * Find the number of subnets we can use, this will dictate the number
> -	 * of AP interfaces that can be created (when using DHCP)
> -	 */
> -	pool.sub_start = (pool.start & 0x0000ff00) >> 8;
> -	pool.sub_end = (pool.end & 0x0000ff00) >> 8;
> -
> -	pool.used = l_uintset_new_from_range(pool.sub_start, pool.sub_end);
> -
> -	return true;
> -}
> -
> -static char *ip_pool_get()
> -{
> -	uint32_t ip;
> -	struct in_addr ia;
> -	uint8_t next_subnet = (uint8_t)l_uintset_find_unused_min(pool.used);
> -
> -	/* This shouldn't happen */
> -	if (next_subnet < pool.sub_start || next_subnet > pool.sub_end)
> -		return NULL;
> -
> -	l_uintset_put(pool.used, next_subnet);
> -
> -	ip = pool.start;
> -	ip &= 0xffff00ff;
> -	ip |= (next_subnet << 8);
> -
> -	ia.s_addr = htonl(ip);
> -	return l_strdup(inet_ntoa(ia));
> -}
> -
> -static bool ip_pool_put(const char *address)
> -{
> -	struct in_addr ia;
> -	uint32_t ip;
> -	uint8_t subnet;
> -
> -	if (inet_aton(address, &ia) < 0)
> -		return false;
> -
> -	ip = ntohl(ia.s_addr);
> -
> -	subnet = (ip & 0x0000ff00) >> 8;
> -
> -	if (subnet < pool.sub_start || subnet > pool.sub_end)
> -		return false;
> -
> -	return l_uintset_take(pool.used, subnet);
> -}
> -
> -static void ip_pool_destroy()
> -{
> -	if (pool.used)
> -		l_uintset_free(pool.used);
> -
> -	memset(&pool, 0, sizeof(pool));
> -}
> -
>   static const char *broadcast_from_ip(const char *ip)
>   {
>   	struct in_addr ia;
> @@ -304,6 +218,13 @@ static void ap_reset(struct ap_state *ap)
>   	if (ap->start_stop_cmd_id)
>   		l_genl_family_cancel(ap->nl80211, ap->start_stop_cmd_id);
>   
> +	if (ap->rtnl_dump_cmd) {
> +		uint32_t cmd = ap->rtnl_dump_cmd;
> +
> +		ap->rtnl_dump_cmd = 0;
> +		l_netlink_cancel(rtnl, cmd);

Why is this so complicated?  Isn't rtnl_dump_cmd reset in the destructor?

> +	}
> +
>   	if (ap->rtnl_add_cmd)
>   		l_netlink_cancel(rtnl, ap->rtnl_add_cmd);
>   
> @@ -326,16 +247,16 @@ static void ap_reset(struct ap_state *ap)
>   					broadcast_from_ip(ap->own_ip),
>   					NULL, NULL, NULL);
>   
> -	if (ap->own_ip) {
> -		/* Release IP from pool if used */
> -		if (ap->use_ip_pool)
> -			ip_pool_put(ap->own_ip);
> -
> +	if (ap->own_ip)
>   		l_free(ap->own_ip);
> -	}
>   
>   	if (ap->server)
>   		l_dhcp_server_stop(ap->server);
> +
> +	if (ap->profile) {
> +		l_settings_free(ap->profile);
> +		ap->profile = NULL;
> +	}
>   }
>   
>   static void ap_del_station(struct sta_state *sta, uint16_t reason,
> @@ -2172,34 +2093,41 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
>   	return cmd;
>   }
>   
> -static void ap_ifaddr4_added_cb(int error, uint16_t type, const void *data,
> -				uint32_t len, void *user_data)
> +static bool ap_start_send(struct ap_state *ap)
>   {
> -	struct ap_state *ap = user_data;
> -	struct l_genl_msg *cmd;
> -
> -	ap->rtnl_add_cmd = 0;
> +	struct l_genl_msg *cmd = ap_build_cmd_start_ap(ap);
>   
> -	if (error) {
> -		l_error("Failed to set IP address");
> -		goto error;
> +	if (!cmd) {
> +		l_error("ap_build_cmd_start_ap failed");
> +		return false;
>   	}
>   
> -	cmd = ap_build_cmd_start_ap(ap);
> -	if (!cmd)
> -		goto error;
> -
>   	ap->start_stop_cmd_id = l_genl_family_send(ap->nl80211, cmd,
>   							ap_start_cb, ap, NULL);
>   	if (!ap->start_stop_cmd_id) {
> +		l_error("AP_START l_genl_family_send failed");
>   		l_genl_msg_unref(cmd);
> -		goto error;
> +		return false;
>   	}
>   
> -	return;
> +	return true;
> +}
>   
> -error:
> -	ap_start_failed(ap);
> +static void ap_ifaddr4_added_cb(int error, uint16_t type, const void *data,
> +				uint32_t len, void *user_data)
> +{
> +	struct ap_state *ap = user_data;
> +
> +	ap->rtnl_add_cmd = 0;
> +
> +	if (error) {
> +		l_error("Failed to set IP address");
> +		ap_start_failed(ap);
> +		return;
> +	}
> +
> +	if (!ap_start_send(ap))
> +		ap_start_failed(ap);
>   }
>   
>   static bool ap_parse_new_station_ies(const void *data, uint16_t len,
> @@ -2394,23 +2322,21 @@ static void ap_mlme_notify(struct l_genl_msg *msg, void *user_data)
>   	}
>   }
>   
> -static bool dhcp_load_settings(struct ap_state *ap, struct l_settings *settings)
> +static bool ap_load_dhcp_settings(struct ap_state *ap,
> +					uint32_t own_ip, uint8_t prefix)

One thing I don't like about this is that the primary IP selection is done 
elsewhere, but then Netmask/IPRange settings are parsed after the fact.  I'm not 
sure if it makes sense for a profile to have Netmask/IPRange settings if it also 
doesn't have the Address set to a single subnet.  l_dhcp_server probably only 
works with prefixes >= 24 when allocating addresses.  Shouldn't we load the 
profile and verify it mostly in one place?

Things like DNSList and LeaseTime are probably OK done separately, since they're 
not dependent on the IP selection.

>   {
>   	struct l_dhcp_server *server = ap->server;
> -	struct in_addr ia;
>   
> -	L_AUTO_FREE_VAR(char *, netmask) = l_settings_get_string(settings,
> -							"IPv4", "Netmask");
> -	L_AUTO_FREE_VAR(char *, gateway) = l_settings_get_string(settings,
> +	L_AUTO_FREE_VAR(char *, gateway) = l_settings_get_string(ap->profile,
>   							"IPv4", "Gateway");
> -	char **dns = l_settings_get_string_list(settings, "IPv4",
> +	char **dns = l_settings_get_string_list(ap->profile, "IPv4",
>   							"DNSList", ',');
> -	char **ip_range = l_settings_get_string_list(settings, "IPv4",
> +	char **ip_range = l_settings_get_string_list(ap->profile, "IPv4",
>   							"IPRange", ',');
>   	unsigned int lease_time;
>   	bool ret = false;
>   
> -	if (!l_settings_get_uint(settings, "IPv4", "LeaseTime", &lease_time))
> +	if (!l_settings_get_uint(ap->profile, "IPv4", "LeaseTime", &lease_time))
>   		lease_time = 0;
>   
>   	if (gateway && !l_dhcp_server_set_gateway(server, gateway)) {
> @@ -2423,17 +2349,34 @@ static bool dhcp_load_settings(struct ap_state *ap, struct l_settings *settings)
>   		goto error;
>   	}
>   
> -	if (netmask && !l_dhcp_server_set_netmask(server, netmask)) {
> -		l_error("[IPv4].Netmask value error");
> -		goto error;
> -	}
> -
>   	if (ip_range) {
> +		int i;
> +
>   		if (l_strv_length(ip_range) != 2) {
>   			l_error("Two addresses expected in [IPv4].IPRange");
>   			goto error;
>   		}
>   
> +		for (i = 0; i < 2; i++) {
> +			struct in_addr range_addr;
> +
> +			if (inet_aton(ip_range[i], &range_addr) != 1) {
> +				l_error("Can't parse address in "
> +					"[IPv4].IPRange[%i]", i + 1);
> +				goto error;
> +			}
> +
> +			if ((own_ip ^ ntohl(range_addr.s_addr)) &
> +					util_netmask_from_prefix(prefix)) {
> +				struct in_addr addr = { htonl(own_ip) };
> +
> +				l_error("[IPv4].IPRange[%i] is not in the "
> +					"%s/%i subnet", i + 1,
> +					inet_ntoa(addr), prefix);
> +				goto error;
> +			}
> +		}
> +
>   		if (!l_dhcp_server_set_ip_range(server, ip_range[0],
>   							ip_range[1])) {
>   			l_error("Error setting IP range from [IPv4].IPRange");
> @@ -2446,11 +2389,6 @@ static bool dhcp_load_settings(struct ap_state *ap, struct l_settings *settings)
>   		goto error;
>   	}
>   
> -	if (netmask && inet_pton(AF_INET, netmask, &ia) > 0)
> -		ap->ip_prefix = __builtin_popcountl(ia.s_addr);
> -	else
> -		ap->ip_prefix = 24;
> -
>   	ret = true;
>   
>   error:
> @@ -2459,166 +2397,404 @@ error:
>   	return ret;
>   }
>   
> +struct ap_ip_range {
> +	uint32_t start;
> +	uint32_t end;
> +};
> +
> +static int ap_ip_range_compare(const void *a, const void *b, void *user_data)
> +{
> +	const struct ap_ip_range *range_a = a;
> +	const struct ap_ip_range *range_b = b;
> +
> +	return range_a->start > range_b->start;
> +}
> +
>   /*
> - * This will determine the IP being used for DHCP. The IP will be automatically
> - * set to ap->own_ip.
> - *
> - * The address to set (or keep) is determined in this order:
> - * 1. Address defined in provisioning file
> - * 2. Address already set on interface
> - * 3. Address in IP pool.
> - *
> - * Returns:  0 if an IP was successfully selected and needs to be set
> - *          -EALREADY if an IP was already set on the interface
> - *          -EEXIST if the IP pool ran out of IP's
> - *          -EINVAL if there was an error.
> + * Append any address ranges within an input start/end range, which contain
> + * at least one full subnet and don't intersect with any subnets already in
> + * use.  This may result in the input range being split into multiple ranges
> + * of different sizes or being skipped altogether.
> + * All inputs must be rounded to the subnet boundary and the @used queue
> + * sorted by the subnet start address.
>    */
> -static int ap_setup_dhcp(struct ap_state *ap, struct l_settings *settings)
> +static void ap_append_range(struct l_queue *to, const struct ap_ip_range *range,
> +				struct l_queue *used, const char *str)
> +{
> +	const struct l_queue_entry *entry = l_queue_get_entries(used);
> +	const struct ap_ip_range *used_range = entry ? entry->data : NULL;
> +	uint32_t start = range->start;
> +	bool print = true;
> +
> +	while (range->end > start) {
> +		while (used_range && used_range->end <= start) {
> +			entry = entry->next;
> +			used_range = entry ? entry->data : NULL;
> +		}
> +
> +		/* No more used ranges that intersect with @start/@range->end */
> +		if (!used_range || range->end <= used_range->start) {
> +			struct ap_ip_range *sub = l_new(struct ap_ip_range, 1);
> +
> +			sub->start = start;
> +			sub->end = range->end;
> +			l_queue_push_tail(to, sub);
> +			l_queue_insert(used, l_memdup(sub, sizeof(*sub)),
> +					ap_ip_range_compare, NULL);
> +			return;
> +		}
> +
> +		if (print) {
> +			l_debug("Address spec %s intersects with at least one "
> +				"subnet already in use on the system or "
> +				"specified in the settings", str);
> +			print = false;
> +		}
> +
> +		/* Now we know @used_range is non-NULL and intersects */
> +		if (start < used_range->start) {
> +			struct ap_ip_range *sub = l_new(struct ap_ip_range, 1);
> +
> +			sub->start = start;
> +			sub->end = used_range->start;
> +			l_queue_push_tail(to, sub);
> +			l_queue_insert(used, l_memdup(sub, sizeof(*sub)),
> +					ap_ip_range_compare, NULL);
> +		}
> +
> +		/* Skip to the start of the next subnet */
> +		start = used_range->end;
> +	}
> +}

Can we split out the address allocation logic out into a separate file so that 
we can more easily add unit tests for it?

> +
> +/* Select a subnet and a host address from a defined space */
> +static int ap_select_addr(const char **addr_str_list, uint8_t prefix,
> +				const char *ifname, uint32_t *out_addr)
>   {
> -	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
> +	uint32_t total = 0;
> +	uint32_t selected;
> +	unsigned int i;
> +	uint32_t addr;
> +	struct ifaddrs *ifaddr;
> +	const struct ifaddrs *ifa;
> +	uint32_t subnet_size = 1 << (32 - prefix);
> +	uint32_t host_mask = subnet_size - 1;
> +	uint32_t subnet_mask = ~host_mask;
> +	uint32_t host_addr = 0;
> +	struct l_queue *ranges = l_queue_new();
> +	struct l_queue *used = l_queue_new();
>   	struct in_addr ia;
> -	uint32_t address = 0;
> -	int ret = -EINVAL;
> +	const struct l_queue_entry *entry;
> +	int err = -EINVAL;
>   
> -	ap->server = l_dhcp_server_new(ifindex);
> -	if (!ap->server) {
> -		l_error("Failed to create DHCP server on %u", ifindex);
> -		return -EINVAL;;
> +	/* Build a sorted list of used/unavailable subnets */
> +	if (getifaddrs(&ifaddr) != 0)
> +		ifaddr = NULL;

So you already made an RTNL dump elsewhere, and are now effectively doing it 
again.  Can we not combine the two somehow?

> +
> +	for (ifa = ifaddr; ifa; ifa = ifa->ifa_next) {
> +		const struct sockaddr_in *addr_sa;
> +		const struct sockaddr_in *netmask_sa;
> +		uint32_t used_addr;
> +		uint32_t used_prefix;
> +		struct ap_ip_range *range;
> +
> +		if (ifa->ifa_addr->sa_family != AF_INET)
> +			continue;
> +
> +		/*
> +		 * Ignore the subnet set on the target interface, we'll be
> +		 * overwriting it if everything goes well.
> +		 */
> +		if (!strcmp(ifa->ifa_name, ifname))
> +			continue;
> +
> +		addr_sa = (struct sockaddr_in *) ifa->ifa_addr;
> +		netmask_sa = (struct sockaddr_in *) ifa->ifa_netmask;
> +		if (!addr_sa || !netmask_sa)
> +			continue;
> +
> +		used_addr = ntohl(addr_sa->sin_addr.s_addr);
> +		used_prefix = __builtin_popcountl(netmask_sa->sin_addr.s_addr);
> +
> +		range = l_new(struct ap_ip_range, 1);
> +		range->start = used_addr & subnet_mask;
> +		range->end = (range->start + (1 << (32 - used_prefix)) +
> +				subnet_size - 1) & subnet_mask;
> +		l_queue_insert(used, range, ap_ip_range_compare, NULL);
>   	}
>   
> -	if (getenv("IWD_DHCP_DEBUG"))
> -		l_dhcp_server_set_debug(ap->server, do_debug,
> -							"[DHCPv4 SERV] ", NULL);
> +	freeifaddrs(ifaddr);
>   
> -	/* get the current address if there is one */
> -	if (l_net_get_address(ifindex, &ia) && ia.s_addr != 0)
> -		address = ia.s_addr;
> +	/* Build the list of available subnets */
>   
> -	if (ap->config->profile) {
> -		char *addr;
> -
> -		addr = l_settings_get_string(settings, "IPv4", "Address");
> -		if (addr) {
> -			if (inet_pton(AF_INET, addr, &ia) < 0)
> -				goto free_addr;
> -
> -			/* Is a matching address already set on interface? */
> -			if (ia.s_addr == address)
> -				ret = -EALREADY;
> -			else
> -				ret = 0;
> -		} else if (address) {
> -			/* No address in config, but interface has one set */
> -			addr = l_strdup(inet_ntoa(ia));
> -			ret = -EALREADY;
> -		} else
> -			goto free_addr;
> +	/* Check for the static IP syntax: Address=<IP> */
> +	if (l_strv_length((char **) addr_str_list) == 1 &&
> +			inet_pton(AF_INET, *addr_str_list, &ia) == 1) {
> +		struct ap_ip_range range;
>   
> -		/* Set the remaining DHCP options in config file */
> -		if (!dhcp_load_settings(ap, settings)) {
> -			ret = -EINVAL;
> -			goto free_addr;
> +		host_addr = ntohl(ia.s_addr);
> +		range.start = host_addr & subnet_mask;
> +		range.end = range.start + subnet_size;
> +		ap_append_range(ranges, &range, used, *addr_str_list);
> +		goto select_addr;
> +	}
> +
> +	for (i = 0; addr_str_list[i]; i++) {
> +		struct ap_ip_range range;
> +		uint8_t addr_prefix;
> +
> +		if (!util_ip_prefix_tohl(addr_str_list[i], &addr_prefix, &addr,
> +						NULL, NULL)) {
> +			l_error("Can't parse %s as a subnet address",
> +				addr_str_list[i]);
> +			goto done;
>   		}
>   
> -		if (!l_dhcp_server_set_ip_address(ap->server, addr)) {
> -			ret = -EINVAL;
> -			goto free_addr;
> +		if (addr_prefix > prefix) {
> +			l_debug("Address spec %s smaller than requested "
> +				"subnet (prefix len %i)", addr_str_list[i],
> +				prefix);
> +			continue;
>   		}
>   
> -		ap->own_ip = l_strdup(addr);
> +		range.start = addr & subnet_mask;
> +		range.end = range.start + (1 << (32 - addr_prefix));
> +		ap_append_range(ranges, &range, used, addr_str_list[i]);
> +	}
> +
> +select_addr:
> +	if (l_queue_isempty(ranges)) {
> +		l_error("No IP subnets available for new Access Point after "
> +			"eliminating those already in use on the system");
> +		err = -EEXIST;
> +		goto done;
> +	}
>   
> -free_addr:
> -		l_free(addr);
> +	/* Count available @prefix-sized subnets */
> +	for (entry = l_queue_get_entries(ranges); entry; entry = entry->next) {
> +		struct ap_ip_range *range = entry->data;
>   
> -		return ret;
> -	} else if (address) {
> -		/* No config file and address is already set */
> -		ap->own_ip = l_strdup(inet_ntoa(ia));
> +		total += (range->end - range->start) >> (32 - prefix);
> +	}
>   
> -		return -EALREADY;
> -	} else if (pool.used) {
> -		/* No config file, no address set. Use IP pool */
> -		ap->own_ip = ip_pool_get();
> -		if (!ap->own_ip) {
> -			l_error("No more IP's in pool, cannot start AP on %u",
> -					ifindex);
> -			return -EEXIST;
> -		}
> +	selected = l_getrandom_uint32() % total;
>   
> -		ap->use_ip_pool = true;
> -		ap->ip_prefix = pool.prefix;
> +	/* Find the @selected'th @prefix-sized subnet */
> +	for (entry = l_queue_get_entries(ranges);; entry = entry->next) {
> +		struct ap_ip_range *range = entry->data;
> +		uint32_t count = (range->end - range->start) >> (32 - prefix);
>   
> -		return 0;
> +		if (selected < count) {
> +			addr = range->start + (selected << (32 - prefix));
> +			break;
> +		}
> +
> +		selected -= count;
>   	}
>   
> -	return -EINVAL;
> +	if (!host_addr)
> +		host_addr = addr + 1;
> +
> +	err = 0;
> +	*out_addr = host_addr;
> +
> +done:
> +	l_queue_destroy(ranges, l_free);
> +	l_queue_destroy(used, l_free);
> +	return err;
>   }
>   
> -static int ap_load_profile_and_dhcp(struct ap_state *ap, bool *wait_dhcp)
> +#define AP_DEFAULT_IPV4_PREFIX_LEN 28
> +
> +/*
> + * This will determine the IP to be used for DHCP.  ap->server,
> + * ap->own_ip and ap->ip_prefix will be set.
> + *
> + * The address to set (or keep) is determined in this order:
> + * 1. Select from address pool defined in the provisioning file
> + * 2. Keep address already set on the interface
> + * 3. Select from the global address pool (default 192.168.0.0/16)
> + *
> + * Returns:  0 if an IP was successfully selected and needs to be set,
> + *          -EALREADY if an IP is already set on the interface (success),
> + *          -EEXIST if all available subnet addresses are in use (error),
> + *          -EINVAL if there was a different error.
> + */
> +static int ap_setup_dhcp(struct ap_state *ap, uint32_t existing_address,
> +				uint8_t existing_prefix)
>   {
>   	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
> -	char *passphrase;
> -	struct l_settings *settings = NULL;
> -	int err = -EINVAL;
> +	struct in_addr if_addr;
> +	struct in_addr if_netmask;
> +	uint32_t address;
> +	uint8_t prefix;
> +	int ret = -EINVAL;
> +	char **addr_str_list;
> +	L_AUTO_FREE_VAR(char *, netmask_str) =
> +		l_settings_get_string(ap->profile, "IPv4", "Netmask");

Elsewhere you check that ap->profile is not NULL, but not here.  Seems inconsistent.

Also, I'm a bit lost why ap->profile might be NULL.  Shouldn't we simply create 
an empty profile instead to simplify the logic?

>   
> -	/* No profile or DHCP settings */
> -	if (!ap->config->profile && !pool.used)
> -		return 0;
> +	ap->server = l_dhcp_server_new(ifindex);

Ah l_dhcp_server_new can't fail...

> +	if (!ap->server) {
> +		l_error("Failed to create DHCP server on %u", ifindex);
> +		return -EINVAL;
> +	}
>   
> -	if (ap->config->profile) {
> -		settings = l_settings_new();
> +	if (getenv("IWD_DHCP_DEBUG"))
> +		l_dhcp_server_set_debug(ap->server, do_debug,
> +							"[DHCPv4 SERV] ", NULL);
>   
> -		if (!l_settings_load_from_file(settings, ap->config->profile))
> -			goto cleanup;
> +	if (ap->profile &&
> +			l_settings_get_value(ap->profile, "IPv4", "Address")) {
> +		addr_str_list = l_settings_get_string_list(ap->profile, "IPv4",
> +								"Address", ',');
> +		if (!addr_str_list || !*addr_str_list) {
> +			l_error("Can't parse the profile [IPv4].Address "
> +				"setting as a string list");
> +			goto done;
> +		}
> +	} else if (existing_address) {
> +		addr_str_list = NULL;
> +	} else
> +		addr_str_list = global_addr_strs;
>   
> -		passphrase = l_settings_get_string(settings, "Security",
> -							"Passphrase");
> -		if (passphrase) {
> -			if (strlen(passphrase) > 63) {
> -				l_error("[Security].Passphrase must not exceed "
> -						"63 characters");
> -				l_free(passphrase);
> -				goto cleanup;
> +	if (addr_str_list) {
> +		int r;
> +
> +		if (netmask_str) {
> +			uint32_t netmask;
> +
> +			if (inet_pton(AF_INET, netmask_str, &if_netmask) < 0) {
> +				l_error("Can't parse the profile "
> +					"[IPv4].Netmask setting");
> +				goto done;
>   			}
>   
> -			strcpy(ap->config->passphrase, passphrase);
> -			l_free(passphrase);
> -		}
> +			netmask = if_netmask.s_addr;
> +			prefix = __builtin_popcountl(netmask);
> +			if ((ntohl(netmask) !=
> +					util_netmask_from_prefix(prefix))) {
> +				l_error("Invalid netmask %s", netmask_str);
> +				goto done;
> +			}
>   
> -		if (!l_settings_has_group(settings, "IPv4")) {
> -			*wait_dhcp = false;
> -			err = 0;
> -			goto cleanup;
> +			if (prefix > 30 || prefix < 8) {
> +				l_error("Netmasks between 8 and 30 bits "
> +					"are allowed");
> +				goto done;
> +			}
> +		} else
> +			prefix = AP_DEFAULT_IPV4_PREFIX_LEN;
> +
> +		r = ap_select_addr((const char **) addr_str_list, prefix,
> +					netdev_get_name(ap->netdev), &address);
> +		if (r < 0) {
> +			ret = r;
> +			goto done;
>   		}
> +	} else {
> +		address = existing_address;
> +		prefix = existing_prefix;
> +	}
> +
> +	if_addr.s_addr = htonl(address);
> +	if (!l_dhcp_server_set_ip_address(ap->server, inet_ntoa(if_addr))) {
> +		l_error("l_dhcp_server_set_ip_address failed");
> +		goto done;
> +	}
> +
> +	if_netmask.s_addr = htonl(util_netmask_from_prefix(prefix));
> +	if (!l_dhcp_server_set_netmask(ap->server, inet_ntoa(if_netmask))) {
> +		l_error("l_dhcp_server_set_netmask failed");
> +		goto done;
>   	}
>   
> -	err = ap_setup_dhcp(ap, settings);
> +	/* Set the remaining DHCP options in config file */
> +	if (ap->profile && !ap_load_dhcp_settings(ap, address, prefix))
> +		goto done;
> +
> +	ap->own_ip = l_strdup(inet_ntoa(if_addr));
> +	ap->ip_prefix = prefix;
> +	ret = (address == existing_address && prefix == existing_prefix) ?
> +		-EALREADY : 0;
> +
> +done:
> +	if (addr_str_list != global_addr_strs)
> +		l_strv_free(addr_str_list);
> +
> +	return ret;
> +}
> +
> +static void ap_netconfig_start(struct ap_state *ap, uint32_t existing_addr,
> +				uint8_t existing_prefix)
> +{
> +	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
> +	int err;
> +
> +	err = ap_setup_dhcp(ap, existing_addr, existing_prefix);
>   	if (err == 0) {
>   		/* Address change required */
>   		ap->rtnl_add_cmd = l_rtnl_ifaddr4_add(rtnl, ifindex,
>   					ap->ip_prefix, ap->own_ip,
>   					broadcast_from_ip(ap->own_ip),
>   					ap_ifaddr4_added_cb, ap, NULL);
> -
> -		if (!ap->rtnl_add_cmd) {
> -			l_error("Failed to add IPv4 address");
> -			err = -EIO;
> -			goto cleanup;
> +		if (ap->rtnl_add_cmd) {
> +			ap->cleanup_ip = true;
> +			return;
>   		}

So we decided not to remove any existing addresses?

>   
> -		ap->cleanup_ip = true;
> -
> -		*wait_dhcp = true;
> -		err = 0;
> +		l_error("Failed to add IPv4 address");
>   	/* Selected address already set, continue normally */
>   	} else if (err == -EALREADY) {
> -		*wait_dhcp = false;
> -		err = 0;
> +		if (ap_start_send(ap))
> +			return;
>   	}
>   
> -cleanup:
> -	l_settings_free(settings);
> -	return err;
> +	ap_start_failed(ap);
> +}
> +
> +static void ap_ifaddr4_dump_cb(int error,
> +				uint16_t type, const void *data,
> +				uint32_t len, void *user_data)
> +{
> +	struct ap_state *ap = user_data;
> +	const struct ifaddrmsg *ifa = data;
> +	char *ip_str = NULL;
> +	struct in_addr ip = {};
> +	uint8_t prefix = 0;
> +	uint32_t cmd;
> +
> +	if (!error && ifa->ifa_index != netdev_get_ifindex(ap->netdev))
> +		return;
> +
> +	if (error || type != RTM_NEWADDR ||
> +			ifa->ifa_prefixlen > 30 || ifa->ifa_prefixlen < 1) {
> +		l_error("Error getting existing IPv4 address on AP iface");
> +		goto done;
> +	}
> +
> +	l_rtnl_ifaddr4_extract(ifa, len, NULL, &ip_str, NULL);

So FYI, this API is slated for the chopping block now that we have 
l_rtnl_address object.  I just haven't gotten around to it yet.

> +	inet_aton(ip_str, &ip);
> +	l_free(ip_str);
> +	prefix = ifa->ifa_prefixlen;
> +
> +done:
> +	cmd = ap->rtnl_dump_cmd;
> +	ap->rtnl_dump_cmd = 0;
> +	l_netlink_cancel(rtnl, cmd);
> +
> +	ap_netconfig_start(ap, ntohl(ip.s_addr), prefix);
> +}
> +
> +static void ap_ifaddr4_dump_destroy_cb(void *user_data)
> +{
> +	struct ap_state *ap = user_data;
> +
> +	if (!ap->rtnl_dump_cmd)
> +		return;
> +
> +	ap->rtnl_dump_cmd = 0;
> +	l_debug("No existing IPv4 addresses on AP iface");
> +	ap_netconfig_start(ap, 0, 0);
>   }
>   
>   /*
> @@ -2639,13 +2815,10 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
>   {
>   	struct ap_state *ap;
>   	struct wiphy *wiphy = netdev_get_wiphy(netdev);
> -	struct l_genl_msg *cmd;
>   	uint64_t wdev_id = netdev_get_wdev_id(netdev);
> -	int err = -EINVAL;
> -	bool wait_on_address = false;
>   
>   	if (err_out)
> -		*err_out = err;
> +		*err_out = -EINVAL;
>   
>   	if (L_WARN_ON(!config->ssid))
>   		return NULL;
> @@ -2661,15 +2834,33 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
>   	ap->ops = ops;
>   	ap->user_data = user_data;
>   
> -	/*
> -	 * This both loads a profile if required and loads DHCP settings either
> -	 * by the profile itself or the IP pool (or does nothing in the case
> -	 * of a profile-less configuration). wait_on_address will be set true
> -	 * if an address change is required.
> -	 */
> -	err = ap_load_profile_and_dhcp(ap, &wait_on_address);
> -	if (err < 0)
> -		goto error;
> +	if (ap->config->profile) {
> +		char *passphrase;
> +
> +		ap->profile = l_settings_new();
> +
> +		if (!l_settings_load_from_file(ap->profile, ap->config->profile))
> +			goto error;
> +
> +		passphrase = l_settings_get_string(ap->profile, "Security",
> +							"Passphrase");
> +		if (!passphrase) {
> +			l_error("[Security].Passphrase not found in "
> +				"AP profile");
> +			goto error;
> +		}
> +
> +		/* (full checks done later in crypto_passphrase_is_valid) */
> +		if (strlen(passphrase) > 63) {
> +			l_error("[Security].Passphrase must not exceed "
> +				"63 characters");
> +			l_free(passphrase);
> +			goto error;
> +		}
> +
> +		strcpy(ap->config->passphrase, passphrase);
> +		l_free(passphrase);
> +	}
>   
>   	if (!config->channel)
>   		/* TODO: Start a Get Survey to decide the channel */
> @@ -2752,33 +2943,31 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
>   	if (!ap->mlme_watch)
>   		l_error("Registering for MLME notification failed");
>   
> -	if (wait_on_address) {
> -		if (err_out)
> -			*err_out = 0;
> +	if (netconfig_enabled &&
> +			(!ap->profile ||
> +			 l_settings_has_group(ap->profile, "IPv4"))) {

It seems to me the logic should go something like this:

1. Load the profile
2. If the profile contains a single address / subnet, go to step 6
3. Issue ifaddr4_dump and record any used ranges
4. If an address is set on the interface and IPv4.Address isn't set, pick one 
(non secondary address), doesn't matter which one and use it as the DHCP 
address.  Goto step 7.
5. Pick an address from the pool taking into account used ranges found in step 3.
6. Set the address picked in step 2, or step 5
7. Start DHCP & AP


> +		ap->rtnl_dump_cmd = l_rtnl_ifaddr4_dump(rtnl,
> +						ap_ifaddr4_dump_cb, ap,
> +						ap_ifaddr4_dump_destroy_cb);
> +		if (!ap->rtnl_dump_cmd) {
> +			if (err_out)
> +				*err_out = -EIO;
> +
> +			l_error("Sending the IPv4 addr dump req failed");
> +			goto error;
> +		}
>   
>   		return ap;
>   	}
>   
> -	cmd = ap_build_cmd_start_ap(ap);
> -	if (!cmd)
> -		goto error;
> +	if (ap_start_send(ap)) {
> +		if (err_out)
> +			*err_out = 0;
>   
> -	ap->start_stop_cmd_id = l_genl_family_send(ap->nl80211, cmd,
> -							ap_start_cb, ap, NULL);
> -	if (!ap->start_stop_cmd_id) {
> -		l_genl_msg_unref(cmd);
> -		goto error;
> +		return ap;
>   	}
>   
> -	if (err_out)
> -		*err_out = 0;
> -
> -	return ap;
> -
>   error:
> -	if (err_out)
> -		*err_out = err;
> -
>   	ap->config = NULL;
>   	ap_reset(ap);
>   	l_genl_family_free(ap->nl80211);
> @@ -3316,7 +3505,6 @@ static void ap_netdev_watch(struct netdev *netdev,
>   static int ap_init(void)
>   {
>   	const struct l_settings *settings = iwd_get_config();
> -	bool dhcp_enable;
>   
>   	netdev_watch = netdev_watch_add(ap_netdev_watch, NULL, NULL);
>   
> @@ -3327,31 +3515,44 @@ static int ap_init(void)
>   			ap_diagnostic_interface_destroy, false);
>   
>   	/*
> -	 * Reusing [General].EnableNetworkConfiguration as a switch to enable
> -	 * DHCP server. If no value is found or it is false do not create a
> -	 * DHCP server.
> +	 * Enable network configuration and DHCP only if
> +	 * [General].EnableNetworkConfiguration is true.
>   	 */
>   	if (!l_settings_get_bool(settings, "General",
> -				"EnableNetworkConfiguration", &dhcp_enable))
> -		dhcp_enable = false;
> -
> -	if (dhcp_enable) {
> -		L_AUTO_FREE_VAR(char *, ip_prefix);
> -
> -		ip_prefix = l_settings_get_string(settings, "General",
> -							"APRanges");
> -		/*
> -		 * In this case its assumed the user only cares about station
> -		 * netconfig so we let ap_init pass but DHCP will not be
> -		 * enabled.
> -		 */
> -		if (!ip_prefix) {
> -			l_warn("[General].APRanges must be set for DHCP");
> -			return 0;
> +					"EnableNetworkConfiguration",
> +					&netconfig_enabled))
> +		netconfig_enabled = false;
> +
> +	if (netconfig_enabled) {
> +		if (l_settings_get_value(settings, "IPv4", "APAddressPool")) {
> +			global_addr_strs = l_settings_get_string_list(settings,
> +								"IPv4",
> +								"APAddressPool",
> +								',');
> +			if (!global_addr_strs || !*global_addr_strs) {
> +				l_error("Can't parse the [IPv4].APAddressPool "
> +					"setting as a string list");
> +				l_strv_free(global_addr_strs);
> +				global_addr_strs = NULL;
> +			}
> +		} else if (l_settings_get_value(settings,
> +						"General", "APRanges")) {
> +			global_addr_strs = l_settings_get_string_list(settings,
> +								"General",
> +								"APRanges",
> +								',');
> +			if (!global_addr_strs || !*global_addr_strs) {
> +				l_error("Can't parse the [General].APRanges "
> +					"setting as a string list");
> +				l_strv_free(global_addr_strs);
> +				global_addr_strs = NULL;
> +			}
>   		}
>   
> -		if (!ip_pool_create(ip_prefix))
> -			return -EINVAL;
> +		/* Fall back to 192.168.0.0/16 */
> +		if (!global_addr_strs)
> +			global_addr_strs =
> +				l_strv_append(NULL, "192.168.0.0/16");
>   	}
>   
>   	rtnl = iwd_get_rtnl();
> @@ -3363,8 +3564,7 @@ static void ap_exit(void)
>   {
>   	netdev_watch_remove(netdev_watch);
>   	l_dbus_unregister_interface(dbus_get_bus(), IWD_AP_INTERFACE);
> -
> -	ip_pool_destroy();
> +	l_strv_free(global_addr_strs);
>   }
>   
>   IWD_MODULE(ap, ap_init, ap_exit)
> 

Regards,
-Denis

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

* Re: [PATCH 1/5] ap: Refactor IP address selection
  2021-03-03 17:14 ` [PATCH 1/5] ap: Refactor IP address selection Denis Kenzior
@ 2021-03-03 19:17   ` Andrew Zaborowski
  2021-03-03 20:30     ` Denis Kenzior
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Zaborowski @ 2021-03-03 19:17 UTC (permalink / raw)
  To: iwd

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

On Wed, 3 Mar 2021 at 18:14, Denis Kenzior <denkenz@gmail.com> wrote:
> On 3/2/21 6:08 AM, Andrew Zaborowski wrote:
> > Refactor how the subnet IP address is selected and change related
> > settings syntax in the following way:
> >
> > * [IPv4].Address in an AP profile can now be given as just an IP address
> >    or a list of prefix notation address spaces.  In the latter case one
> >    subnet address of the size implied by [IPv4].Netmask is selected
> >    randomly from that space, defaults to a 28-bit netmask.  In the former
> >    case [IPv4].Address is now also checked for conflicts with addresses
> >    already in use.
> >
> > * main.conf's [IPv4].APAddressPool is now the fallback setting that uses
> >    the same syntax as profile-local [IPv4].Address and has a default
> >    value of 192.168.0.0/16.
> >
> > * main.conf's [General].APRanges is deprecated in favour of
> >    [IPv4].APAddressPool but this change should be backwards compatible.
> >
> > ap_setup_dhcp now always prints error messages, it seems it was assumed
> > that the caller would do this before but it didn't.  It also now always
> > reads [IPv4].Netmask or the value already set on the interface (when
> > existing IP is used) and uses that value.  Previously the configured
> > netmask was only being announced through DHCP but not used locally.
> > Require that the WPA-PSK passphrase is present in the AP profile,
> > previously it was treated as optional but the rest of the code was
> > expecting it to always be present.
>
> So I really wish this patch was broken up more.  Reviewing 800 line patchsets is
> already a pain.  And you have a bunch of different things going on here.

Right, partly it's because I'm replacing the existing code and I can't
change just one part of it but I'll try to perhaps drop it in one
patch and re-add in steps.

>
> >
> > Drop the l_net_get_address() usage, replace with the more complicated
> > l_rtnl_* calls.  This is to avoid also adding an l_net_get_netmask() to
> > ell since we'd need that and we reportely don't want to keep using the
> > netdev ioctls.
>
> This is really metadata / change log.  Does not belong in the description.

Ok, although personally when I'm looking through a commit log a lot of
the time my first question is *why* something is being changed.

>
> > ---
> >   src/ap.c | 804 ++++++++++++++++++++++++++++++++++---------------------
> >   1 file changed, 502 insertions(+), 302 deletions(-)
> >
> > diff --git a/src/ap.c b/src/ap.c
> > index 2042cde6..d871f973 100644
> > --- a/src/ap.c
> > +++ b/src/ap.c
> > @@ -28,6 +28,7 @@
> >   #include <linux/if_ether.h>
> >   #include <netinet/in.h>
> >   #include <arpa/inet.h>
> > +#include <ifaddrs.h>
> >
> >   #include <ell/ell.h>
> >
> > @@ -78,15 +79,16 @@ struct ap_state {
> >       uint16_t last_aid;
> >       struct l_queue *sta_states;
> >
> > +     struct l_settings *profile;
> >       struct l_dhcp_server *server;
> > +     uint32_t rtnl_dump_cmd;
> >       uint32_t rtnl_add_cmd;
> >       char *own_ip;
> > -     unsigned int ip_prefix;
> > +     uint8_t ip_prefix;
> >
> >       bool started : 1;
> >       bool gtk_set : 1;
> >       bool cleanup_ip : 1;
> > -     bool use_ip_pool : 1;
> >   };
> >
> >   struct sta_state {
> > @@ -115,99 +117,11 @@ struct ap_wsc_pbc_probe_record {
> >       uint64_t timestamp;
> >   };
> >
> > -struct ap_ip_pool {
> > -     uint32_t start;
> > -     uint32_t end;
> > -     uint8_t prefix;
> > -
> > -     /* Fist/last valid subnet */
> > -     uint8_t sub_start;
> > -     uint8_t sub_end;
> > -
> > -     struct l_uintset *used;
> > -};
> > -
> > -struct ap_ip_pool pool;
> > +static bool netconfig_enabled;
> > +static char **global_addr_strs;
> >   static uint32_t netdev_watch;
> >   struct l_netlink *rtnl;
> >
> > -/*
> > - * Creates pool of IPs which AP intefaces can use. Each call to ip_pool_get
> > - * will advance the subnet +1 so there are no IP conflicts between AP
> > - * interfaces
> > - */
> > -static bool ip_pool_create(const char *ip_prefix)
> > -{
> > -     if (!util_ip_prefix_tohl(ip_prefix, &pool.prefix, &pool.start,
> > -                                     &pool.end, NULL))
> > -             return false;
> > -
> > -     if (pool.prefix > 24) {
> > -             l_error("APRanges prefix must 24 or less (%u used)",
> > -                             pool.prefix);
> > -             memset(&pool, 0, sizeof(pool));
> > -             return false;
> > -     }
> > -
> > -     /*
> > -      * Find the number of subnets we can use, this will dictate the number
> > -      * of AP interfaces that can be created (when using DHCP)
> > -      */
> > -     pool.sub_start = (pool.start & 0x0000ff00) >> 8;
> > -     pool.sub_end = (pool.end & 0x0000ff00) >> 8;
> > -
> > -     pool.used = l_uintset_new_from_range(pool.sub_start, pool.sub_end);
> > -
> > -     return true;
> > -}
> > -
> > -static char *ip_pool_get()
> > -{
> > -     uint32_t ip;
> > -     struct in_addr ia;
> > -     uint8_t next_subnet = (uint8_t)l_uintset_find_unused_min(pool.used);
> > -
> > -     /* This shouldn't happen */
> > -     if (next_subnet < pool.sub_start || next_subnet > pool.sub_end)
> > -             return NULL;
> > -
> > -     l_uintset_put(pool.used, next_subnet);
> > -
> > -     ip = pool.start;
> > -     ip &= 0xffff00ff;
> > -     ip |= (next_subnet << 8);
> > -
> > -     ia.s_addr = htonl(ip);
> > -     return l_strdup(inet_ntoa(ia));
> > -}
> > -
> > -static bool ip_pool_put(const char *address)
> > -{
> > -     struct in_addr ia;
> > -     uint32_t ip;
> > -     uint8_t subnet;
> > -
> > -     if (inet_aton(address, &ia) < 0)
> > -             return false;
> > -
> > -     ip = ntohl(ia.s_addr);
> > -
> > -     subnet = (ip & 0x0000ff00) >> 8;
> > -
> > -     if (subnet < pool.sub_start || subnet > pool.sub_end)
> > -             return false;
> > -
> > -     return l_uintset_take(pool.used, subnet);
> > -}
> > -
> > -static void ip_pool_destroy()
> > -{
> > -     if (pool.used)
> > -             l_uintset_free(pool.used);
> > -
> > -     memset(&pool, 0, sizeof(pool));
> > -}
> > -
> >   static const char *broadcast_from_ip(const char *ip)
> >   {
> >       struct in_addr ia;
> > @@ -304,6 +218,13 @@ static void ap_reset(struct ap_state *ap)
> >       if (ap->start_stop_cmd_id)
> >               l_genl_family_cancel(ap->nl80211, ap->start_stop_cmd_id);
> >
> > +     if (ap->rtnl_dump_cmd) {
> > +             uint32_t cmd = ap->rtnl_dump_cmd;
> > +
> > +             ap->rtnl_dump_cmd = 0;
> > +             l_netlink_cancel(rtnl, cmd);
>
> Why is this so complicated?  Isn't rtnl_dump_cmd reset in the destructor?

I abused rtnl_dump_cmd = 0 here to signal that there's no need to call
ap_netconfig_start.

>
> > +     }
> > +
> >       if (ap->rtnl_add_cmd)
> >               l_netlink_cancel(rtnl, ap->rtnl_add_cmd);
> >
> > @@ -326,16 +247,16 @@ static void ap_reset(struct ap_state *ap)
> >                                       broadcast_from_ip(ap->own_ip),
> >                                       NULL, NULL, NULL);
> >
> > -     if (ap->own_ip) {
> > -             /* Release IP from pool if used */
> > -             if (ap->use_ip_pool)
> > -                     ip_pool_put(ap->own_ip);
> > -
> > +     if (ap->own_ip)
> >               l_free(ap->own_ip);
> > -     }
> >
> >       if (ap->server)
> >               l_dhcp_server_stop(ap->server);
> > +
> > +     if (ap->profile) {
> > +             l_settings_free(ap->profile);
> > +             ap->profile = NULL;
> > +     }
> >   }
> >
> >   static void ap_del_station(struct sta_state *sta, uint16_t reason,
> > @@ -2172,34 +2093,41 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
> >       return cmd;
> >   }
> >
> > -static void ap_ifaddr4_added_cb(int error, uint16_t type, const void *data,
> > -                             uint32_t len, void *user_data)
> > +static bool ap_start_send(struct ap_state *ap)
> >   {
> > -     struct ap_state *ap = user_data;
> > -     struct l_genl_msg *cmd;
> > -
> > -     ap->rtnl_add_cmd = 0;
> > +     struct l_genl_msg *cmd = ap_build_cmd_start_ap(ap);
> >
> > -     if (error) {
> > -             l_error("Failed to set IP address");
> > -             goto error;
> > +     if (!cmd) {
> > +             l_error("ap_build_cmd_start_ap failed");
> > +             return false;
> >       }
> >
> > -     cmd = ap_build_cmd_start_ap(ap);
> > -     if (!cmd)
> > -             goto error;
> > -
> >       ap->start_stop_cmd_id = l_genl_family_send(ap->nl80211, cmd,
> >                                                       ap_start_cb, ap, NULL);
> >       if (!ap->start_stop_cmd_id) {
> > +             l_error("AP_START l_genl_family_send failed");
> >               l_genl_msg_unref(cmd);
> > -             goto error;
> > +             return false;
> >       }
> >
> > -     return;
> > +     return true;
> > +}
> >
> > -error:
> > -     ap_start_failed(ap);
> > +static void ap_ifaddr4_added_cb(int error, uint16_t type, const void *data,
> > +                             uint32_t len, void *user_data)
> > +{
> > +     struct ap_state *ap = user_data;
> > +
> > +     ap->rtnl_add_cmd = 0;
> > +
> > +     if (error) {
> > +             l_error("Failed to set IP address");
> > +             ap_start_failed(ap);
> > +             return;
> > +     }
> > +
> > +     if (!ap_start_send(ap))
> > +             ap_start_failed(ap);
> >   }
> >
> >   static bool ap_parse_new_station_ies(const void *data, uint16_t len,
> > @@ -2394,23 +2322,21 @@ static void ap_mlme_notify(struct l_genl_msg *msg, void *user_data)
> >       }
> >   }
> >
> > -static bool dhcp_load_settings(struct ap_state *ap, struct l_settings *settings)
> > +static bool ap_load_dhcp_settings(struct ap_state *ap,
> > +                                     uint32_t own_ip, uint8_t prefix)
>
> One thing I don't like about this is that the primary IP selection is done
> elsewhere, but then Netmask/IPRange settings are parsed after the fact.

Actually I'm not a fan of that either but that's how the existing (and
reviewed?) code does it. In this patch I'm moving the Netconfig
handling together with the subnet selection but I didn't touch the
IPRange parsing, the change is already big as you noted.

>  I'm not
> sure if it makes sense for a profile to have Netmask/IPRange settings if it also
> doesn't have the Address set to a single subnet.

Agreed about IPRange, but for Netmask I see it as a way to specify the
prefix length basically, i.e. how much address space the AP wants.

>  l_dhcp_server probably only
> works with prefixes >= 24 when allocating addresses.

I haven't looked at it but even then you may want to specify your own
value between 24 - 28 or 29... 28 being the new default.

(Obviously all of these are very specific settings... I assume there's
some reason why we have them)

>  Shouldn't we load the
> profile and verify it mostly in one place?

I haven't really checked but my assumption was that the existing code
skips some of the verification because the l_dhcp_* methods would
verify the values passed to it and we avoid duplicating that work.

>
> Things like DNSList and LeaseTime are probably OK done separately, since they're
> not dependent on the IP selection.
>
> >   {
> >       struct l_dhcp_server *server = ap->server;
> > -     struct in_addr ia;
> >
> > -     L_AUTO_FREE_VAR(char *, netmask) = l_settings_get_string(settings,
> > -                                                     "IPv4", "Netmask");
> > -     L_AUTO_FREE_VAR(char *, gateway) = l_settings_get_string(settings,
> > +     L_AUTO_FREE_VAR(char *, gateway) = l_settings_get_string(ap->profile,
> >                                                       "IPv4", "Gateway");
> > -     char **dns = l_settings_get_string_list(settings, "IPv4",
> > +     char **dns = l_settings_get_string_list(ap->profile, "IPv4",
> >                                                       "DNSList", ',');
> > -     char **ip_range = l_settings_get_string_list(settings, "IPv4",
> > +     char **ip_range = l_settings_get_string_list(ap->profile, "IPv4",
> >                                                       "IPRange", ',');
> >       unsigned int lease_time;
> >       bool ret = false;
> >
> > -     if (!l_settings_get_uint(settings, "IPv4", "LeaseTime", &lease_time))
> > +     if (!l_settings_get_uint(ap->profile, "IPv4", "LeaseTime", &lease_time))
> >               lease_time = 0;
> >
> >       if (gateway && !l_dhcp_server_set_gateway(server, gateway)) {
> > @@ -2423,17 +2349,34 @@ static bool dhcp_load_settings(struct ap_state *ap, struct l_settings *settings)
> >               goto error;
> >       }
> >
> > -     if (netmask && !l_dhcp_server_set_netmask(server, netmask)) {
> > -             l_error("[IPv4].Netmask value error");
> > -             goto error;
> > -     }
> > -
> >       if (ip_range) {
> > +             int i;
> > +
> >               if (l_strv_length(ip_range) != 2) {
> >                       l_error("Two addresses expected in [IPv4].IPRange");
> >                       goto error;
> >               }
> >
> > +             for (i = 0; i < 2; i++) {
> > +                     struct in_addr range_addr;
> > +
> > +                     if (inet_aton(ip_range[i], &range_addr) != 1) {
> > +                             l_error("Can't parse address in "
> > +                                     "[IPv4].IPRange[%i]", i + 1);
> > +                             goto error;
> > +                     }
> > +
> > +                     if ((own_ip ^ ntohl(range_addr.s_addr)) &
> > +                                     util_netmask_from_prefix(prefix)) {
> > +                             struct in_addr addr = { htonl(own_ip) };
> > +
> > +                             l_error("[IPv4].IPRange[%i] is not in the "
> > +                                     "%s/%i subnet", i + 1,
> > +                                     inet_ntoa(addr), prefix);
> > +                             goto error;
> > +                     }
> > +             }
> > +
> >               if (!l_dhcp_server_set_ip_range(server, ip_range[0],
> >                                                       ip_range[1])) {
> >                       l_error("Error setting IP range from [IPv4].IPRange");
> > @@ -2446,11 +2389,6 @@ static bool dhcp_load_settings(struct ap_state *ap, struct l_settings *settings)
> >               goto error;
> >       }
> >
> > -     if (netmask && inet_pton(AF_INET, netmask, &ia) > 0)
> > -             ap->ip_prefix = __builtin_popcountl(ia.s_addr);
> > -     else
> > -             ap->ip_prefix = 24;
> > -
> >       ret = true;
> >
> >   error:
> > @@ -2459,166 +2397,404 @@ error:
> >       return ret;
> >   }
> >
> > +struct ap_ip_range {
> > +     uint32_t start;
> > +     uint32_t end;
> > +};
> > +
> > +static int ap_ip_range_compare(const void *a, const void *b, void *user_data)
> > +{
> > +     const struct ap_ip_range *range_a = a;
> > +     const struct ap_ip_range *range_b = b;
> > +
> > +     return range_a->start > range_b->start;
> > +}
> > +
> >   /*
> > - * This will determine the IP being used for DHCP. The IP will be automatically
> > - * set to ap->own_ip.
> > - *
> > - * The address to set (or keep) is determined in this order:
> > - * 1. Address defined in provisioning file
> > - * 2. Address already set on interface
> > - * 3. Address in IP pool.
> > - *
> > - * Returns:  0 if an IP was successfully selected and needs to be set
> > - *          -EALREADY if an IP was already set on the interface
> > - *          -EEXIST if the IP pool ran out of IP's
> > - *          -EINVAL if there was an error.
> > + * Append any address ranges within an input start/end range, which contain
> > + * at least one full subnet and don't intersect with any subnets already in
> > + * use.  This may result in the input range being split into multiple ranges
> > + * of different sizes or being skipped altogether.
> > + * All inputs must be rounded to the subnet boundary and the @used queue
> > + * sorted by the subnet start address.
> >    */
> > -static int ap_setup_dhcp(struct ap_state *ap, struct l_settings *settings)
> > +static void ap_append_range(struct l_queue *to, const struct ap_ip_range *range,
> > +                             struct l_queue *used, const char *str)
> > +{
> > +     const struct l_queue_entry *entry = l_queue_get_entries(used);
> > +     const struct ap_ip_range *used_range = entry ? entry->data : NULL;
> > +     uint32_t start = range->start;
> > +     bool print = true;
> > +
> > +     while (range->end > start) {
> > +             while (used_range && used_range->end <= start) {
> > +                     entry = entry->next;
> > +                     used_range = entry ? entry->data : NULL;
> > +             }
> > +
> > +             /* No more used ranges that intersect with @start/@range->end */
> > +             if (!used_range || range->end <= used_range->start) {
> > +                     struct ap_ip_range *sub = l_new(struct ap_ip_range, 1);
> > +
> > +                     sub->start = start;
> > +                     sub->end = range->end;
> > +                     l_queue_push_tail(to, sub);
> > +                     l_queue_insert(used, l_memdup(sub, sizeof(*sub)),
> > +                                     ap_ip_range_compare, NULL);
> > +                     return;
> > +             }
> > +
> > +             if (print) {
> > +                     l_debug("Address spec %s intersects with at least one "
> > +                             "subnet already in use on the system or "
> > +                             "specified in the settings", str);
> > +                     print = false;
> > +             }
> > +
> > +             /* Now we know @used_range is non-NULL and intersects */
> > +             if (start < used_range->start) {
> > +                     struct ap_ip_range *sub = l_new(struct ap_ip_range, 1);
> > +
> > +                     sub->start = start;
> > +                     sub->end = used_range->start;
> > +                     l_queue_push_tail(to, sub);
> > +                     l_queue_insert(used, l_memdup(sub, sizeof(*sub)),
> > +                                     ap_ip_range_compare, NULL);
> > +             }
> > +
> > +             /* Skip to the start of the next subnet */
> > +             start = used_range->end;
> > +     }
> > +}
>
> Can we split out the address allocation logic out into a separate file so that
> we can more easily add unit tests for it?

Ok.  Note that the autotests James added already test the logic pretty well.

>
> > +
> > +/* Select a subnet and a host address from a defined space */
> > +static int ap_select_addr(const char **addr_str_list, uint8_t prefix,
> > +                             const char *ifname, uint32_t *out_addr)
> >   {
> > -     uint32_t ifindex = netdev_get_ifindex(ap->netdev);
> > +     uint32_t total = 0;
> > +     uint32_t selected;
> > +     unsigned int i;
> > +     uint32_t addr;
> > +     struct ifaddrs *ifaddr;
> > +     const struct ifaddrs *ifa;
> > +     uint32_t subnet_size = 1 << (32 - prefix);
> > +     uint32_t host_mask = subnet_size - 1;
> > +     uint32_t subnet_mask = ~host_mask;
> > +     uint32_t host_addr = 0;
> > +     struct l_queue *ranges = l_queue_new();
> > +     struct l_queue *used = l_queue_new();
> >       struct in_addr ia;
> > -     uint32_t address = 0;
> > -     int ret = -EINVAL;
> > +     const struct l_queue_entry *entry;
> > +     int err = -EINVAL;
> >
> > -     ap->server = l_dhcp_server_new(ifindex);
> > -     if (!ap->server) {
> > -             l_error("Failed to create DHCP server on %u", ifindex);
> > -             return -EINVAL;;
> > +     /* Build a sorted list of used/unavailable subnets */
> > +     if (getifaddrs(&ifaddr) != 0)
> > +             ifaddr = NULL;
>
> So you already made an RTNL dump elsewhere, and are now effectively doing it
> again.  Can we not combine the two somehow?

True, I should probably combine them.

>
> > +
> > +     for (ifa = ifaddr; ifa; ifa = ifa->ifa_next) {
> > +             const struct sockaddr_in *addr_sa;
> > +             const struct sockaddr_in *netmask_sa;
> > +             uint32_t used_addr;
> > +             uint32_t used_prefix;
> > +             struct ap_ip_range *range;
> > +
> > +             if (ifa->ifa_addr->sa_family != AF_INET)
> > +                     continue;
> > +
> > +             /*
> > +              * Ignore the subnet set on the target interface, we'll be
> > +              * overwriting it if everything goes well.
> > +              */
> > +             if (!strcmp(ifa->ifa_name, ifname))
> > +                     continue;
> > +
> > +             addr_sa = (struct sockaddr_in *) ifa->ifa_addr;
> > +             netmask_sa = (struct sockaddr_in *) ifa->ifa_netmask;
> > +             if (!addr_sa || !netmask_sa)
> > +                     continue;
> > +
> > +             used_addr = ntohl(addr_sa->sin_addr.s_addr);
> > +             used_prefix = __builtin_popcountl(netmask_sa->sin_addr.s_addr);
> > +
> > +             range = l_new(struct ap_ip_range, 1);
> > +             range->start = used_addr & subnet_mask;
> > +             range->end = (range->start + (1 << (32 - used_prefix)) +
> > +                             subnet_size - 1) & subnet_mask;
> > +             l_queue_insert(used, range, ap_ip_range_compare, NULL);
> >       }
> >
> > -     if (getenv("IWD_DHCP_DEBUG"))
> > -             l_dhcp_server_set_debug(ap->server, do_debug,
> > -                                                     "[DHCPv4 SERV] ", NULL);
> > +     freeifaddrs(ifaddr);
> >
> > -     /* get the current address if there is one */
> > -     if (l_net_get_address(ifindex, &ia) && ia.s_addr != 0)
> > -             address = ia.s_addr;
> > +     /* Build the list of available subnets */
> >
> > -     if (ap->config->profile) {
> > -             char *addr;
> > -
> > -             addr = l_settings_get_string(settings, "IPv4", "Address");
> > -             if (addr) {
> > -                     if (inet_pton(AF_INET, addr, &ia) < 0)
> > -                             goto free_addr;
> > -
> > -                     /* Is a matching address already set on interface? */
> > -                     if (ia.s_addr == address)
> > -                             ret = -EALREADY;
> > -                     else
> > -                             ret = 0;
> > -             } else if (address) {
> > -                     /* No address in config, but interface has one set */
> > -                     addr = l_strdup(inet_ntoa(ia));
> > -                     ret = -EALREADY;
> > -             } else
> > -                     goto free_addr;
> > +     /* Check for the static IP syntax: Address=<IP> */
> > +     if (l_strv_length((char **) addr_str_list) == 1 &&
> > +                     inet_pton(AF_INET, *addr_str_list, &ia) == 1) {
> > +             struct ap_ip_range range;
> >
> > -             /* Set the remaining DHCP options in config file */
> > -             if (!dhcp_load_settings(ap, settings)) {
> > -                     ret = -EINVAL;
> > -                     goto free_addr;
> > +             host_addr = ntohl(ia.s_addr);
> > +             range.start = host_addr & subnet_mask;
> > +             range.end = range.start + subnet_size;
> > +             ap_append_range(ranges, &range, used, *addr_str_list);
> > +             goto select_addr;
> > +     }
> > +
> > +     for (i = 0; addr_str_list[i]; i++) {
> > +             struct ap_ip_range range;
> > +             uint8_t addr_prefix;
> > +
> > +             if (!util_ip_prefix_tohl(addr_str_list[i], &addr_prefix, &addr,
> > +                                             NULL, NULL)) {
> > +                     l_error("Can't parse %s as a subnet address",
> > +                             addr_str_list[i]);
> > +                     goto done;
> >               }
> >
> > -             if (!l_dhcp_server_set_ip_address(ap->server, addr)) {
> > -                     ret = -EINVAL;
> > -                     goto free_addr;
> > +             if (addr_prefix > prefix) {
> > +                     l_debug("Address spec %s smaller than requested "
> > +                             "subnet (prefix len %i)", addr_str_list[i],
> > +                             prefix);
> > +                     continue;
> >               }
> >
> > -             ap->own_ip = l_strdup(addr);
> > +             range.start = addr & subnet_mask;
> > +             range.end = range.start + (1 << (32 - addr_prefix));
> > +             ap_append_range(ranges, &range, used, addr_str_list[i]);
> > +     }
> > +
> > +select_addr:
> > +     if (l_queue_isempty(ranges)) {
> > +             l_error("No IP subnets available for new Access Point after "
> > +                     "eliminating those already in use on the system");
> > +             err = -EEXIST;
> > +             goto done;
> > +     }
> >
> > -free_addr:
> > -             l_free(addr);
> > +     /* Count available @prefix-sized subnets */
> > +     for (entry = l_queue_get_entries(ranges); entry; entry = entry->next) {
> > +             struct ap_ip_range *range = entry->data;
> >
> > -             return ret;
> > -     } else if (address) {
> > -             /* No config file and address is already set */
> > -             ap->own_ip = l_strdup(inet_ntoa(ia));
> > +             total += (range->end - range->start) >> (32 - prefix);
> > +     }
> >
> > -             return -EALREADY;
> > -     } else if (pool.used) {
> > -             /* No config file, no address set. Use IP pool */
> > -             ap->own_ip = ip_pool_get();
> > -             if (!ap->own_ip) {
> > -                     l_error("No more IP's in pool, cannot start AP on %u",
> > -                                     ifindex);
> > -                     return -EEXIST;
> > -             }
> > +     selected = l_getrandom_uint32() % total;
> >
> > -             ap->use_ip_pool = true;
> > -             ap->ip_prefix = pool.prefix;
> > +     /* Find the @selected'th @prefix-sized subnet */
> > +     for (entry = l_queue_get_entries(ranges);; entry = entry->next) {
> > +             struct ap_ip_range *range = entry->data;
> > +             uint32_t count = (range->end - range->start) >> (32 - prefix);
> >
> > -             return 0;
> > +             if (selected < count) {
> > +                     addr = range->start + (selected << (32 - prefix));
> > +                     break;
> > +             }
> > +
> > +             selected -= count;
> >       }
> >
> > -     return -EINVAL;
> > +     if (!host_addr)
> > +             host_addr = addr + 1;
> > +
> > +     err = 0;
> > +     *out_addr = host_addr;
> > +
> > +done:
> > +     l_queue_destroy(ranges, l_free);
> > +     l_queue_destroy(used, l_free);
> > +     return err;
> >   }
> >
> > -static int ap_load_profile_and_dhcp(struct ap_state *ap, bool *wait_dhcp)
> > +#define AP_DEFAULT_IPV4_PREFIX_LEN 28
> > +
> > +/*
> > + * This will determine the IP to be used for DHCP.  ap->server,
> > + * ap->own_ip and ap->ip_prefix will be set.
> > + *
> > + * The address to set (or keep) is determined in this order:
> > + * 1. Select from address pool defined in the provisioning file
> > + * 2. Keep address already set on the interface
> > + * 3. Select from the global address pool (default 192.168.0.0/16)
> > + *
> > + * Returns:  0 if an IP was successfully selected and needs to be set,
> > + *          -EALREADY if an IP is already set on the interface (success),
> > + *          -EEXIST if all available subnet addresses are in use (error),
> > + *          -EINVAL if there was a different error.
> > + */
> > +static int ap_setup_dhcp(struct ap_state *ap, uint32_t existing_address,
> > +                             uint8_t existing_prefix)
> >   {
> >       uint32_t ifindex = netdev_get_ifindex(ap->netdev);
> > -     char *passphrase;
> > -     struct l_settings *settings = NULL;
> > -     int err = -EINVAL;
> > +     struct in_addr if_addr;
> > +     struct in_addr if_netmask;
> > +     uint32_t address;
> > +     uint8_t prefix;
> > +     int ret = -EINVAL;
> > +     char **addr_str_list;
> > +     L_AUTO_FREE_VAR(char *, netmask_str) =
> > +             l_settings_get_string(ap->profile, "IPv4", "Netmask");
>
> Elsewhere you check that ap->profile is not NULL, but not here.  Seems inconsistent.

Good point.

>
> Also, I'm a bit lost why ap->profile might be NULL.  Shouldn't we simply create
> an empty profile instead to simplify the logic?

I'm not sure how much this is going to simplify the logic but we can
do that, again that wasn't my choice.

>
> >
> > -     /* No profile or DHCP settings */
> > -     if (!ap->config->profile && !pool.used)
> > -             return 0;
> > +     ap->server = l_dhcp_server_new(ifindex);
>
> Ah l_dhcp_server_new can't fail...

Ok.

>
> > +     if (!ap->server) {
> > +             l_error("Failed to create DHCP server on %u", ifindex);
> > +             return -EINVAL;
> > +     }
> >
> > -     if (ap->config->profile) {
> > -             settings = l_settings_new();
> > +     if (getenv("IWD_DHCP_DEBUG"))
> > +             l_dhcp_server_set_debug(ap->server, do_debug,
> > +                                                     "[DHCPv4 SERV] ", NULL);
> >
> > -             if (!l_settings_load_from_file(settings, ap->config->profile))
> > -                     goto cleanup;
> > +     if (ap->profile &&
> > +                     l_settings_get_value(ap->profile, "IPv4", "Address")) {
> > +             addr_str_list = l_settings_get_string_list(ap->profile, "IPv4",
> > +                                                             "Address", ',');
> > +             if (!addr_str_list || !*addr_str_list) {
> > +                     l_error("Can't parse the profile [IPv4].Address "
> > +                             "setting as a string list");
> > +                     goto done;
> > +             }
> > +     } else if (existing_address) {
> > +             addr_str_list = NULL;
> > +     } else
> > +             addr_str_list = global_addr_strs;
> >
> > -             passphrase = l_settings_get_string(settings, "Security",
> > -                                                     "Passphrase");
> > -             if (passphrase) {
> > -                     if (strlen(passphrase) > 63) {
> > -                             l_error("[Security].Passphrase must not exceed "
> > -                                             "63 characters");
> > -                             l_free(passphrase);
> > -                             goto cleanup;
> > +     if (addr_str_list) {
> > +             int r;
> > +
> > +             if (netmask_str) {
> > +                     uint32_t netmask;
> > +
> > +                     if (inet_pton(AF_INET, netmask_str, &if_netmask) < 0) {
> > +                             l_error("Can't parse the profile "
> > +                                     "[IPv4].Netmask setting");
> > +                             goto done;
> >                       }
> >
> > -                     strcpy(ap->config->passphrase, passphrase);
> > -                     l_free(passphrase);
> > -             }
> > +                     netmask = if_netmask.s_addr;
> > +                     prefix = __builtin_popcountl(netmask);
> > +                     if ((ntohl(netmask) !=
> > +                                     util_netmask_from_prefix(prefix))) {
> > +                             l_error("Invalid netmask %s", netmask_str);
> > +                             goto done;
> > +                     }
> >
> > -             if (!l_settings_has_group(settings, "IPv4")) {
> > -                     *wait_dhcp = false;
> > -                     err = 0;
> > -                     goto cleanup;
> > +                     if (prefix > 30 || prefix < 8) {
> > +                             l_error("Netmasks between 8 and 30 bits "
> > +                                     "are allowed");
> > +                             goto done;
> > +                     }
> > +             } else
> > +                     prefix = AP_DEFAULT_IPV4_PREFIX_LEN;
> > +
> > +             r = ap_select_addr((const char **) addr_str_list, prefix,
> > +                                     netdev_get_name(ap->netdev), &address);
> > +             if (r < 0) {
> > +                     ret = r;
> > +                     goto done;
> >               }
> > +     } else {
> > +             address = existing_address;
> > +             prefix = existing_prefix;
> > +     }
> > +
> > +     if_addr.s_addr = htonl(address);
> > +     if (!l_dhcp_server_set_ip_address(ap->server, inet_ntoa(if_addr))) {
> > +             l_error("l_dhcp_server_set_ip_address failed");
> > +             goto done;
> > +     }
> > +
> > +     if_netmask.s_addr = htonl(util_netmask_from_prefix(prefix));
> > +     if (!l_dhcp_server_set_netmask(ap->server, inet_ntoa(if_netmask))) {
> > +             l_error("l_dhcp_server_set_netmask failed");
> > +             goto done;
> >       }
> >
> > -     err = ap_setup_dhcp(ap, settings);
> > +     /* Set the remaining DHCP options in config file */
> > +     if (ap->profile && !ap_load_dhcp_settings(ap, address, prefix))
> > +             goto done;
> > +
> > +     ap->own_ip = l_strdup(inet_ntoa(if_addr));
> > +     ap->ip_prefix = prefix;
> > +     ret = (address == existing_address && prefix == existing_prefix) ?
> > +             -EALREADY : 0;
> > +
> > +done:
> > +     if (addr_str_list != global_addr_strs)
> > +             l_strv_free(addr_str_list);
> > +
> > +     return ret;
> > +}
> > +
> > +static void ap_netconfig_start(struct ap_state *ap, uint32_t existing_addr,
> > +                             uint8_t existing_prefix)
> > +{
> > +     uint32_t ifindex = netdev_get_ifindex(ap->netdev);
> > +     int err;
> > +
> > +     err = ap_setup_dhcp(ap, existing_addr, existing_prefix);
> >       if (err == 0) {
> >               /* Address change required */
> >               ap->rtnl_add_cmd = l_rtnl_ifaddr4_add(rtnl, ifindex,
> >                                       ap->ip_prefix, ap->own_ip,
> >                                       broadcast_from_ip(ap->own_ip),
> >                                       ap_ifaddr4_added_cb, ap, NULL);
> > -
> > -             if (!ap->rtnl_add_cmd) {
> > -                     l_error("Failed to add IPv4 address");
> > -                     err = -EIO;
> > -                     goto cleanup;
> > +             if (ap->rtnl_add_cmd) {
> > +                     ap->cleanup_ip = true;
> > +                     return;
> >               }
>
> So we decided not to remove any existing addresses?

I didn't decide it ;-) but I think it's safe to not remove them if
we're lazy.  You said:

21:51 <@denkenz_> add something like l_rtnl_ifaddr_flush
21:53 <@denkenz_> or simply assign multiple addresses, that works too

So you seemed to be ok with this, in any case it's orthogonal to this patchset.

>
> >
> > -             ap->cleanup_ip = true;
> > -
> > -             *wait_dhcp = true;
> > -             err = 0;
> > +             l_error("Failed to add IPv4 address");
> >       /* Selected address already set, continue normally */
> >       } else if (err == -EALREADY) {
> > -             *wait_dhcp = false;
> > -             err = 0;
> > +             if (ap_start_send(ap))
> > +                     return;
> >       }
> >
> > -cleanup:
> > -     l_settings_free(settings);
> > -     return err;
> > +     ap_start_failed(ap);
> > +}
> > +
> > +static void ap_ifaddr4_dump_cb(int error,
> > +                             uint16_t type, const void *data,
> > +                             uint32_t len, void *user_data)
> > +{
> > +     struct ap_state *ap = user_data;
> > +     const struct ifaddrmsg *ifa = data;
> > +     char *ip_str = NULL;
> > +     struct in_addr ip = {};
> > +     uint8_t prefix = 0;
> > +     uint32_t cmd;
> > +
> > +     if (!error && ifa->ifa_index != netdev_get_ifindex(ap->netdev))
> > +             return;
> > +
> > +     if (error || type != RTM_NEWADDR ||
> > +                     ifa->ifa_prefixlen > 30 || ifa->ifa_prefixlen < 1) {
> > +             l_error("Error getting existing IPv4 address on AP iface");
> > +             goto done;
> > +     }
> > +
> > +     l_rtnl_ifaddr4_extract(ifa, len, NULL, &ip_str, NULL);
>
> So FYI, this API is slated for the chopping block now that we have
> l_rtnl_address object.  I just haven't gotten around to it yet.
>
> > +     inet_aton(ip_str, &ip);
> > +     l_free(ip_str);
> > +     prefix = ifa->ifa_prefixlen;
> > +
> > +done:
> > +     cmd = ap->rtnl_dump_cmd;
> > +     ap->rtnl_dump_cmd = 0;
> > +     l_netlink_cancel(rtnl, cmd);
> > +
> > +     ap_netconfig_start(ap, ntohl(ip.s_addr), prefix);
> > +}
> > +
> > +static void ap_ifaddr4_dump_destroy_cb(void *user_data)
> > +{
> > +     struct ap_state *ap = user_data;
> > +
> > +     if (!ap->rtnl_dump_cmd)
> > +             return;
> > +
> > +     ap->rtnl_dump_cmd = 0;
> > +     l_debug("No existing IPv4 addresses on AP iface");
> > +     ap_netconfig_start(ap, 0, 0);
> >   }
> >
> >   /*
> > @@ -2639,13 +2815,10 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
> >   {
> >       struct ap_state *ap;
> >       struct wiphy *wiphy = netdev_get_wiphy(netdev);
> > -     struct l_genl_msg *cmd;
> >       uint64_t wdev_id = netdev_get_wdev_id(netdev);
> > -     int err = -EINVAL;
> > -     bool wait_on_address = false;
> >
> >       if (err_out)
> > -             *err_out = err;
> > +             *err_out = -EINVAL;
> >
> >       if (L_WARN_ON(!config->ssid))
> >               return NULL;
> > @@ -2661,15 +2834,33 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
> >       ap->ops = ops;
> >       ap->user_data = user_data;
> >
> > -     /*
> > -      * This both loads a profile if required and loads DHCP settings either
> > -      * by the profile itself or the IP pool (or does nothing in the case
> > -      * of a profile-less configuration). wait_on_address will be set true
> > -      * if an address change is required.
> > -      */
> > -     err = ap_load_profile_and_dhcp(ap, &wait_on_address);
> > -     if (err < 0)
> > -             goto error;
> > +     if (ap->config->profile) {
> > +             char *passphrase;
> > +
> > +             ap->profile = l_settings_new();
> > +
> > +             if (!l_settings_load_from_file(ap->profile, ap->config->profile))
> > +                     goto error;
> > +
> > +             passphrase = l_settings_get_string(ap->profile, "Security",
> > +                                                     "Passphrase");
> > +             if (!passphrase) {
> > +                     l_error("[Security].Passphrase not found in "
> > +                             "AP profile");
> > +                     goto error;
> > +             }
> > +
> > +             /* (full checks done later in crypto_passphrase_is_valid) */
> > +             if (strlen(passphrase) > 63) {
> > +                     l_error("[Security].Passphrase must not exceed "
> > +                             "63 characters");
> > +                     l_free(passphrase);
> > +                     goto error;
> > +             }
> > +
> > +             strcpy(ap->config->passphrase, passphrase);
> > +             l_free(passphrase);
> > +     }
> >
> >       if (!config->channel)
> >               /* TODO: Start a Get Survey to decide the channel */
> > @@ -2752,33 +2943,31 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
> >       if (!ap->mlme_watch)
> >               l_error("Registering for MLME notification failed");
> >
> > -     if (wait_on_address) {
> > -             if (err_out)
> > -                     *err_out = 0;
> > +     if (netconfig_enabled &&
> > +                     (!ap->profile ||
> > +                      l_settings_has_group(ap->profile, "IPv4"))) {
>
> It seems to me the logic should go something like this:
>
> 1. Load the profile
> 2. If the profile contains a single address / subnet, go to step 6

So the current logic retrieves the current address anyway just to
check if it's the same as the one we want to set, I think it doesn't
harm to do this and the only difference is steps 2 and 3 are reversed.

> 3. Issue ifaddr4_dump and record any used ranges
> 4. If an address is set on the interface and IPv4.Address isn't set, pick one
> (non secondary address), doesn't matter which one and use it as the DHCP
> address.  Goto step 7.
> 5. Pick an address from the pool taking into account used ranges found in step 3.
> 6. Set the address picked in step 2, or step 5
> 7. Start DHCP & AP

Best regards

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

* Re: [PATCH 1/5] ap: Refactor IP address selection
  2021-03-03 19:17   ` Andrew Zaborowski
@ 2021-03-03 20:30     ` Denis Kenzior
  0 siblings, 0 replies; 8+ messages in thread
From: Denis Kenzior @ 2021-03-03 20:30 UTC (permalink / raw)
  To: iwd

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

Hi Andrew,

>> One thing I don't like about this is that the primary IP selection is done
>> elsewhere, but then Netmask/IPRange settings are parsed after the fact.
> 
> Actually I'm not a fan of that either but that's how the existing (and
> reviewed?) code does it. In this patch I'm moving the Netconfig
> handling together with the subnet selection but I didn't touch the
> IPRange parsing, the change is already big as you noted.
> 

Still... The existing code could only work with a single subnet, you're doing 
something more flexible...

I think the fix for this is to break up your changes up into more manageable 
chunks instead of blaming that the 'change is too big' ;)

>>   I'm not
>> sure if it makes sense for a profile to have Netmask/IPRange settings if it also
>> doesn't have the Address set to a single subnet.
> 
> Agreed about IPRange, but for Netmask I see it as a way to specify the
> prefix length basically, i.e. how much address space the AP wants.

Sure, I get that.  And you added some sanity checking for IPRange.  But there's 
no point doing all the work of dumping addresses, selecting ranges if the 
IPRange / Netmask settings make no sense in the first place.  Fail early.

> 
>>   l_dhcp_server probably only
>> works with prefixes >= 24 when allocating addresses.
> 
> I haven't looked at it but even then you may want to specify your own
> value between 24 - 28 or 29... 28 being the new default.

These will work, but I'm not sure a prefix length of 23 would work for example.

> 
> (Obviously all of these are very specific settings... I assume there's
> some reason why we have them)
> 

Mostly testing, but we've documented them, so you can't just handwave them away :)

>>   Shouldn't we load the
>> profile and verify it mostly in one place?
> 
> I haven't really checked but my assumption was that the existing code
> skips some of the verification because the l_dhcp_* methods would
> verify the values passed to it and we avoid duplicating that work.
> 

It skips verification because the assumption was that only a single class C 
subnet would be setup in the profile.  This is no longer the case...  I don't 
think l_dhcp_server_* does much verification, if any.

>>
>> Also, I'm a bit lost why ap->profile might be NULL.  Shouldn't we simply create
>> an empty profile instead to simplify the logic?
> 
> I'm not sure how much this is going to simplify the logic but we can
> do that, again that wasn't my choice.
> 

Anything that simplifies the logic helps.  And besides, this could be a simple 
cleanup commit that you build up on.

>>
>> So we decided not to remove any existing addresses?
> 
> I didn't decide it ;-) but I think it's safe to not remove them if
> we're lazy.  You said:
> 
> 21:51 <@denkenz_> add something like l_rtnl_ifaddr_flush
> 21:53 <@denkenz_> or simply assign multiple addresses, that works too
> 
> So you seemed to be ok with this, in any case it's orthogonal to this patchset.
> 

That's fine.  We had discussed removing the old addresses as well, so I'm just 
checking what you decided on.

>> It seems to me the logic should go something like this:
>>
>> 1. Load the profile
>> 2. If the profile contains a single address / subnet, go to step 6
> 
> So the current logic retrieves the current address anyway just to
> check if it's the same as the one we want to set, I think it doesn't
> harm to do this and the only difference is steps 2 and 3 are reversed.
> 

Yeah I thought you'd say that ;)  Anyway, I'm fine doing it this way too.

>> 3. Issue ifaddr4_dump and record any used ranges
>> 4. If an address is set on the interface and IPv4.Address isn't set, pick one
>> (non secondary address), doesn't matter which one and use it as the DHCP
>> address.  Goto step 7.
>> 5. Pick an address from the pool taking into account used ranges found in step 3.
>> 6. Set the address picked in step 2, or step 5
>> 7. Start DHCP & AP
> 

Regards,
-Denis

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

end of thread, other threads:[~2021-03-03 20:30 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-02 12:08 [PATCH 1/5] ap: Refactor IP address selection Andrew Zaborowski
2021-03-02 12:08 ` [PATCH 2/5] ap: Send a specific error message on async AP start failure Andrew Zaborowski
2021-03-02 12:08 ` [PATCH 3/5] ap: Move the DHCP server freeing to ap_reset Andrew Zaborowski
2021-03-02 12:08 ` [PATCH 4/5] autotests: Update APRanges usage in testAP Andrew Zaborowski
2021-03-02 12:08 ` [PATCH 5/5] doc: Update AP settings in iwd.ap and iwd.config(5) Andrew Zaborowski
2021-03-03 17:14 ` [PATCH 1/5] ap: Refactor IP address selection Denis Kenzior
2021-03-03 19:17   ` Andrew Zaborowski
2021-03-03 20:30     ` Denis Kenzior

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.