[PATCH 01/11] ap: Move sending CMD_START_AP to common function

[PATCH 01/11] ap: Move sending CMD_START_AP to common function

[Andrew Zaborowski]

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

---
 src/ap.c | 59 +++++++++++++++++++++++++++-----------------------------
 1 file changed, 28 insertions(+), 31 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index a0420ed1..8851cb85 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -2260,34 +2260,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,
@@ -2858,7 +2865,6 @@ struct ap_state *ap_start(struct netdev *netdev, struct l_settings *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;
@@ -2949,22 +2955,13 @@ struct ap_state *ap_start(struct netdev *netdev, struct l_settings *config,
 		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;
-- 
2.27.0

[PATCH 02/11] ap: Refactor DHCP settings loading

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 14984 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 without selecting a specific subnet at
this stage because we will need to query the addresses already in use
before this.  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.
---
 src/ap.c | 346 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 165 insertions(+), 181 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 8851cb85..db2a2c04 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -88,15 +88,14 @@ struct ap_state {
 	uint16_t last_aid;
 	struct l_queue *sta_states;
 
-	struct l_dhcp_server *server;
+	struct l_dhcp_server *netconfig_dhcp;
+	uint8_t netconfig_prefix_len4;
+	char **netconfig_addr4_str_list;
 	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 enable_netconfig4 : 1;
 };
 
 struct sta_state {
@@ -321,22 +320,12 @@ static void ap_reset(struct ap_state *ap)
 	ap->started = false;
 
 	/* Delete IP if one was set by IWD */
-	if (ap->cleanup_ip)
-		l_rtnl_ifaddr4_delete(rtnl, netdev_get_ifindex(netdev),
-					ap->ip_prefix, ap->own_ip,
-					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);
 
-		l_free(ap->own_ip);
-	}
+	if (ap->netconfig_dhcp)
+		l_dhcp_server_stop(ap->netconfig_dhcp);
 
-	if (ap->server)
-		l_dhcp_server_stop(ap->server);
+	l_strv_free(ap->netconfig_addr4_str_list);
+	ap->netconfig_addr4_str_list = NULL;
 }
 
 static void ap_del_station(struct sta_state *sta, uint16_t reason,
@@ -2163,7 +2152,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 +2479,185 @@ 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_load_ipv4(struct ap_state *ap,
+				const struct l_settings *config)
 {
-	struct l_dhcp_server *server = ap->server;
+	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
+	uint32_t static_addr4 = 0;
 	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,
-							"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;
-
-	if (!l_settings_get_uint(settings, "IPv4", "LeaseTime", &lease_time))
-		lease_time = 0;
-
-	if (gateway && !l_dhcp_server_set_gateway(server, gateway)) {
-		l_error("[IPv4].Gateway value error");
-		goto error;
-	}
+	if (!l_settings_has_group(config, "IPv4") || !pool.used)
+		return 0;
 
-	if (dns && !l_dhcp_server_set_dns(server, dns)) {
-		l_error("[IPv4].DNSList value error");
-		goto error;
-	}
+	ap->enable_netconfig4 = true;
 
-	if (netmask && !l_dhcp_server_set_netmask(server, netmask)) {
-		l_error("[IPv4].Netmask value error");
-		goto error;
-	}
+	if (l_settings_has_key(config, "IPv4", "Netmask")) {
+		L_AUTO_FREE_VAR(char *, netmask_str) =
+			l_settings_get_string(config, "IPv4", "Netmask");
 
-	if (ip_range) {
-		if (l_strv_length(ip_range) != 2) {
-			l_error("Two addresses expected in [IPv4].IPRange");
-			goto error;
+		if (inet_pton(AF_INET, netmask_str, &ia) != 1) {
+			l_error("Can't parse the profile [IPv4].Netmask "
+				"setting");
+			return -EINVAL;
 		}
 
-		if (!l_dhcp_server_set_ip_range(server, ip_range[0],
-							ip_range[1])) {
-			l_error("Error setting IP range from [IPv4].IPRange");
-			goto error;
+		ap->netconfig_prefix_len4 = __builtin_popcount(ia.s_addr);
+
+		if (ntohl(ia.s_addr) != util_netmask_from_prefix(
+						ap->netconfig_prefix_len4)) {
+			l_error("Invalid profile [IPv4].Netmask value");
+			return -EINVAL;
+		}
+	} else
+		ap->netconfig_prefix_len4 = AP_DEFAULT_IPV4_PREFIX_LEN;
+
+	if (l_settings_has_key(config, "IPv4", "Address")) {
+		char **str_list = l_settings_get_string_list(config, "IPv4",
+								"Address", ',');
+
+		if (!str_list || !*str_list) {
+			l_error("Can't parse the profile [IPv4].Address "
+				"setting as a string list");
+			l_free(str_list);
+			return -EINVAL;
 		}
+
+		ap->netconfig_addr4_str_list = str_list;
+
+		/* Check for the static IP syntax: Address=<IP> */
+		if (l_strv_length(str_list) == 1 &&
+				inet_pton(AF_INET, *str_list, &ia) == 1)
+			static_addr4 = ntohl(ia.s_addr);
 	}
 
-	if (lease_time && !l_dhcp_server_set_lease_time(server, lease_time)) {
-		l_error("[IPv4].LeaseTime value error");
-		goto error;
+	ap->netconfig_dhcp = l_dhcp_server_new(ifindex);
+	if (!ap->netconfig_dhcp) {
+		l_error("Failed to create DHCP server on ifindex %u", ifindex);
+		return -EINVAL;
 	}
 
-	if (netmask && inet_pton(AF_INET, netmask, &ia) > 0)
-		ap->ip_prefix = __builtin_popcountl(ia.s_addr);
-	else
-		ap->ip_prefix = 24;
+	if (getenv("IWD_DHCP_DEBUG"))
+		l_dhcp_server_set_debug(ap->netconfig_dhcp, do_debug,
+					"[DHCPv4 SERV] ", NULL);
 
-	ret = true;
+	ia.s_addr = htonl(util_netmask_from_prefix(ap->netconfig_prefix_len4));
 
-error:
-	l_strv_free(dns);
-	l_strv_free(ip_range);
-	return ret;
-}
+	if (!l_dhcp_server_set_netmask(ap->netconfig_dhcp, inet_ntoa(ia))) {
+		l_error("l_dhcp_server_set_netmask failed");
+		return -EINVAL;
+	}
 
-/*
- * 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)
-{
-	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;
+	if (l_settings_has_key(config, "IPv4", "Gateway")) {
+		L_AUTO_FREE_VAR(char *, gateway_str) =
+			l_settings_get_string(config, "IPv4", "Gateway");
+
+		if (!l_dhcp_server_set_gateway(ap->netconfig_dhcp,
+						gateway_str)) {
+			l_error("l_dhcp_server_set_gateway failed");
+			return -EINVAL;
+		}
+	}
 
-	/* 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_get_value(config, "IPv4", "IPRange")) {
+		char **ip_range = l_settings_get_string_list(config, "IPv4",
+								"IPRange", ',');
+		int i;
+		uint32_t netmask;
+		bool r;
 
-	addr = l_settings_get_string(settings, "IPv4", "Address");
-	if (addr) {
-		if (inet_pton(AF_INET, addr, &ia) < 0)
+		if (!static_addr4 || !l_settings_has_key(config,
+							"IPv4", "Netmask")) {
+			l_error("[IPv4].IPRange only makes sense in an AP "
+				"profile if a static local address and "
+				"netmask have also been specified");
+			l_strv_free(ip_range);
 			return -EINVAL;
+		}
 
-		/* 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;
+		if (!ip_range || l_strv_length(ip_range) != 2) {
+			l_error("Can't parse the profile [IPv4].IPRange "
+				"setting as two address strings");
+			l_strv_free(ip_range);
+			return -EINVAL;
 		}
 
-		ap->use_ip_pool = true;
-		ap->ip_prefix = pool.prefix;
-		ret = 0;
-	} else
-		return -EALREADY;
+		netmask = util_netmask_from_prefix(ap->netconfig_prefix_len4);
 
-	ap->server = l_dhcp_server_new(ifindex);
-	if (!ap->server) {
-		l_error("Failed to create DHCP server on %u", ifindex);
-		return -EINVAL;
-	}
+		for (i = 0; i < 2; i++) {
+			struct in_addr range_addr;
 
-	if (getenv("IWD_DHCP_DEBUG"))
-		l_dhcp_server_set_debug(ap->server, do_debug,
-							"[DHCPv4 SERV] ", NULL);
+			if (inet_aton(ip_range[i], &range_addr) != 1) {
+				l_error("Can't parse address in "
+					"[IPv4].IPRange[%i]", i + 1);
+				break;
+			}
 
-	/* Set the remaining DHCP options in config file */
-	if (!dhcp_load_settings(ap, settings))
-		return -EINVAL;
+			if ((static_addr4 ^ ntohl(range_addr.s_addr)) &
+					netmask) {
+				struct in_addr addr = { htonl(static_addr4) };
 
-	if (!l_dhcp_server_set_ip_address(ap->server, addr))
-		return -EINVAL;
+				l_error("[IPv4].IPRange[%i] is not in the "
+					"%s/%i subnet", i + 1, inet_ntoa(addr),
+					ap->netconfig_prefix_len4);
+				break;
+			}
+		}
 
-	ap->own_ip = l_steal_ptr(addr);
-	return ret;
-}
+		if (i < 2) {
+			l_strv_free(ip_range);
+			return -EINVAL;
+		}
 
-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;
+		r = l_dhcp_server_set_ip_range(ap->netconfig_dhcp,
+						ip_range[0], ip_range[1]);
+		l_strv_free(ip_range);
 
-	if (!l_settings_has_group(config, "IPv4"))
-		return 0;
+		if (!r) {
+			l_error("l_dhcp_server_set_ip_range failed");
+			return -EINVAL;
+		}
+	}
+
+	if (l_settings_has_key(config, "IPv4", "DNSList")) {
+		char **str_list = l_settings_get_string_list(config, "IPv4",
+								"DNSList", ',');
+		bool r;
 
-	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);
-
-		if (!ap->rtnl_add_cmd) {
-			l_error("Failed to add IPv4 address");
-			return -EIO;
+		if (!str_list || !*str_list) {
+			l_error("Can't parse the profile [IPv4].DNSList "
+				"setting as a string list");
+			l_free(str_list);
+			return -EINVAL;
 		}
 
-		ap->cleanup_ip = true;
+		r = l_dhcp_server_set_dns(ap->netconfig_dhcp, str_list);
+		l_strv_free(str_list);
 
-		*wait_dhcp = true;
-		err = 0;
-	/* Selected address already set, continue normally */
-	} else if (err == -EALREADY) {
-		*wait_dhcp = false;
-		err = 0;
+		if (!r) {
+			l_error("l_dhcp_server_set_dns failed");
+			return -EINVAL;
+		}
 	}
 
-	return err;
+	if (l_settings_has_key(config, "IPv4", "LeaseTime")) {
+		unsigned int lease_time;
+
+		if (!l_settings_get_uint(config, "IPv4", "LeaseTime",
+						&lease_time)) {
+			l_error("Error parsing [IPv4].LeaseTime as an integer");
+			return -EINVAL;
+		}
+
+		if (!l_dhcp_server_set_lease_time(ap->netconfig_dhcp,
+							lease_time)) {
+			l_error("l_dhcp_server_set_lease_time failed");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
 }
 
 static bool ap_load_psk(struct ap_state *ap, const struct l_settings *config)
@@ -2720,7 +2712,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 +2738,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 at 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,12 +2859,10 @@ 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;
 	bool cck_rates = true;
 
 	if (err_out)
-		*err_out = err;
+		*err_out = -EINVAL;
 
 	if (L_WARN_ON(!config))
 		return NULL;
@@ -2882,12 +2873,9 @@ 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);
-	if (err)
+	if (ap_load_config(ap, config, &cck_rates) < 0)
 		goto error;
 
-	err = -EINVAL;
-
 	/* TODO: Add all ciphers supported by wiphy */
 	ap->ciphers = wiphy_select_cipher(wiphy, 0xffff);
 	ap->group_cipher = wiphy_select_cipher(wiphy, 0xffff);
@@ -2948,9 +2936,8 @@ 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->enable_netconfig4) {
+		/* TODO: select an IP address, set it and call ap_start_send */
 
 		return ap;
 	}
@@ -2963,9 +2950,6 @@ struct ap_state *ap_start(struct netdev *netdev, struct l_settings *config,
 	}
 
 error:
-	if (err_out)
-		*err_out = err;
-
 	ap_reset(ap);
 	l_genl_family_free(ap->nl80211);
 	l_free(ap);
@@ -3072,8 +3056,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 03/11] ap: Refactor global address pool loading

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 6219 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.  [IPv4].Address is loaded as
ap->netconfig_addr4_str_list the subnet selection code will fall back
to the global setting, this way we'll have common code to handle both
settings.
---
 src/ap.c | 169 +++++++++++++------------------------------------------
 1 file changed, 38 insertions(+), 131 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index db2a2c04..872c0a1f 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -126,116 +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_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;
-	uint32_t bcast;
-
-	if (inet_aton(ip, &ia) != 1)
-		return NULL;
-
-	bcast = ntohl(ia.s_addr);
-	bcast &= 0xffffff00;
-	bcast |= 0x000000ff;
-
-	ia.s_addr = htonl(bcast);
-
-	return inet_ntoa(ia);
-}
-
 static void ap_stop_handshake(struct sta_state *sta)
 {
 	if (sta->sm) {
@@ -2488,7 +2383,7 @@ static int ap_load_ipv4(struct ap_state *ap,
 	uint32_t static_addr4 = 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;
 
 	ap->enable_netconfig4 = true;
@@ -3511,7 +3406,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);
 
@@ -3522,31 +3416,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();
@@ -3559,7 +3466,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

[PATCH 04/11] ap: Add subnet address selection logic

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 8962 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 from a second list.
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.)

This is added as src/ap-addr.c so that it's self-contained.
---
 Makefile.am   |   3 +-
 src/ap-addr.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/ap.h      |   9 ++
 3 files changed, 242 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..5d911956
--- /dev/null
+++ b/src/ap-addr.c
@@ -0,0 +1,231 @@
+/*
+ *
+ *  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/mpdu.h"
+#include "src/wscutil.h"
+#include "src/netdev.h"
+#include "src/ap.h"
+
+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 ? 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,
+			const struct l_queue *used_addr_dump,
+			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;
+	int err = -EINVAL;
+
+	/* Build a sorted list of used/unavailable subnets */
+	for (entry = l_queue_get_entries((struct l_queue *) used_addr_dump);
+			entry; entry = entry->next) {
+		const struct ap_rtnl_addr4_record *rec = entry->data;
+		struct ap_ip_range *range;
+
+		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;
+}
diff --git a/src/ap.h b/src/ap.h
index b0100f34..2163d2db 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;
+};
+
+int ap_select_addr4(const char **addr_str_list, uint8_t prefix,
+			const struct l_queue *used_addr_dump,
+			uint32_t *out_addr);
-- 
2.27.0

[PATCH 05/11] ap: Dump addreses in use and assign local IP

[Andrew Zaborowski]

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

During AP start dump addresses in use by all netdevs, then handle the
local and global subnet address settings to select the final subnet and
IP and set that on the interface if not already set.

Restore the IP cleanup after stopping AP, removed temporarily in an
earlier commit.
---
 src/ap.c | 182 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 178 insertions(+), 4 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 872c0a1f..37b71a6f 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -89,12 +89,16 @@ struct ap_state {
 	struct l_queue *sta_states;
 
 	struct l_dhcp_server *netconfig_dhcp;
+	uint32_t netconfig_addr4;
 	uint8_t netconfig_prefix_len4;
 	char **netconfig_addr4_str_list;
+	struct l_queue *netconfig_addr4_dump;
 	uint32_t rtnl_add_cmd;
+	uint32_t rtnl_dump_cmd;
 
 	bool started : 1;
 	bool gtk_set : 1;
+	bool cleanup_addr4 : 1;
 	bool enable_netconfig4 : 1;
 };
 
@@ -205,6 +209,13 @@ static void ap_reset(struct ap_state *ap)
 	if (ap->rtnl_add_cmd)
 		l_netlink_cancel(rtnl, ap->rtnl_add_cmd);
 
+	if (ap->rtnl_dump_cmd) {
+		uint32_t cmd = ap->rtnl_dump_cmd;
+
+		ap->rtnl_dump_cmd = 0;
+		l_netlink_cancel(rtnl, cmd);
+	}
+
 	l_queue_destroy(ap->sta_states, ap_sta_free);
 
 	if (ap->rates)
@@ -215,10 +226,29 @@ static void ap_reset(struct ap_state *ap)
 	ap->started = false;
 
 	/* Delete IP if one was set by IWD */
+	if (ap->cleanup_addr4) {
+		struct in_addr ia;
+		char ip_str[16];
+		uint32_t broadcast = ap->netconfig_addr4 |
+			~util_netmask_from_prefix(ap->netconfig_prefix_len4);
+		char broadcast_str[16];
+
+		ia.s_addr = htonl(ap->netconfig_addr4);
+		strcpy(ip_str, inet_ntoa(ia));
+		ia.s_addr = htonl(broadcast);
+		strcpy(broadcast_str, inet_ntoa(ia));
+
+		l_rtnl_ifaddr4_delete(rtnl, netdev_get_ifindex(netdev),
+					ap->netconfig_prefix_len4, ip_str,
+					broadcast_str, NULL, NULL, NULL);
+		ap->cleanup_addr4 = false;
+	}
 
 	if (ap->netconfig_dhcp)
 		l_dhcp_server_stop(ap->netconfig_dhcp);
 
+	l_queue_destroy(ap->netconfig_addr4_dump, l_free);
+	ap->netconfig_addr4_dump = NULL;
 	l_strv_free(ap->netconfig_addr4_str_list);
 	ap->netconfig_addr4_str_list = NULL;
 }
@@ -2047,9 +2077,20 @@ static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
 		goto failed;
 	}
 
-	if (ap->netconfig_dhcp && !l_dhcp_server_start(ap->netconfig_dhcp)) {
-		l_error("DHCP server failed to start");
-		goto failed;
+	if (ap->netconfig_dhcp) {
+		/*
+		 * 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.
+		 */
+		struct in_addr ia = { .s_addr = htonl(ap->netconfig_addr4) };
+
+		if (!l_dhcp_server_set_ip_address(ap->netconfig_dhcp,
+							inet_ntoa(ia)) ||
+				!l_dhcp_server_start(ap->netconfig_dhcp)) {
+			l_error("DHCP server failed to start");
+			goto failed;
+		}
 	}
 
 	ap->started = true;
@@ -2737,6 +2778,130 @@ static int ap_load_config(struct ap_state *ap, const struct l_settings *config,
 	return 0;
 }
 
+static void ap_netconfig_start(struct ap_state *ap)
+{
+	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
+	int ret;
+	struct in_addr ia;
+	char addr4_str[16];
+	char broadcast4_str[16];
+	uint32_t new_addr4;
+
+	/*
+	 * 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 (APAddressPool setting).
+	 */
+	if (ap->netconfig_addr4_str_list)
+		ret = ap_select_addr4((const char **)
+					ap->netconfig_addr4_str_list,
+					ap->netconfig_prefix_len4,
+					ap->netconfig_addr4_dump, &new_addr4);
+	else if (ap->netconfig_addr4)
+		goto already_set;
+	else
+		ret = ap_select_addr4((const char **) global_addr_strs,
+					ap->netconfig_prefix_len4,
+					ap->netconfig_addr4_dump, &new_addr4);
+
+	if (ret)
+		goto error;
+
+	/* See if the selected subnet is already set on the interface */
+	if (new_addr4 == ap->netconfig_addr4) {
+already_set:
+		/* Selected address already set, continue normally */
+		ap->cleanup_addr4 = false;
+
+		if (!ap_start_send(ap))
+			goto error;
+
+		goto cleanup;
+	}
+
+	ap->netconfig_addr4 = new_addr4;
+	ia.s_addr = htonl(ap->netconfig_addr4);
+	strcpy(addr4_str, inet_ntoa(ia));
+	ia.s_addr = htonl(ap->netconfig_addr4 |
+			~util_netmask_from_prefix(ap->netconfig_prefix_len4));
+	strcpy(broadcast4_str, inet_ntoa(ia));
+
+	ap->rtnl_add_cmd = l_rtnl_ifaddr4_add(rtnl, ifindex,
+						ap->netconfig_prefix_len4,
+						addr4_str, broadcast4_str,
+						ap_ifaddr4_added_cb, ap, NULL);
+	if (!ap->rtnl_add_cmd) {
+		l_error("Failed to add the IPv4 address");
+		goto error;
+	}
+
+	ap->cleanup_addr4 = true;
+
+cleanup:
+	l_queue_destroy(ap->netconfig_addr4_dump, l_free);
+	ap->netconfig_addr4_dump = NULL;
+	l_strv_free(ap->netconfig_addr4_str_list);
+	ap->netconfig_addr4_str_list = NULL;
+	return;
+
+error:
+	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;
+	struct in_addr ia;
+	struct ap_rtnl_addr4_record *addr;
+
+	if (error) {
+		l_error("Error getting existing IPv4 addresses on AP iface");
+		return;
+	}
+
+	if (type != RTM_NEWADDR || ifa->ifa_prefixlen < 1)
+		return;
+
+	l_rtnl_ifaddr4_extract(ifa, len, NULL, &ip_str, NULL);
+	inet_aton(ip_str, &ia);
+	l_free(ip_str);
+
+	/*
+	 * 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 (ifa->ifa_index == netdev_get_ifindex(ap->netdev)) {
+		ap->netconfig_prefix_len4 = ifa->ifa_prefixlen;
+		ap->netconfig_addr4 = ntohl(ia.s_addr);
+		return;
+	}
+
+	addr = l_new(struct ap_rtnl_addr4_record, 1);
+	addr->prefix_len = ifa->ifa_prefixlen;
+	addr->addr = ntohl(ia.s_addr);
+
+	if (!ap->netconfig_addr4_dump)
+		ap->netconfig_addr4_dump = l_queue_new();
+
+	l_queue_push_tail(ap->netconfig_addr4_dump, addr);
+}
+
+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;
+	ap_netconfig_start(ap);
+}
+
 /*
  * Start a simple independent WPA2 AP on given netdev.
  *
@@ -2832,7 +2997,16 @@ struct ap_state *ap_start(struct netdev *netdev, struct l_settings *config,
 		l_error("Registering for MLME notification failed");
 
 	if (ap->enable_netconfig4) {
-		/* TODO: select an IP address, set it and call ap_start_send */
+		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;
 	}
-- 
2.27.0

[PATCH 06/11] ap: Move the DHCP server freeing to ap_reset

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 948 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 37b71a6f..9813a90d 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -244,8 +244,10 @@ static void ap_reset(struct ap_state *ap)
 		ap->cleanup_addr4 = false;
 	}
 
-	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;
+	}
 
 	l_queue_destroy(ap->netconfig_addr4_dump, l_free);
 	ap->netconfig_addr4_dump = NULL;
@@ -3125,8 +3127,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 07/11] ap: Send a specific error message on async AP start failure

[Andrew Zaborowski]

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

diff --git a/src/ap.c b/src/ap.c
index 9813a90d..a676bd28 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -2058,9 +2058,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);
 
@@ -2075,8 +2077,8 @@ 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) {
@@ -2091,17 +2093,13 @@ static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
 							inet_ntoa(ia)) ||
 				!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)
@@ -2216,12 +2214,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,
@@ -2393,10 +2391,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;
@@ -2815,8 +2815,10 @@ already_set:
 		/* Selected address already set, continue normally */
 		ap->cleanup_addr4 = false;
 
-		if (!ap_start_send(ap))
+		if (!ap_start_send(ap)) {
+			ret = -EIO;
 			goto error;
+		}
 
 		goto cleanup;
 	}
@@ -2834,6 +2836,7 @@ already_set:
 						ap_ifaddr4_added_cb, ap, NULL);
 	if (!ap->rtnl_add_cmd) {
 		l_error("Failed to add the IPv4 address");
+		ret = -EIO;
 		goto error;
 	}
 
@@ -2847,7 +2850,7 @@ cleanup:
 	return;
 
 error:
-	ap_start_failed(ap);
+	ap_start_failed(ap, ret);
 }
 
 static void ap_ifaddr4_dump_cb(int error,
@@ -3205,13 +3208,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 2163d2db..30da3b34 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 08/11] 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 09/11] 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 478bd616..190c3fbd 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
@@ -175,7 +175,7 @@ The group ``[General]`` contains general settings.
        then setting ``DisableANQP`` to ``false`` is recommended.
 
 Network
----------
+-------
 
 The group ``[Network]`` contains network configuration related settings.
 
@@ -312,6 +312,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

[PATCH 10/11] main: Add NetworkConfigurationEnabled to Daemon.GetInfo()

[Andrew Zaborowski]

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

---
 src/main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/main.c b/src/main.c
index 01f1be22..992a1233 100644
--- a/src/main.c
+++ b/src/main.c
@@ -218,11 +218,19 @@ static struct l_dbus_message *iwd_dbus_get_info(struct l_dbus *dbus,
 	struct l_dbus_message *reply;
 	struct l_dbus_message_builder *builder;
 	L_AUTO_FREE_VAR(char *, storage_dir) = storage_get_path(NULL);
+	bool netconfig_enabled;
+
+	if (!l_settings_get_bool(iwd_config, "General",
+					"EnableNetworkConfiguration",
+					&netconfig_enabled))
+		netconfig_enabled = false;
 
 	reply = l_dbus_message_new_method_return(message);
 	builder = l_dbus_message_builder_new(reply);
 	l_dbus_message_builder_enter_array(builder, "{sv}");
 
+	dbus_append_dict_basic(builder, "NetworkConfigurationEnabled", 'b',
+				&netconfig_enabled);
 	dbus_append_dict_basic(builder, "StateDirectory", 's', storage_dir);
 	dbus_append_dict_basic(builder, "Version", 's', VERSION);
 
-- 
2.27.0

[PATCH 11/11] doc: Document NetworkConfigurationEnabled in Daemon.GetInfo()

[Andrew Zaborowski]

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

---
 doc/daemon-api.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/doc/daemon-api.txt b/doc/daemon-api.txt
index 439bf6f7..0e8b6b49 100644
--- a/doc/daemon-api.txt
+++ b/doc/daemon-api.txt
@@ -22,3 +22,9 @@ Methods		dict GetInfo()
 			state directory where network configuration and
 			runtime state files are kept (see $STATE_DIRECTORY in
 			iwd(8) and iwd.network(5)).
+
+			bool NetworkConfigurationEnabled - Whether network
+			configuration is enabled (see iwd(8)).  When true,
+			IWD configures the address, netmask, the resolver,
+			a route, the DHCP client or server (in station vs.
+			access point mode.)
-- 
2.27.0

Re: [PATCH 01/11] ap: Move sending CMD_START_AP to common function

[Denis Kenzior]

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

Hi Andrew,

On 5/10/21 3:57 AM, Andrew Zaborowski wrote:
> ---
>   src/ap.c | 59 +++++++++++++++++++++++++++-----------------------------
>   1 file changed, 28 insertions(+), 31 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

Re: [PATCH 02/11] ap: Refactor DHCP settings loading

[Denis Kenzior]

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

Hi Andrew,

On 5/10/21 3:57 AM, Andrew Zaborowski wrote:
> 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 without selecting a specific subnet at
> this stage because we will need to query the addresses already in use
> before this.  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.
> ---
>   src/ap.c | 346 ++++++++++++++++++++++++++-----------------------------
>   1 file changed, 165 insertions(+), 181 deletions(-)
> 

<snip>

> @@ -2490,182 +2479,185 @@ 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_load_ipv4(struct ap_state *ap,
> +				const struct l_settings *config)
>   {
> -	struct l_dhcp_server *server = ap->server;
> +	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
> +	uint32_t static_addr4 = 0;
>   	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,
> -							"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;
> -
> -	if (!l_settings_get_uint(settings, "IPv4", "LeaseTime", &lease_time))
> -		lease_time = 0;
> -
> -	if (gateway && !l_dhcp_server_set_gateway(server, gateway)) {
> -		l_error("[IPv4].Gateway value error");
> -		goto error;
> -	}
> +	if (!l_settings_has_group(config, "IPv4") || !pool.used)
> +		return 0;
>   
> -	if (dns && !l_dhcp_server_set_dns(server, dns)) {
> -		l_error("[IPv4].DNSList value error");
> -		goto error;
> -	}
> +	ap->enable_netconfig4 = true;
>   
> -	if (netmask && !l_dhcp_server_set_netmask(server, netmask)) {
> -		l_error("[IPv4].Netmask value error");
> -		goto error;
> -	}
> +	if (l_settings_has_key(config, "IPv4", "Netmask")) {
> +		L_AUTO_FREE_VAR(char *, netmask_str) =
> +			l_settings_get_string(config, "IPv4", "Netmask");
>   
> -	if (ip_range) {
> -		if (l_strv_length(ip_range) != 2) {
> -			l_error("Two addresses expected in [IPv4].IPRange");
> -			goto error;
> +		if (inet_pton(AF_INET, netmask_str, &ia) != 1) {
> +			l_error("Can't parse the profile [IPv4].Netmask "
> +				"setting");
> +			return -EINVAL;
>   		}
>   
> -		if (!l_dhcp_server_set_ip_range(server, ip_range[0],
> -							ip_range[1])) {
> -			l_error("Error setting IP range from [IPv4].IPRange");
> -			goto error;
> +		ap->netconfig_prefix_len4 = __builtin_popcount(ia.s_addr);
> +
> +		if (ntohl(ia.s_addr) != util_netmask_from_prefix(
> +						ap->netconfig_prefix_len4)) {
> +			l_error("Invalid profile [IPv4].Netmask value");
> +			return -EINVAL;
> +		}
> +	} else
> +		ap->netconfig_prefix_len4 = AP_DEFAULT_IPV4_PREFIX_LEN;
> +
> +	if (l_settings_has_key(config, "IPv4", "Address")) {
> +		char **str_list = l_settings_get_string_list(config, "IPv4",
> +								"Address", ',');
> +
> +		if (!str_list || !*str_list) {
> +			l_error("Can't parse the profile [IPv4].Address "
> +				"setting as a string list");
> +			l_free(str_list);
> +			return -EINVAL;
>   		}
> +
> +		ap->netconfig_addr4_str_list = str_list;

So you assign str_list here...

> +
> +		/* Check for the static IP syntax: Address=<IP> */
> +		if (l_strv_length(str_list) == 1 &&
> +				inet_pton(AF_INET, *str_list, &ia) == 1)
> +			static_addr4 = ntohl(ia.s_addr);
>   	}
>   
> -	if (lease_time && !l_dhcp_server_set_lease_time(server, lease_time)) {
> -		l_error("[IPv4].LeaseTime value error");
> -		goto error;
> +	ap->netconfig_dhcp = l_dhcp_server_new(ifindex);
> +	if (!ap->netconfig_dhcp) {
> +		l_error("Failed to create DHCP server on ifindex %u", ifindex);
> +		return -EINVAL;

But then it seems it is never freed?  I think in the end ap_reset does end up 
being called (several levels up), but I'd really prefer if we didn't side-effect 
things on error to make such error cases easier to maintain / review.

>   	}
>   
> -	if (netmask && inet_pton(AF_INET, netmask, &ia) > 0)
> -		ap->ip_prefix = __builtin_popcountl(ia.s_addr);
> -	else
> -		ap->ip_prefix = 24;
> +	if (getenv("IWD_DHCP_DEBUG"))
> +		l_dhcp_server_set_debug(ap->netconfig_dhcp, do_debug,
> +					"[DHCPv4 SERV] ", NULL);
>   
> -	ret = true;
> +	ia.s_addr = htonl(util_netmask_from_prefix(ap->netconfig_prefix_len4));
>   
> -error:
> -	l_strv_free(dns);
> -	l_strv_free(ip_range);
> -	return ret;
> -}
> +	if (!l_dhcp_server_set_netmask(ap->netconfig_dhcp, inet_ntoa(ia))) {
> +		l_error("l_dhcp_server_set_netmask failed");
> +		return -EINVAL;
> +	}
>   
> -/*
> - * 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)
> -{
> -	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;
> +	if (l_settings_has_key(config, "IPv4", "Gateway")) {
> +		L_AUTO_FREE_VAR(char *, gateway_str) =
> +			l_settings_get_string(config, "IPv4", "Gateway");
> +
> +		if (!l_dhcp_server_set_gateway(ap->netconfig_dhcp,
> +						gateway_str)) {
> +			l_error("l_dhcp_server_set_gateway failed");
> +			return -EINVAL;
> +		}
> +	}
>   
> -	/* 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_get_value(config, "IPv4", "IPRange")) {
> +		char **ip_range = l_settings_get_string_list(config, "IPv4",
> +								"IPRange", ',');
> +		int i;
> +		uint32_t netmask;
> +		bool r;
>   
> -	addr = l_settings_get_string(settings, "IPv4", "Address");
> -	if (addr) {
> -		if (inet_pton(AF_INET, addr, &ia) < 0)
> +		if (!static_addr4 || !l_settings_has_key(config,
> +							"IPv4", "Netmask")) {
> +			l_error("[IPv4].IPRange only makes sense in an AP "
> +				"profile if a static local address and "
> +				"netmask have also been specified");
> +			l_strv_free(ip_range);
>   			return -EINVAL;
> +		}
>   
> -		/* 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;
> +		if (!ip_range || l_strv_length(ip_range) != 2) {
> +			l_error("Can't parse the profile [IPv4].IPRange "
> +				"setting as two address strings");
> +			l_strv_free(ip_range);
> +			return -EINVAL;
>   		}
>   
> -		ap->use_ip_pool = true;
> -		ap->ip_prefix = pool.prefix;
> -		ret = 0;
> -	} else
> -		return -EALREADY;
> +		netmask = util_netmask_from_prefix(ap->netconfig_prefix_len4);
>   
> -	ap->server = l_dhcp_server_new(ifindex);
> -	if (!ap->server) {
> -		l_error("Failed to create DHCP server on %u", ifindex);
> -		return -EINVAL;
> -	}
> +		for (i = 0; i < 2; i++) {
> +			struct in_addr range_addr;
>   
> -	if (getenv("IWD_DHCP_DEBUG"))
> -		l_dhcp_server_set_debug(ap->server, do_debug,
> -							"[DHCPv4 SERV] ", NULL);
> +			if (inet_aton(ip_range[i], &range_addr) != 1) {
> +				l_error("Can't parse address in "
> +					"[IPv4].IPRange[%i]", i + 1);
> +				break;
> +			}
>   
> -	/* Set the remaining DHCP options in config file */
> -	if (!dhcp_load_settings(ap, settings))
> -		return -EINVAL;
> +			if ((static_addr4 ^ ntohl(range_addr.s_addr)) &
> +					netmask) {
> +				struct in_addr addr = { htonl(static_addr4) };
>   
> -	if (!l_dhcp_server_set_ip_address(ap->server, addr))
> -		return -EINVAL;
> +				l_error("[IPv4].IPRange[%i] is not in the "
> +					"%s/%i subnet", i + 1, inet_ntoa(addr),
> +					ap->netconfig_prefix_len4);
> +				break;
> +			}
> +		}
>   
> -	ap->own_ip = l_steal_ptr(addr);
> -	return ret;
> -}
> +		if (i < 2) {
> +			l_strv_free(ip_range);
> +			return -EINVAL;
> +		}
>   
> -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;
> +		r = l_dhcp_server_set_ip_range(ap->netconfig_dhcp,
> +						ip_range[0], ip_range[1]);
> +		l_strv_free(ip_range);
>   
> -	if (!l_settings_has_group(config, "IPv4"))
> -		return 0;
> +		if (!r) {
> +			l_error("l_dhcp_server_set_ip_range failed");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (l_settings_has_key(config, "IPv4", "DNSList")) {
> +		char **str_list = l_settings_get_string_list(config, "IPv4",
> +								"DNSList", ',');
> +		bool r;
>   
> -	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);
> -
> -		if (!ap->rtnl_add_cmd) {
> -			l_error("Failed to add IPv4 address");
> -			return -EIO;
> +		if (!str_list || !*str_list) {
> +			l_error("Can't parse the profile [IPv4].DNSList "
> +				"setting as a string list");
> +			l_free(str_list);
> +			return -EINVAL;
>   		}
>   
> -		ap->cleanup_ip = true;
> +		r = l_dhcp_server_set_dns(ap->netconfig_dhcp, str_list);
> +		l_strv_free(str_list);
>   
> -		*wait_dhcp = true;
> -		err = 0;
> -	/* Selected address already set, continue normally */
> -	} else if (err == -EALREADY) {
> -		*wait_dhcp = false;
> -		err = 0;
> +		if (!r) {
> +			l_error("l_dhcp_server_set_dns failed");
> +			return -EINVAL;
> +		}
>   	}
>   
> -	return err;
> +	if (l_settings_has_key(config, "IPv4", "LeaseTime")) {
> +		unsigned int lease_time;
> +
> +		if (!l_settings_get_uint(config, "IPv4", "LeaseTime",
> +						&lease_time)) {
> +			l_error("Error parsing [IPv4].LeaseTime as an integer");
> +			return -EINVAL;
> +		}
> +
> +		if (!l_dhcp_server_set_lease_time(ap->netconfig_dhcp,
> +							lease_time)) {
> +			l_error("l_dhcp_server_set_lease_time failed");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
>   }
>   
>   static bool ap_load_psk(struct ap_state *ap, const struct l_settings *config)

Regards,
-Denis

Re: [PATCH 04/11] ap: Add subnet address selection logic

[Denis Kenzior]

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

Hi Andrew,

On 5/10/21 3:57 AM, Andrew Zaborowski wrote:
> 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 from a second list.
> 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.)
> 
> This is added as src/ap-addr.c so that it's self-contained.
> ---
>   Makefile.am   |   3 +-
>   src/ap-addr.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/ap.h      |   9 ++
>   3 files changed, 242 insertions(+), 1 deletion(-)
>   create mode 100644 src/ap-addr.c
> 

I would rather we made this into a self contained module that pays attention to 
RTNL and tracks any addresses added / removed.  That way the caller isn't forced 
into performing an RTNL dump first...

> diff --git a/src/ap.h b/src/ap.h
> index b0100f34..2163d2db 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;
> +};
> +
> +int ap_select_addr4(const char **addr_str_list, uint8_t prefix,
> +			const struct l_queue *used_addr_dump,
> +			uint32_t *out_addr);
> 

Do we have a chance of collisions if 2 APs are brought up simultaneously? 
Should there be an explicit reserve / release semantic?  I know you use 
l_getrandom to pick the subnet, but what if the IP pool size is pretty small?

Regards,
-Denis

Re: [PATCH 10/11] main: Add NetworkConfigurationEnabled to Daemon.GetInfo()

[Denis Kenzior]

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

Hi Andrew,

On 5/10/21 3:58 AM, Andrew Zaborowski wrote:
> ---
>   src/main.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 

Patch 10 & 11 applied, thanks.

Regards,
-Denis

Re: [PATCH 04/11] ap: Add subnet address selection logic

[Andrew Zaborowski]

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

Hi Denis,

On Tue, 11 May 2021 at 17:06, Denis Kenzior <denkenz@gmail.com> wrote:
> On 5/10/21 3:57 AM, Andrew Zaborowski wrote:
> > 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 from a second list.
> > 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.)
> >
> > This is added as src/ap-addr.c so that it's self-contained.
> > ---
> >   Makefile.am   |   3 +-
> >   src/ap-addr.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   src/ap.h      |   9 ++
> >   3 files changed, 242 insertions(+), 1 deletion(-)
> >   create mode 100644 src/ap-addr.c
> >
>
> I would rather we made this into a self contained module that pays attention to
> RTNL and tracks any addresses added / removed.  That way the caller isn't forced
> into performing an RTNL dump first...

That is something I considered too but think about how many RTNL
events and process wakeups we're generating this way compared to how
often this data is actually used in a typical IWD session... do you
still think it's a good idea?

Also note we wouldn't be saving any code because the RTNL dump is
still needed at the module init, it's only moved to a different file.

>
> > diff --git a/src/ap.h b/src/ap.h
> > index b0100f34..2163d2db 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;
> > +};
> > +
> > +int ap_select_addr4(const char **addr_str_list, uint8_t prefix,
> > +                     const struct l_queue *used_addr_dump,
> > +                     uint32_t *out_addr);
> >
>
> Do we have a chance of collisions if 2 APs are brought up simultaneously?
> Should there be an explicit reserve / release semantic?  I know you use
> l_getrandom to pick the subnet, but what if the IP pool size is pretty small?

If you started the APs simultaneously and the RTNL dumps had been
running simultaneously then yeah, I guess this can happen.  I wouldn't
worry about this, we're trying to do the best thing that's easily
doable here but there's no guarantees that the subnets are unique on
the system.  We can always have a race with another process, or that
process may not even be trying to avoid conflicts or not be able to,
because it receives a conflicting IP from DHCP.

Best regards

Re: [PATCH 04/11] ap: Add subnet address selection logic

[Denis Kenzior]

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

Hi Andrew,

>> I would rather we made this into a self contained module that pays attention to
>> RTNL and tracks any addresses added / removed.  That way the caller isn't forced
>> into performing an RTNL dump first...
> 
> That is something I considered too but think about how many RTNL
> events and process wakeups we're generating this way compared to how
> often this data is actually used in a typical IWD session... do you
> still think it's a good idea?

Well, iwd already gets woken up by the RTNL events just by virtue of having rtnl 
socket open.  We can install bpf filters for this, I suppose, but I wouldn't 
want to try and implement one...  So I don't think this is a concern.

> 
> Also note we wouldn't be saving any code because the RTNL dump is
> still needed at the module init, it's only moved to a different file.

Sure, but then we can dump only once and listen to events after that.  Either 
way, it would simplify ap.c code quite a bit.

Regards,
-Denis