[PATCH 1/9] ap: Make rtnl global static

[PATCH 1/9] ap: Make rtnl global static

[Andrew Zaborowski]

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

---
 src/ap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/ap.c b/src/ap.c
index 8851cb85..7eac07d7 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -141,7 +141,7 @@ struct ap_ip_pool {
 
 struct ap_ip_pool pool;
 static uint32_t netdev_watch;
-struct l_netlink *rtnl;
+static struct l_netlink *rtnl;
 
 /*
  * Creates pool of IPs which AP intefaces can use. Each call to ip_pool_get
-- 
2.27.0

[PATCH 2/9] ap: Track IPv4 addresses in use

[Andrew Zaborowski]

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

Add the ap_addr submodule that tracks IPv4 addresses in use on the
system for use when selecting the address for a new AP.
---
 Makefile.am   |   3 +-
 src/ap-addr.c | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/ap.h      |   9 +++
 3 files changed, 177 insertions(+), 1 deletion(-)
 create mode 100644 src/ap-addr.c

diff --git a/Makefile.am b/Makefile.am
index 68035e46..631ab710 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -220,7 +220,8 @@ src_iwd_SOURCES = src/main.c linux/nl80211.h src/iwd.h src/missing.h \
 					src/knownnetworks.c \
 					src/rfkill.h src/rfkill.c \
 					src/ft.h src/ft.c \
-					src/ap.h src/ap.c src/adhoc.c \
+					src/ap.h src/ap.c src/ap-addr.c \
+					src/adhoc.c \
 					src/sae.h src/sae.c \
 					src/nl80211util.h src/nl80211util.c \
 					src/nl80211cmd.h src/nl80211cmd.c \
diff --git a/src/ap-addr.c b/src/ap-addr.c
new file mode 100644
index 00000000..238cc7b5
--- /dev/null
+++ b/src/ap-addr.c
@@ -0,0 +1,166 @@
+/*
+ *
+ *  Wireless daemon for Linux
+ *
+ *  Copyright (C) 2021  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
+
+#include <ell/ell.h>
+
+#include "src/util.h"
+#include "src/iwd.h"
+#include "src/module.h"
+#include "src/netdev.h"
+#include "src/mpdu.h"
+#include "src/ap.h"
+
+static struct l_queue *used_addr4_list;
+static struct l_netlink *rtnl;
+
+static bool ap_rtnl_addr4_match_ifindex(const void *a, const void *b)
+{
+	const struct ap_rtnl_addr4_record *addr = a;
+
+	return addr->ifindex == L_PTR_TO_UINT(b) && !addr->secondary;
+}
+
+const struct ap_rtnl_addr4_record *ap_get_addr4(struct netdev *netdev)
+{
+	uint32_t ifindex = netdev ? netdev_get_ifindex(netdev) : 0;
+
+	return l_queue_find(used_addr4_list, ap_rtnl_addr4_match_ifindex,
+				L_UINT_TO_PTR(ifindex));
+}
+
+static bool ap_rtnl_addr4_match_free(void *data, void *user_data)
+{
+	const struct ap_rtnl_addr4_record *a = data;
+	const struct ap_rtnl_addr4_record *b = user_data;
+
+	if (a->ifindex != b->ifindex || a->addr != b->addr ||
+			a->prefix_len != b->prefix_len)
+		return false;
+
+	l_free(data);
+	return true;
+}
+
+static void ap_addr_notify(uint16_t type, const void *data, uint32_t len,
+				void *user_data)
+{
+	const struct ifaddrmsg *ifa = data;
+	char *ip_str = NULL;
+	int r;
+	struct in_addr ip;
+
+	if (ifa->ifa_family != AF_INET || ifa->ifa_prefixlen < 1)
+		return;
+
+	len -= NLMSG_ALIGN(sizeof(struct ifaddrmsg));
+
+	l_rtnl_ifaddr4_extract(ifa, len, NULL, &ip_str, NULL);
+	if (!ip_str)
+		return;
+
+	r = inet_aton(ip_str, &ip);
+	l_free(ip_str);
+
+	if (r < 0)
+		return;
+
+	if (type == RTM_NEWADDR) {
+		struct ap_rtnl_addr4_record *addr;
+
+		addr = l_new(struct ap_rtnl_addr4_record, 1);
+		addr->addr = ntohl(ip.s_addr);
+		addr->prefix_len = ifa->ifa_prefixlen;
+		addr->ifindex = ifa->ifa_index;
+		addr->secondary = !!(ifa->ifa_flags & IFA_F_SECONDARY);
+
+		if (!used_addr4_list)
+			used_addr4_list = l_queue_new();
+
+		l_queue_push_tail(used_addr4_list, addr);
+	} else if (type == RTM_DELADDR) {
+		struct ap_rtnl_addr4_record addr;
+
+		addr.addr = ntohl(ip.s_addr);
+		addr.prefix_len = ifa->ifa_prefixlen;
+		addr.ifindex = ifa->ifa_index;
+
+		l_queue_foreach_remove(used_addr4_list,
+					ap_rtnl_addr4_match_free, &addr);
+	}
+}
+
+static void ap_addr4_dump_cb(int error,
+				uint16_t type, const void *data,
+				uint32_t len, void *user_data)
+{
+	if (error) {
+		l_error("addr4_dump_cb: %s (%i)", strerror(-error), -error);
+		return;
+	}
+
+	ap_addr_notify(type, data, len, user_data);
+}
+
+static int ap_addr_init(void)
+{
+	const struct l_settings *settings = iwd_get_config();
+	bool netconfig_enabled;
+
+	if (!l_settings_get_bool(settings, "General",
+				"EnableNetworkConfiguration",
+				&netconfig_enabled))
+		netconfig_enabled = false;
+
+	if (!netconfig_enabled)
+		return 0;
+
+	rtnl = iwd_get_rtnl();
+
+	if (!l_netlink_register(rtnl, RTNLGRP_IPV4_IFADDR,
+				ap_addr_notify, NULL, NULL)) {
+		l_error("Failed to register for RTNL link notifications");
+		return -EIO;
+	}
+
+	if (!l_rtnl_ifaddr4_dump(rtnl, ap_addr4_dump_cb, NULL, NULL)) {
+		l_error("Sending the IPv4 addr dump req failed");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void ap_addr_exit(void)
+{
+	l_queue_destroy(used_addr4_list, l_free);
+	used_addr4_list = NULL;
+}
+
+IWD_MODULE(ap_addr, ap_addr_init, ap_addr_exit)
diff --git a/src/ap.h b/src/ap.h
index b0100f34..12baca8b 100644
--- a/src/ap.h
+++ b/src/ap.h
@@ -97,3 +97,12 @@ bool ap_station_disconnect(struct ap_state *ap, const uint8_t *mac,
 
 bool ap_push_button(struct ap_state *ap);
 void ap_update_beacon(struct ap_state *ap);
+
+struct ap_rtnl_addr4_record {
+	uint32_t addr;
+	uint8_t prefix_len;
+	uint32_t ifindex;
+	bool secondary;
+};
+
+const struct ap_rtnl_addr4_record *ap_get_addr4(struct netdev *netdev);
-- 
2.27.0

[PATCH 3/9] ap: Add subnet address selection logic

[Andrew Zaborowski]

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

Add the ap_select_addr4 function to select a random subnet of requested
size from an address space defined by a string list (for use with the
AP profile [IPv4].Address and the global [IPv4].APAddressPool settings),
avoiding those subnets that conflict with subnets in use.  We take care
to give a similar weight to all subnets contained in the specified
ranges regardless of how many ranges contain each, basically so that
overlapping ranges don't affect the probabilities (debatable.)
---
 src/ap-addr.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/ap.h      |   2 +
 2 files changed, 204 insertions(+)

diff --git a/src/ap-addr.c b/src/ap-addr.c
index 238cc7b5..b2a87b37 100644
--- a/src/ap-addr.c
+++ b/src/ap-addr.c
@@ -37,9 +37,211 @@
 #include "src/mpdu.h"
 #include "src/ap.h"
 
+struct ap_ip_range {
+	uint32_t start;
+	uint32_t end;
+};
+
 static struct l_queue *used_addr4_list;
 static struct l_netlink *rtnl;
 
+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 ? 1 : -1;
+}
+
+/*
+ * 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 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.
+ *
+ * Returns:  0 when an address was selected and written to *out_addr,
+ *          -EEXIST if all available subnet addresses are in use,
+ *          -EINVAL if there was a different error.
+ */
+int ap_select_addr4(const char **addr_str_list, uint8_t prefix,
+			struct netdev *netdev, uint32_t *out_addr)
+{
+	uint32_t total = 0;
+	uint32_t selected;
+	unsigned int i;
+	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;
+	const struct l_queue_entry *entry;
+	uint32_t ifindex = netdev ? netdev_get_ifindex(netdev) : 0;
+	int err = -EINVAL;
+
+	/* Build a sorted list of used/unavailable subnets */
+	for (entry = l_queue_get_entries((struct l_queue *) used_addr4_list);
+			entry; entry = entry->next) {
+		const struct ap_rtnl_addr4_record *rec = entry->data;
+		struct ap_ip_range *range;
+
+		/*
+		 * Don't add current subnets from the target interface to the used
+		 * subnet address list because we'll be replacing them (or adding
+		 * another secondary address) so there won't be a routing conflict.
+		 */
+		if (rec->ifindex == ifindex)
+			continue;
+
+		if (!rec->addr || !rec->prefix_len)
+			continue;
+
+		range = l_new(struct ap_ip_range, 1);
+		range->start = rec->addr & subnet_mask;
+		range->end = (range->start + (1 << (32 - rec->prefix_len)) +
+				subnet_size - 1) & subnet_mask;
+		l_queue_insert(used, range, ap_ip_range_compare, NULL);
+	}
+
+	/* Build the list of available subnets */
+
+	/* 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;
+
+		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 check_avail;
+	}
+
+	for (i = 0; addr_str_list[i]; i++) {
+		struct ap_ip_range range;
+		uint32_t addr;
+		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 cleanup;
+		}
+
+		if (addr_prefix > prefix) {
+			l_debug("Address spec %s smaller than requested "
+				"subnet (prefix len %i)", addr_str_list[i],
+				prefix);
+			continue;
+		}
+
+		range.start = addr & subnet_mask;
+		range.end = range.start + (1 << (32 - addr_prefix));
+		ap_append_range(ranges, &range, used, addr_str_list[i]);
+	}
+
+check_avail:
+	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 cleanup;
+	}
+
+	if (host_addr)
+		goto done;
+
+	/* Count available @prefix-sized subnets */
+	for (entry = l_queue_get_entries(ranges); entry; entry = entry->next) {
+		struct ap_ip_range *range = entry->data;
+
+		total += (range->end - range->start) >> (32 - prefix);
+	}
+
+	selected = l_getrandom_uint32() % total;
+
+	/* 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);
+
+		if (selected < count) {
+			host_addr = range->start + (selected << (32 - prefix));
+			break;
+		}
+
+		selected -= count;
+	}
+
+	if ((host_addr & 0xff) == 0)
+		host_addr += 1;
+
+done:
+	err = 0;
+	*out_addr = host_addr;
+
+cleanup:
+	l_queue_destroy(ranges, l_free);
+	l_queue_destroy(used, l_free);
+	return err;
+}
+
 static bool ap_rtnl_addr4_match_ifindex(const void *a, const void *b)
 {
 	const struct ap_rtnl_addr4_record *addr = a;
diff --git a/src/ap.h b/src/ap.h
index 12baca8b..ff6b635d 100644
--- a/src/ap.h
+++ b/src/ap.h
@@ -105,4 +105,6 @@ struct ap_rtnl_addr4_record {
 	bool secondary;
 };
 
+int ap_select_addr4(const char **addr_str_list, uint8_t prefix,
+			struct netdev *netdev, uint32_t *out_addr);
 const struct ap_rtnl_addr4_record *ap_get_addr4(struct netdev *netdev);
-- 
2.27.0

[PATCH 4/9] ap: Refactor DHCP settings loading

[Andrew Zaborowski]

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

Extend the [IPv4].Address setting's syntax to allow a new format: a list
of <IP>/<prefix_len> -form strings that define the address space from
which a subnet is selected.  Rewrite the DHCP settings loading with
other notable changes:

 * validate some of the settings more thoroughly,
 * name all netconfig-related ap_state members with the netconfig_
   prefix,
 * make sure we always call l_dhcp_server_set_netmask(),
 * allow netmasks other than 24-bit and change the default to 28 bits,
 * as requested avoid using the l_net_ ioctl-based functions although
   l_dhcp still uses them internally,
 * as requested avoid touching the ap_state members until the end of
   some functions so that on error they're basically a no-op (for
   readabaility).
---
 src/ap.c | 423 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 261 insertions(+), 162 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 7eac07d7..a3dee5e4 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -88,15 +88,15 @@ struct ap_state {
 	uint16_t last_aid;
 	struct l_queue *sta_states;
 
-	struct l_dhcp_server *server;
+	struct l_dhcp_server *netconfig_dhcp;
+	char *netconfig_addr4_str;
+	uint8_t netconfig_prefix_len4;
 	uint32_t rtnl_add_cmd;
-	char *own_ip;
-	unsigned int ip_prefix;
 
 	bool started : 1;
 	bool gtk_set : 1;
-	bool cleanup_ip : 1;
-	bool use_ip_pool : 1;
+	bool netconfig_set_addr4 : 1;
+	bool netconfig_use_ip_pool : 1;
 };
 
 struct sta_state {
@@ -173,15 +173,14 @@ static bool ip_pool_create(const char *ip_prefix)
 	return true;
 }
 
-static char *ip_pool_get()
+static uint32_t 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;
+		return 0;
 
 	l_uintset_put(pool.used, next_subnet);
 
@@ -189,8 +188,7 @@ static char *ip_pool_get()
 	ip &= 0xffff00ff;
 	ip |= (next_subnet << 8);
 
-	ia.s_addr = htonl(ip);
-	return l_strdup(inet_ntoa(ia));
+	return ip;
 }
 
 static bool ip_pool_put(const char *address)
@@ -321,22 +319,26 @@ static void ap_reset(struct ap_state *ap)
 	ap->started = false;
 
 	/* Delete IP if one was set by IWD */
-	if (ap->cleanup_ip)
+	if (ap->netconfig_set_addr4) {
 		l_rtnl_ifaddr4_delete(rtnl, netdev_get_ifindex(netdev),
-					ap->ip_prefix, ap->own_ip,
-					broadcast_from_ip(ap->own_ip),
-					NULL, NULL, NULL);
+				ap->netconfig_prefix_len4,
+				ap->netconfig_addr4_str,
+				broadcast_from_ip(ap->netconfig_addr4_str),
+				NULL, NULL, NULL);
+		ap->netconfig_set_addr4 = false;
+	}
 
-	if (ap->own_ip) {
+	if (ap->netconfig_use_ip_pool) {
 		/* Release IP from pool if used */
-		if (ap->use_ip_pool)
-			ip_pool_put(ap->own_ip);
-
-		l_free(ap->own_ip);
+		ip_pool_put(ap->netconfig_addr4_str);
+		ap->netconfig_use_ip_pool = false;
 	}
 
-	if (ap->server)
-		l_dhcp_server_stop(ap->server);
+	l_free(ap->netconfig_addr4_str);
+	ap->netconfig_addr4_str = NULL;
+
+	if (ap->netconfig_dhcp)
+		l_dhcp_server_stop(ap->netconfig_dhcp);
 }
 
 static void ap_del_station(struct sta_state *sta, uint16_t reason,
@@ -2163,7 +2165,7 @@ static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
 		goto failed;
 	}
 
-	if (ap->server && !l_dhcp_server_start(ap->server)) {
+	if (ap->netconfig_dhcp && !l_dhcp_server_start(ap->netconfig_dhcp)) {
 		l_error("DHCP server failed to start");
 		goto failed;
 	}
@@ -2490,182 +2492,267 @@ static void ap_mlme_notify(struct l_genl_msg *msg, void *user_data)
 	}
 }
 
-static bool dhcp_load_settings(struct ap_state *ap,
-				const struct l_settings *settings)
+#define AP_DEFAULT_IPV4_PREFIX_LEN 28
+
+static int ap_setup_netconfig4(struct ap_state *ap, const char **addr_str_list,
+				uint8_t prefix_len, const char *gateway_str,
+				const char **ip_range,
+				const char **dns_str_list,
+				unsigned int lease_time)
 {
-	struct l_dhcp_server *server = ap->server;
+	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
+	const struct ap_rtnl_addr4_record *addr_rec = ap_get_addr4(ap->netdev);
+	uint32_t addr;
+	int ret;
 	struct in_addr ia;
+	struct l_dhcp_server *dhcp;
+	bool use_ip_pool = false;
+	bool r;
 
-	L_AUTO_FREE_VAR(char *, netmask) = l_settings_get_string(settings,
-							"IPv4", "Netmask");
-	L_AUTO_FREE_VAR(char *, gateway) = l_settings_get_string(settings,
-							"IPv4", "Gateway");
-	char **dns = l_settings_get_string_list(settings, "IPv4",
-							"DNSList", ',');
-	char **ip_range = l_settings_get_string_list(settings, "IPv4",
-							"IPRange", ',');
-	unsigned int lease_time;
-	bool ret = false;
+	dhcp = l_dhcp_server_new(ifindex);
+	if (!dhcp) {
+		l_error("Failed to create DHCP server on ifindex %u", ifindex);
+		return -EIO;
+	}
+
+	if (getenv("IWD_DHCP_DEBUG"))
+		l_dhcp_server_set_debug(dhcp, do_debug,
+					"[DHCPv4 SERV] ", NULL);
+
+	/*
+	 * The address pool specified for this AP (if any) has the priority,
+	 * next is the address currently set on the interface (if any) and
+	 * last is the global AP address pool (APRanges setting).
+	 */
+	if (addr_str_list) {
+		if (!prefix_len)
+			prefix_len = AP_DEFAULT_IPV4_PREFIX_LEN;
+
+		ret = ap_select_addr4(addr_str_list,
+					prefix_len, ap->netdev, &addr);
+	} else if (addr_rec && addr_rec->addr &&
+			addr_rec->prefix_len > 0 && addr_rec->prefix_len < 31) {
+		addr = addr_rec->addr;
+
+		if (!prefix_len)
+			prefix_len = addr_rec->prefix_len;
+
+		ret = 0;
+	} else {
+		if (prefix_len) {
+			l_error("[IPv4].Netmask can't be used without an "
+				"address");
+			ret = -EINVAL;
+			goto done;
+		}
+
+		/* No config, no address set. Use IP pool */
+		addr = ip_pool_get();
+		if (addr) {
+			use_ip_pool = true;
+			prefix_len = pool.prefix;
+			ret = 0;
+		} else {
+			l_error("No more IP's in pool, cannot start AP on %u",
+					ifindex);
+			ret = -EEXIST;
+		}
+	}
+
+	if (ret)
+		goto error;
 
-	if (!l_settings_get_uint(settings, "IPv4", "LeaseTime", &lease_time))
-		lease_time = 0;
+	ret = -EIO;
 
-	if (gateway && !l_dhcp_server_set_gateway(server, gateway)) {
-		l_error("[IPv4].Gateway value error");
+	/*
+	 * l_dhcp_server_start() would retrieve the current IPv4 from
+	 * the interface but set it anyway in case there are multiple
+	 * addresses, saves one ioctl too.
+	 */
+	ia.s_addr = htonl(addr);
+
+	if (!l_dhcp_server_set_ip_address(dhcp, inet_ntoa(ia))) {
+		l_error("l_dhcp_server_set_ip_address failed");
 		goto error;
 	}
 
-	if (dns && !l_dhcp_server_set_dns(server, dns)) {
-		l_error("[IPv4].DNSList value error");
+	ia.s_addr = htonl(util_netmask_from_prefix(prefix_len));
+
+	if (!l_dhcp_server_set_netmask(dhcp, inet_ntoa(ia))) {
+		l_error("l_dhcp_server_set_netmask failed");
 		goto error;
 	}
 
-	if (netmask && !l_dhcp_server_set_netmask(server, netmask)) {
-		l_error("[IPv4].Netmask value error");
+	if (gateway_str && !l_dhcp_server_set_gateway(dhcp, gateway_str)) {
+		l_error("l_dhcp_server_set_gateway failed");
 		goto error;
 	}
 
 	if (ip_range) {
-		if (l_strv_length(ip_range) != 2) {
-			l_error("Two addresses expected in [IPv4].IPRange");
+		r = l_dhcp_server_set_ip_range(dhcp, ip_range[0], ip_range[1]);
+		if (!r) {
+			l_error("l_dhcp_server_set_ip_range failed");
 			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");
+	if (dns_str_list) {
+		r = l_dhcp_server_set_dns(dhcp, (char **) dns_str_list);
+		if (!r) {
+			l_error("l_dhcp_server_set_dns failed");
 			goto error;
 		}
 	}
 
-	if (lease_time && !l_dhcp_server_set_lease_time(server, lease_time)) {
-		l_error("[IPv4].LeaseTime value error");
+	if (lease_time && !l_dhcp_server_set_lease_time(dhcp, lease_time)) {
+		l_error("l_dhcp_server_set_lease_time failed");
 		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;
+	ia.s_addr = htonl(addr);
+	ap->netconfig_addr4_str = l_strdup(inet_ntoa(ia));
+	ap->netconfig_prefix_len4 = prefix_len;
+	ap->netconfig_set_addr4 = !addr_rec || addr_rec->addr != addr ||
+		addr_rec->prefix_len > prefix_len;
+	ap->netconfig_use_ip_pool = use_ip_pool;
+	ap->netconfig_dhcp = dhcp;
+	return 0;
 
 error:
-	l_strv_free(dns);
-	l_strv_free(ip_range);
+	l_dhcp_server_destroy(dhcp);
 	return ret;
 }
 
-/*
- * 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 or
- *            IP configuration was not enabled,
- *          -EEXIST if the IP pool ran out of IP's
- *          -EINVAL if there was an error.
- */
-static int ap_setup_dhcp(struct ap_state *ap, const struct l_settings *settings)
+static int ap_load_ipv4(struct ap_state *ap, const struct l_settings *config)
 {
-	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
-	struct in_addr ia;
-	uint32_t address = 0;
-	L_AUTO_FREE_VAR(char *, addr) = NULL;
 	int ret = -EINVAL;
+	char **addr_str_list = NULL;
+	uint32_t static_addr = 0;
+	uint8_t prefix_len = 0;
+	char *gateway_str = NULL;
+	char **ip_range = NULL;
+	char **dns_str_list = NULL;
+	unsigned int lease_time = 0;
+	struct in_addr ia;
 
-	/* get the current address if there is one */
-	if (l_net_get_address(ifindex, &ia) && ia.s_addr != 0)
-		address = ia.s_addr;
+	if (!l_settings_has_group(config, "IPv4") || !pool.used)
+		return 0;
 
-	addr = l_settings_get_string(settings, "IPv4", "Address");
-	if (addr) {
-		if (inet_pton(AF_INET, addr, &ia) < 0)
-			return -EINVAL;
+	if (l_settings_has_key(config, "IPv4", "Address")) {
+		addr_str_list = l_settings_get_string_list(config, "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;
+		}
 
-		/* 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 if (pool.used) {
-		/* No config file, no address set. Use IP pool */
-		addr = ip_pool_get();
-		if (!addr) {
-			l_error("No more IP's in pool, cannot start AP on %u",
-					ifindex);
-			return -EEXIST;
+		/* Check for the static IP syntax: Address=<IP> */
+		if (l_strv_length(addr_str_list) == 1 &&
+				inet_pton(AF_INET, *addr_str_list, &ia) == 1)
+			static_addr = ntohl(ia.s_addr);
+	}
+
+	if (l_settings_has_key(config, "IPv4", "Netmask")) {
+		L_AUTO_FREE_VAR(char *, netmask_str) =
+			l_settings_get_string(config, "IPv4", "Netmask");
+
+		if (inet_pton(AF_INET, netmask_str, &ia) != 1) {
+			l_error("Can't parse the profile [IPv4].Netmask "
+				"setting");
+			goto done;
 		}
 
-		ap->use_ip_pool = true;
-		ap->ip_prefix = pool.prefix;
-		ret = 0;
-	} else
-		return -EALREADY;
+		prefix_len = __builtin_popcount(ia.s_addr);
 
-	ap->server = l_dhcp_server_new(ifindex);
-	if (!ap->server) {
-		l_error("Failed to create DHCP server on %u", ifindex);
-		return -EINVAL;
+		if (ntohl(ia.s_addr) != util_netmask_from_prefix(prefix_len)) {
+			l_error("Invalid profile [IPv4].Netmask value");
+			goto done;
+		}
 	}
 
-	if (getenv("IWD_DHCP_DEBUG"))
-		l_dhcp_server_set_debug(ap->server, do_debug,
-							"[DHCPv4 SERV] ", NULL);
+	if (l_settings_has_key(config, "IPv4", "Gateway")) {
+		gateway_str = l_settings_get_string(config, "IPv4", "Gateway");
+		if (!gateway_str) {
+			l_error("Invalid profile [IPv4].Gateway value");
+			goto done;
+		}
+	}
 
-	/* Set the remaining DHCP options in config file */
-	if (!dhcp_load_settings(ap, settings))
-		return -EINVAL;
+	if (l_settings_get_value(config, "IPv4", "IPRange")) {
+		int i;
+		uint32_t netmask;
+		uint8_t tmp_len = prefix_len ?: AP_DEFAULT_IPV4_PREFIX_LEN;
 
-	if (!l_dhcp_server_set_ip_address(ap->server, addr))
-		return -EINVAL;
+		ip_range = l_settings_get_string_list(config, "IPv4",
+							"IPRange", ',');
 
-	ap->own_ip = l_steal_ptr(addr);
-	return ret;
-}
+		if (!static_addr) {
+			l_error("[IPv4].IPRange only makes sense in an AP "
+				"profile if a static local address has also "
+				"been specified");
+			goto done;
+		}
 
-static int ap_load_dhcp(struct ap_state *ap, const struct l_settings *config,
-			bool *wait_dhcp)
-{
-	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
-	int err = -EINVAL;
+		if (!ip_range || l_strv_length(ip_range) != 2) {
+			l_error("Can't parse the profile [IPv4].IPRange "
+				"setting as two address strings");
+			goto done;
+		}
 
-	if (!l_settings_has_group(config, "IPv4"))
-		return 0;
+		netmask = util_netmask_from_prefix(tmp_len);
 
-	err = ap_setup_dhcp(ap, config);
-	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);
+		for (i = 0; i < 2; i++) {
+			struct in_addr range_addr;
 
-		if (!ap->rtnl_add_cmd) {
-			l_error("Failed to add IPv4 address");
-			return -EIO;
+			if (inet_aton(ip_range[i], &range_addr) != 1) {
+				l_error("Can't parse address in "
+					"[IPv4].IPRange[%i]", i + 1);
+				goto done;
+			}
+
+			if ((static_addr ^ ntohl(range_addr.s_addr)) &
+					netmask) {
+				ia.s_addr = htonl(static_addr);
+				l_error("[IPv4].IPRange[%i] is not in the "
+					"%s/%i subnet", i + 1, inet_ntoa(ia),
+					tmp_len);
+				goto done;
+			}
 		}
+	}
 
-		ap->cleanup_ip = true;
+	if (l_settings_has_key(config, "IPv4", "DNSList")) {
+		dns_str_list = l_settings_get_string_list(config, "IPv4",
+								"DNSList", ',');
+		if (!dns_str_list || !*dns_str_list) {
+			l_error("Can't parse the profile [IPv4].DNSList "
+				"setting as a string list");
+			goto done;
+		}
+	}
 
-		*wait_dhcp = true;
-		err = 0;
-	/* Selected address already set, continue normally */
-	} else if (err == -EALREADY) {
-		*wait_dhcp = false;
-		err = 0;
+	if (l_settings_has_key(config, "IPv4", "LeaseTime")) {
+		if (!l_settings_get_uint(config, "IPv4", "LeaseTime",
+						&lease_time) ||
+				lease_time < 1) {
+			l_error("Error parsing [IPv4].LeaseTime as a positive "
+				"integer");
+			goto done;
+		}
 	}
 
-	return err;
+	ret = ap_setup_netconfig4(ap, (const char **) addr_str_list, prefix_len,
+					gateway_str, (const char **) ip_range,
+					(const char **) dns_str_list,
+					lease_time);
+
+done:
+	l_strv_free(addr_str_list);
+	l_free(gateway_str);
+	l_strv_free(ip_range);
+	l_strv_free(dns_str_list);
+	return ret;
 }
 
 static bool ap_load_psk(struct ap_state *ap, const struct l_settings *config)
@@ -2720,7 +2807,7 @@ static bool ap_load_psk(struct ap_state *ap, const struct l_settings *config)
 }
 
 static int ap_load_config(struct ap_state *ap, const struct l_settings *config,
-				bool *out_wait_dhcp, bool *out_cck_rates)
+				bool *out_cck_rates)
 {
 	size_t len;
 	L_AUTO_FREE_VAR(char *, strval) = NULL;
@@ -2746,11 +2833,12 @@ static int ap_load_config(struct ap_state *ap, const struct l_settings *config,
 		return -EINVAL;
 
 	/*
-	 * This loads DHCP settings either from the l_settings object or uses
-	 * the defaults. wait_on_address will be set true if an address change
-	 * is required.
+	 * This looks@the network configuration settings in @config and
+	 * relevant global settings and if it determines that netconfig is to
+	 * be enabled for the AP, it both creates the DHCP server object and
+	 * processes IP settings, applying the defaults where needed.
 	 */
-	err = ap_load_dhcp(ap, config, out_wait_dhcp);
+	err = ap_load_ipv4(ap, config);
 	if (err)
 		return err;
 
@@ -2866,15 +2954,15 @@ struct ap_state *ap_start(struct netdev *netdev, struct l_settings *config,
 	struct ap_state *ap;
 	struct wiphy *wiphy = netdev_get_wiphy(netdev);
 	uint64_t wdev_id = netdev_get_wdev_id(netdev);
-	int err = -EINVAL;
-	bool wait_on_address = false;
+	int err;
 	bool cck_rates = true;
 
-	if (err_out)
-		*err_out = err;
+	if (L_WARN_ON(!config)) {
+		if (err_out)
+			*err_out = -EINVAL;
 
-	if (L_WARN_ON(!config))
 		return NULL;
+	}
 
 	ap = l_new(struct ap_state, 1);
 	ap->nl80211 = l_genl_family_new(iwd_get_genl(), NL80211_GENL_NAME);
@@ -2882,7 +2970,7 @@ struct ap_state *ap_start(struct netdev *netdev, struct l_settings *config,
 	ap->ops = ops;
 	ap->user_data = user_data;
 
-	err = ap_load_config(ap, config, &wait_on_address, &cck_rates);
+	err = ap_load_config(ap, config, &cck_rates);
 	if (err)
 		goto error;
 
@@ -2948,9 +3036,20 @@ struct ap_state *ap_start(struct netdev *netdev, struct l_settings *config,
 	if (!ap->mlme_watch)
 		l_error("Registering for MLME notification failed");
 
-	if (wait_on_address) {
-		if (err_out)
-			*err_out = 0;
+	if (ap->netconfig_set_addr4) {
+		const char *broadcast_str =
+			broadcast_from_ip(ap->netconfig_addr4_str);
+
+		ap->rtnl_add_cmd = l_rtnl_ifaddr4_add(rtnl,
+						netdev_get_ifindex(netdev),
+						ap->netconfig_prefix_len4,
+						ap->netconfig_addr4_str,
+						broadcast_str,
+						ap_ifaddr4_added_cb, ap, NULL);
+		if (!ap->rtnl_add_cmd) {
+			l_error("Failed to add the IPv4 address");
+			goto error;
+		}
 
 		return ap;
 	}
@@ -3072,8 +3171,8 @@ 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);
+	if (ap->netconfig_dhcp)
+		l_dhcp_server_destroy(ap->netconfig_dhcp);
 	l_free(ap);
 }
 
-- 
2.27.0

[PATCH 5/9] ap: Refactor global address pool loading

[Andrew Zaborowski]

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

Deprecate the global [General].APRanges setting in favour of
[IPv4].APAddressPool with an extended (but backwards-compatible) syntax.
Drop the existing address pool creation code.

The new APAddressPool setting has the same syntax as the profile-local
[IPv4].Address setting and the subnet selection code will fall back
to the global setting if it's missing, this way we use common code to
handle both settings.
---
 src/ap.c | 180 +++++++++++++------------------------------------------
 1 file changed, 42 insertions(+), 138 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index a3dee5e4..587b908b 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -96,7 +96,6 @@ struct ap_state {
 	bool started : 1;
 	bool gtk_set : 1;
 	bool netconfig_set_addr4 : 1;
-	bool netconfig_use_ip_pool : 1;
 };
 
 struct sta_state {
@@ -127,97 +126,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_addr4_strs;
 static uint32_t netdev_watch;
 static 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 uint32_t ip_pool_get()
-{
-	uint32_t ip;
-	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 0;
-
-	l_uintset_put(pool.used, next_subnet);
-
-	ip = pool.start;
-	ip &= 0xffff00ff;
-	ip |= (next_subnet << 8);
-
-	return ip;
-}
-
-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;
@@ -328,12 +241,6 @@ static void ap_reset(struct ap_state *ap)
 		ap->netconfig_set_addr4 = false;
 	}
 
-	if (ap->netconfig_use_ip_pool) {
-		/* Release IP from pool if used */
-		ip_pool_put(ap->netconfig_addr4_str);
-		ap->netconfig_use_ip_pool = false;
-	}
-
 	l_free(ap->netconfig_addr4_str);
 	ap->netconfig_addr4_str = NULL;
 
@@ -2506,7 +2413,6 @@ static int ap_setup_netconfig4(struct ap_state *ap, const char **addr_str_list,
 	int ret;
 	struct in_addr ia;
 	struct l_dhcp_server *dhcp;
-	bool use_ip_pool = false;
 	bool r;
 
 	dhcp = l_dhcp_server_new(ifindex);
@@ -2539,24 +2445,11 @@ static int ap_setup_netconfig4(struct ap_state *ap, const char **addr_str_list,
 
 		ret = 0;
 	} else {
-		if (prefix_len) {
-			l_error("[IPv4].Netmask can't be used without an "
-				"address");
-			ret = -EINVAL;
-			goto done;
-		}
+		if (!prefix_len)
+			prefix_len = AP_DEFAULT_IPV4_PREFIX_LEN;
 
-		/* No config, no address set. Use IP pool */
-		addr = ip_pool_get();
-		if (addr) {
-			use_ip_pool = true;
-			prefix_len = pool.prefix;
-			ret = 0;
-		} else {
-			l_error("No more IP's in pool, cannot start AP on %u",
-					ifindex);
-			ret = -EEXIST;
-		}
+		ret = ap_select_addr4((const char **) global_addr4_strs,
+					prefix_len, ap->netdev, &addr);
 	}
 
 	if (ret)
@@ -2614,7 +2507,6 @@ static int ap_setup_netconfig4(struct ap_state *ap, const char **addr_str_list,
 	ap->netconfig_prefix_len4 = prefix_len;
 	ap->netconfig_set_addr4 = !addr_rec || addr_rec->addr != addr ||
 		addr_rec->prefix_len > prefix_len;
-	ap->netconfig_use_ip_pool = use_ip_pool;
 	ap->netconfig_dhcp = dhcp;
 	return 0;
 
@@ -2635,7 +2527,7 @@ static int ap_load_ipv4(struct ap_state *ap, const struct l_settings *config)
 	unsigned int lease_time = 0;
 	struct in_addr ia;
 
-	if (!l_settings_has_group(config, "IPv4") || !pool.used)
+	if (!l_settings_has_group(config, "IPv4") || !netconfig_enabled)
 		return 0;
 
 	if (l_settings_has_key(config, "IPv4", "Address")) {
@@ -3626,7 +3518,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);
 
@@ -3637,31 +3528,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_addr4_strs = l_settings_get_string_list(settings,
+								"IPv4",
+								"APAddressPool",
+								',');
+			if (!global_addr4_strs || !*global_addr4_strs) {
+				l_error("Can't parse the [IPv4].APAddressPool "
+					"setting as a string list");
+				l_strv_free(global_addr4_strs);
+				global_addr4_strs = NULL;
+			}
+		} else if (l_settings_get_value(settings,
+						"General", "APRanges")) {
+			global_addr4_strs = l_settings_get_string_list(settings,
+								"General",
+								"APRanges",
+								',');
+			if (!global_addr4_strs || !*global_addr4_strs) {
+				l_error("Can't parse the [General].APRanges "
+					"setting as a string list");
+				l_strv_free(global_addr4_strs);
+				global_addr4_strs = NULL;
+			}
 		}
 
-		if (!ip_pool_create(ip_prefix))
-			return -EINVAL;
+		/* Fall back to 192.168.0.0/16 */
+		if (!global_addr4_strs)
+			global_addr4_strs =
+				l_strv_append(NULL, "192.168.0.0/16");
 	}
 
 	rtnl = iwd_get_rtnl();
@@ -3674,7 +3578,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_addr4_strs);
 }
 
 IWD_MODULE(ap, ap_init, ap_exit)
-- 
2.27.0

[PATCH 6/9] ap: Move the DHCP server freeing to ap_reset

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 966 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 587b908b..cffb2906 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -244,8 +244,10 @@ static void ap_reset(struct ap_state *ap)
 	l_free(ap->netconfig_addr4_str);
 	ap->netconfig_addr4_str = NULL;
 
-	if (ap->netconfig_dhcp)
-		l_dhcp_server_stop(ap->netconfig_dhcp);
+	if (ap->netconfig_dhcp) {
+		l_dhcp_server_destroy(ap->netconfig_dhcp);
+		ap->netconfig_dhcp = NULL;
+	}
 }
 
 static void ap_del_station(struct sta_state *sta, uint16_t reason,
@@ -3063,8 +3065,6 @@ void ap_free(struct ap_state *ap)
 {
 	ap_reset(ap);
 	l_genl_family_free(ap->nl80211);
-	if (ap->netconfig_dhcp)
-		l_dhcp_server_destroy(ap->netconfig_dhcp);
 	l_free(ap);
 }
 
-- 
2.27.0

[PATCH 7/9] ap: Send a specific error message on async AP start failure

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3543 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 | 32 ++++++++++++++++++--------------
 src/ap.h |  4 ++++
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index cffb2906..edb280ae 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -2053,9 +2053,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);
 
@@ -2070,22 +2072,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->netconfig_dhcp && !l_dhcp_server_start(ap->netconfig_dhcp)) {
 		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)
@@ -2200,12 +2198,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,
@@ -2377,10 +2375,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;
@@ -3143,13 +3143,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 ff6b635d..2f1422ca 100644
--- a/src/ap.h
+++ b/src/ap.h
@@ -35,6 +35,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 *assoc_ies;
-- 
2.27.0

[PATCH 8/9] autotests: Update APRanges usage in testAP

[Andrew Zaborowski]

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

[PATCH 9/9] doc: Update AP settings in iwd.ap(5) and iwd.config(5)

[Andrew Zaborowski]

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

---
 src/iwd.ap.rst     | 31 ++++++++++++++++++++++++-------
 src/iwd.config.rst | 29 ++++++++++++++++++++++++++---
 2 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/src/iwd.ap.rst b/src/iwd.ap.rst
index 51622509..5fa23179 100644
--- a/src/iwd.ap.rst
+++ b/src/iwd.ap.rst
@@ -104,14 +104,31 @@ is desired, the group header line must still be present:
    :widths: 20 80
 
    * - Address
-     - Local IP address
+     - 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 provided this addresss will be set on the AP interface and any other
-       DHCP server options will be derived from it, unless they are overridden
-       by other settings below.  If *Address* is not provided and no IP
-       address is set on the interface prior to calling `StartProfile`,  the IP
-       pool defined by the global ``[General].APRanges`` setting will be used.
+       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
+       if not overridden by other settings below.
+
+       If a list of addresses and prefix lengths is specified (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 is based on the ``[IPv4].Netmask`` setting.
+
+       If *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 to 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 desired subnet gives
+       IWD a chance to avoid conflicts if other interfaces on the system use
+       dynamically assigned addresses.
 
    * - Gateway
      - IP Address of gateway
@@ -122,7 +139,7 @@ is desired, the group header line must still be present:
    * - Netmask
      - 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 as a comma-separated IP address list
diff --git a/src/iwd.config.rst b/src/iwd.config.rst
index 60f5b5db..78ec284e 100644
--- a/src/iwd.config.rst
+++ b/src/iwd.config.rst
@@ -67,8 +67,8 @@ 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 not override it.
 
        The network configuration feature is disabled by default.  See
        ``[Network]`` settings for additional settings related to network
@@ -182,7 +182,7 @@ The group ``[General]`` contains general settings.
        then setting ``DisableANQP`` to ``false`` is recommended.
 
 Network
----------
+-------
 
 The group ``[Network]`` contains network configuration related settings.
 
@@ -319,6 +319,29 @@ 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 network configuration.
+
+.. list-table::
+   :header-rows: 0
+   :stub-columns: 0
+   :widths: 20 80
+   :align: left
+
+   * - APAddressPool
+     - Values: comma-separated list of prefix-notation IP 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

Re: [PATCH 8/9] autotests: Update APRanges usage in testAP

[James Prestwood]

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

Hi Andrew,

On Thu, 2021-05-13 at 20:47 +0200, Andrew Zaborowski wrote:
> [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"

Not really a huge fan of this try/except. Why is it not deterministic
which IP each station will have? I'm probably missing something with
the previous patches but with the new APAddressPool you set you should
have a range of .80.1 to .80.31 right? So why wouldn't the DHCP server
just choose 80.1 and 80.2 still?

>  
>              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()

Thanks,
James

Re: [PATCH 8/9] autotests: Update APRanges usage in testAP

[Andrew Zaborowski]

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

Hi James,

On Thu, 13 May 2021 at 21:05, James Prestwood <prestwoj@gmail.com> wrote:
> On Thu, 2021-05-13 at 20:47 +0200, Andrew Zaborowski wrote:
> > [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"
>
> Not really a huge fan of this try/except. Why is it not deterministic
> which IP each station will have?

The new code selects a random subnet from the configured space on
purpose.  We can retrieve the actual address here and compare the 31
bits of it that we want to validate but it was easier to reuse the
testutil.test_ip_address_match API, I'll change this though if I have
to resend this another time.

> I'm probably missing something with
> the previous patches but with the new APAddressPool you set you should
> have a range of .80.1 to .80.31 right? So why wouldn't the DHCP server
> just choose 80.1 and 80.2 still?

The default subnet size is 16 addresses now so the code should select
one of the two subnets that fit in the .80.0 - .80.31 range and we
don't know which one it's going to be.

Best regards

Re: [PATCH 1/9] ap: Make rtnl global static

[Denis Kenzior]

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

Hi Andrew,

On 5/13/21 1:47 PM, Andrew Zaborowski wrote:
> ---
>   src/ap.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Applied, thanks.

Re: [PATCH 2/9] ap: Track IPv4 addresses in use

[Denis Kenzior]

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

Hi Andrew,

On 5/13/21 1:47 PM, Andrew Zaborowski wrote:
> Add the ap_addr submodule that tracks IPv4 addresses in use on the
> system for use when selecting the address for a new AP.
> ---
>   Makefile.am   |   3 +-
>   src/ap-addr.c | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/ap.h      |   9 +++
>   3 files changed, 177 insertions(+), 1 deletion(-)
>   create mode 100644 src/ap-addr.c

Can we name it ippool or ipv4pool?

> diff --git a/src/ap-addr.c b/src/ap-addr.c
> new file mode 100644
> index 00000000..238cc7b5
> --- /dev/null
> +++ b/src/ap-addr.c
> @@ -0,0 +1,166 @@
> +/*
> + *
> + *  Wireless daemon for Linux
> + *
> + *  Copyright (C) 2021  Intel Corporation. All rights reserved.
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <errno.h>
> +#include <netinet/in.h>
> +#include <arpa/inet.h>
> +
> +#include <ell/ell.h>
> +
> +#include "src/util.h"
> +#include "src/iwd.h"
> +#include "src/module.h"
> +#include "src/netdev.h"

Why does it need netdev or mpdu?

> +#include "src/mpdu.h"
> +#include "src/ap.h"

Or ap?

> +
> +static struct l_queue *used_addr4_list;
> +static struct l_netlink *rtnl;
> +
> +static bool ap_rtnl_addr4_match_ifindex(const void *a, const void *b)
> +{
> +	const struct ap_rtnl_addr4_record *addr = a;
> +
> +	return addr->ifindex == L_PTR_TO_UINT(b) && !addr->secondary;
> +}
> +
> +const struct ap_rtnl_addr4_record *ap_get_addr4(struct netdev *netdev)

I would think looking up by ifindex would be just as good?

> +{
> +	uint32_t ifindex = netdev ? netdev_get_ifindex(netdev) : 0;

?? 0 ?

> +
> +	return l_queue_find(used_addr4_list, ap_rtnl_addr4_match_ifindex,
> +				L_UINT_TO_PTR(ifindex));
> +}
> +
> +static bool ap_rtnl_addr4_match_free(void *data, void *user_data)
> +{
> +	const struct ap_rtnl_addr4_record *a = data;
> +	const struct ap_rtnl_addr4_record *b = user_data;
> +
> +	if (a->ifindex != b->ifindex || a->addr != b->addr ||
> +			a->prefix_len != b->prefix_len)
> +		return false;
> +
> +	l_free(data);
> +	return true;
> +}
> +
> +static void ap_addr_notify(uint16_t type, const void *data, uint32_t len,
> +				void *user_data)
> +{
> +	const struct ifaddrmsg *ifa = data;
> +	char *ip_str = NULL;
> +	int r;
> +	struct in_addr ip;
> +
> +	if (ifa->ifa_family != AF_INET || ifa->ifa_prefixlen < 1)
> +		return;
> +
> +	len -= NLMSG_ALIGN(sizeof(struct ifaddrmsg));
> +
> +	l_rtnl_ifaddr4_extract(ifa, len, NULL, &ip_str, NULL);
> +	if (!ip_str)
> +		return;
> +
> +	r = inet_aton(ip_str, &ip);
> +	l_free(ip_str);
> +
> +	if (r < 0)
> +		return;
> +
> +	if (type == RTM_NEWADDR) {
> +		struct ap_rtnl_addr4_record *addr;
> +
> +		addr = l_new(struct ap_rtnl_addr4_record, 1);
> +		addr->addr = ntohl(ip.s_addr);
> +		addr->prefix_len = ifa->ifa_prefixlen;
> +		addr->ifindex = ifa->ifa_index;
> +		addr->secondary = !!(ifa->ifa_flags & IFA_F_SECONDARY);

so why is l_rtnl_address not good enough?

> +
> +		if (!used_addr4_list)
> +			used_addr4_list = l_queue_new();

I don't think lazy initialization makes any sense in this case.  Just move it to 
the init function.

> +
> +		l_queue_push_tail(used_addr4_list, addr);
> +	} else if (type == RTM_DELADDR) {
> +		struct ap_rtnl_addr4_record addr;
> +
> +		addr.addr = ntohl(ip.s_addr);
> +		addr.prefix_len = ifa->ifa_prefixlen;
> +		addr.ifindex = ifa->ifa_index;
> +
> +		l_queue_foreach_remove(used_addr4_list,
> +					ap_rtnl_addr4_match_free, &addr);
> +	}
> +}
> +
> +static void ap_addr4_dump_cb(int error,
> +				uint16_t type, const void *data,
> +				uint32_t len, void *user_data)
> +{
> +	if (error) {
> +		l_error("addr4_dump_cb: %s (%i)", strerror(-error), -error);
> +		return;
> +	}
> +
> +	ap_addr_notify(type, data, len, user_data);
> +}
> +
> +static int ap_addr_init(void)
> +{
> +	const struct l_settings *settings = iwd_get_config();
> +	bool netconfig_enabled;
> +
> +	if (!l_settings_get_bool(settings, "General",
> +				"EnableNetworkConfiguration",
> +				&netconfig_enabled))
> +		netconfig_enabled = false;
> +
> +	if (!netconfig_enabled)
> +		return 0;
> +
> +	rtnl = iwd_get_rtnl();
> +
> +	if (!l_netlink_register(rtnl, RTNLGRP_IPV4_IFADDR,
> +				ap_addr_notify, NULL, NULL)) {
> +		l_error("Failed to register for RTNL link notifications");
> +		return -EIO;
> +	}
> +
> +	if (!l_rtnl_ifaddr4_dump(rtnl, ap_addr4_dump_cb, NULL, NULL)) {
> +		l_error("Sending the IPv4 addr dump req failed");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static void ap_addr_exit(void)
> +{
> +	l_queue_destroy(used_addr4_list, l_free);
> +	used_addr4_list = NULL;
> +}
> +
> +IWD_MODULE(ap_addr, ap_addr_init, ap_addr_exit)
> diff --git a/src/ap.h b/src/ap.h
> index b0100f34..12baca8b 100644
> --- a/src/ap.h
> +++ b/src/ap.h
> @@ -97,3 +97,12 @@ bool ap_station_disconnect(struct ap_state *ap, const uint8_t *mac,
>   
>   bool ap_push_button(struct ap_state *ap);
>   void ap_update_beacon(struct ap_state *ap);
> +
> +struct ap_rtnl_addr4_record {
> +	uint32_t addr;
> +	uint8_t prefix_len;
> +	uint32_t ifindex;
> +	bool secondary;
> +};
> +
> +const struct ap_rtnl_addr4_record *ap_get_addr4(struct netdev *netdev);
> 

Regards,
-Denis

Re: [PATCH 2/9] ap: Track IPv4 addresses in use

[Andrew Zaborowski]

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

Hi Denis,

On Fri, 14 May 2021 at 17:26, Denis Kenzior <denkenz@gmail.com> wrote:
> On 5/13/21 1:47 PM, Andrew Zaborowski wrote:
> > Add the ap_addr submodule that tracks IPv4 addresses in use on the
> > system for use when selecting the address for a new AP.
> > ---
> >   Makefile.am   |   3 +-
> >   src/ap-addr.c | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   src/ap.h      |   9 +++
> >   3 files changed, 177 insertions(+), 1 deletion(-)
> >   create mode 100644 src/ap-addr.c
>
> Can we name it ippool or ipv4pool?

Ok, I assume without the ap prefix either.

I'll go for ippool so we don't have to create a new file if we want to
add IPv6 versions.

>
> > diff --git a/src/ap-addr.c b/src/ap-addr.c
> > new file mode 100644
> > index 00000000..238cc7b5
> > --- /dev/null
> > +++ b/src/ap-addr.c
> > @@ -0,0 +1,166 @@
> > +/*
> > + *
> > + *  Wireless daemon for Linux
> > + *
> > + *  Copyright (C) 2021  Intel Corporation. All rights reserved.
> > + *
> > + *  This library is free software; you can redistribute it and/or
> > + *  modify it under the terms of the GNU Lesser General Public
> > + *  License as published by the Free Software Foundation; either
> > + *  version 2.1 of the License, or (at your option) any later version.
> > + *
> > + *  This library is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + *  Lesser General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU Lesser General Public
> > + *  License along with this library; if not, write to the Free Software
> > + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > + *
> > + */
> > +
> > +#ifdef HAVE_CONFIG_H
> > +#include <config.h>
> > +#endif
> > +
> > +#include <errno.h>
> > +#include <netinet/in.h>
> > +#include <arpa/inet.h>
> > +
> > +#include <ell/ell.h>
> > +
> > +#include "src/util.h"
> > +#include "src/iwd.h"
> > +#include "src/module.h"
> > +#include "src/netdev.h"
>
> Why does it need netdev or mpdu?

Both were required for ap.h.  I guess it's better to get around it by
adding the missing declarations to ap.h itself, will do that.

>
> > +#include "src/mpdu.h"
> > +#include "src/ap.h"
>
> Or ap?

Because we define the non-static functions exported by ap-addr.c in
ap.h.  If we're renaming it ippool.c, then ap.h isn't going to be
needed.

>
> > +
> > +static struct l_queue *used_addr4_list;
> > +static struct l_netlink *rtnl;
> > +
> > +static bool ap_rtnl_addr4_match_ifindex(const void *a, const void *b)
> > +{
> > +     const struct ap_rtnl_addr4_record *addr = a;
> > +
> > +     return addr->ifindex == L_PTR_TO_UINT(b) && !addr->secondary;
> > +}
> > +
> > +const struct ap_rtnl_addr4_record *ap_get_addr4(struct netdev *netdev)
>
> I would think looking up by ifindex would be just as good?

Yep, that's what the previous versions did but since we more often
manage netdevs it made for a nicer API to receive the netdev.

>
> > +{
> > +     uint32_t ifindex = netdev ? netdev_get_ifindex(netdev) : 0;
>
> ?? 0 ?

Yep.  No netdev --> not excluding any addresses from the conflict
checks based on the target interface.

>
> > +
> > +     return l_queue_find(used_addr4_list, ap_rtnl_addr4_match_ifindex,
> > +                             L_UINT_TO_PTR(ifindex));
> > +}
> > +
> > +static bool ap_rtnl_addr4_match_free(void *data, void *user_data)
> > +{
> > +     const struct ap_rtnl_addr4_record *a = data;
> > +     const struct ap_rtnl_addr4_record *b = user_data;
> > +
> > +     if (a->ifindex != b->ifindex || a->addr != b->addr ||
> > +                     a->prefix_len != b->prefix_len)
> > +             return false;
> > +
> > +     l_free(data);
> > +     return true;
> > +}
> > +
> > +static void ap_addr_notify(uint16_t type, const void *data, uint32_t len,
> > +                             void *user_data)
> > +{
> > +     const struct ifaddrmsg *ifa = data;
> > +     char *ip_str = NULL;
> > +     int r;
> > +     struct in_addr ip;
> > +
> > +     if (ifa->ifa_family != AF_INET || ifa->ifa_prefixlen < 1)
> > +             return;
> > +
> > +     len -= NLMSG_ALIGN(sizeof(struct ifaddrmsg));
> > +
> > +     l_rtnl_ifaddr4_extract(ifa, len, NULL, &ip_str, NULL);
> > +     if (!ip_str)
> > +             return;
> > +
> > +     r = inet_aton(ip_str, &ip);
> > +     l_free(ip_str);
> > +
> > +     if (r < 0)
> > +             return;
> > +
> > +     if (type == RTM_NEWADDR) {
> > +             struct ap_rtnl_addr4_record *addr;
> > +
> > +             addr = l_new(struct ap_rtnl_addr4_record, 1);
> > +             addr->addr = ntohl(ip.s_addr);
> > +             addr->prefix_len = ifa->ifa_prefixlen;
> > +             addr->ifindex = ifa->ifa_index;
> > +             addr->secondary = !!(ifa->ifa_flags & IFA_F_SECONDARY);
>
> so why is l_rtnl_address not good enough?

1. Seems this is internal to ell/rtnl.c, 2. stores way more
information than we need and sort-of implies network byte order which
I didn't want to have to deal with (reduces readability too).

>
> > +
> > +             if (!used_addr4_list)
> > +                     used_addr4_list = l_queue_new();
>
> I don't think lazy initialization makes any sense in this case.  Just move it to
> the init function.

Ok, good point.

>
> > +
> > +             l_queue_push_tail(used_addr4_list, addr);
> > +     } else if (type == RTM_DELADDR) {
> > +             struct ap_rtnl_addr4_record addr;
> > +
> > +             addr.addr = ntohl(ip.s_addr);
> > +             addr.prefix_len = ifa->ifa_prefixlen;
> > +             addr.ifindex = ifa->ifa_index;
> > +
> > +             l_queue_foreach_remove(used_addr4_list,
> > +                                     ap_rtnl_addr4_match_free, &addr);
> > +     }
> > +}
> > +
> > +static void ap_addr4_dump_cb(int error,
> > +                             uint16_t type, const void *data,
> > +                             uint32_t len, void *user_data)
> > +{
> > +     if (error) {
> > +             l_error("addr4_dump_cb: %s (%i)", strerror(-error), -error);
> > +             return;
> > +     }
> > +
> > +     ap_addr_notify(type, data, len, user_data);
> > +}
> > +
> > +static int ap_addr_init(void)
> > +{
> > +     const struct l_settings *settings = iwd_get_config();
> > +     bool netconfig_enabled;
> > +
> > +     if (!l_settings_get_bool(settings, "General",
> > +                             "EnableNetworkConfiguration",
> > +                             &netconfig_enabled))
> > +             netconfig_enabled = false;
> > +
> > +     if (!netconfig_enabled)
> > +             return 0;
> > +
> > +     rtnl = iwd_get_rtnl();
> > +
> > +     if (!l_netlink_register(rtnl, RTNLGRP_IPV4_IFADDR,
> > +                             ap_addr_notify, NULL, NULL)) {
> > +             l_error("Failed to register for RTNL link notifications");
> > +             return -EIO;
> > +     }
> > +
> > +     if (!l_rtnl_ifaddr4_dump(rtnl, ap_addr4_dump_cb, NULL, NULL)) {
> > +             l_error("Sending the IPv4 addr dump req failed");
> > +             return -EIO;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void ap_addr_exit(void)
> > +{
> > +     l_queue_destroy(used_addr4_list, l_free);
> > +     used_addr4_list = NULL;
> > +}
> > +
> > +IWD_MODULE(ap_addr, ap_addr_init, ap_addr_exit)
> > diff --git a/src/ap.h b/src/ap.h
> > index b0100f34..12baca8b 100644
> > --- a/src/ap.h
> > +++ b/src/ap.h
> > @@ -97,3 +97,12 @@ bool ap_station_disconnect(struct ap_state *ap, const uint8_t *mac,
> >
> >   bool ap_push_button(struct ap_state *ap);
> >   void ap_update_beacon(struct ap_state *ap);
> > +
> > +struct ap_rtnl_addr4_record {
> > +     uint32_t addr;
> > +     uint8_t prefix_len;
> > +     uint32_t ifindex;
> > +     bool secondary;
> > +};
> > +
> > +const struct ap_rtnl_addr4_record *ap_get_addr4(struct netdev *netdev);
> >
>
> Regards,
> -Denis
> _______________________________________________
> iwd mailing list -- iwd(a)lists.01.org
> To unsubscribe send an email to iwd-leave(a)lists.01.org

Re: [PATCH 2/9] ap: Track IPv4 addresses in use

[Andrew Zaborowski]

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

On Sat, 15 May 2021 at 00:16, Andrew Zaborowski
<andrew.zaborowski@intel.com> wrote:
> On Fri, 14 May 2021 at 17:26, Denis Kenzior <denkenz@gmail.com> wrote:
> > On 5/13/21 1:47 PM, Andrew Zaborowski wrote:
> > > +             addr = l_new(struct ap_rtnl_addr4_record, 1);
> > > +             addr->addr = ntohl(ip.s_addr);
> > > +             addr->prefix_len = ifa->ifa_prefixlen;
> > > +             addr->ifindex = ifa->ifa_index;
> > > +             addr->secondary = !!(ifa->ifa_flags & IFA_F_SECONDARY);
> >
> > so why is l_rtnl_address not good enough?
>
> 1. Seems this is internal to ell/rtnl.c,

Ok, I see what you mean.  We can do this but I wouldn't see it as an
improvement because of the below and simply the extra indirection.

> 2. stores way more
> information than we need and sort-of implies network byte order which
> I didn't want to have to deal with (reduces readability too).

Best regards