[PATCH 1/2] ap: Move AP parameters to a struct

[PATCH 1/2] ap: Move AP parameters to a struct

[Andrew Zaborowski]

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

To limit the number of ap_start parameters, group basic AP config
parameters in the ap_config struct that is passed as a pointer and owned
by the ap_state.
---
 src/ap.c | 134 +++++++++++++++++++++++++++++++------------------------
 src/ap.h |  23 ++++++++--
 2 files changed, 96 insertions(+), 61 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 51ee23a8..ff8ced18 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -53,10 +53,7 @@ struct ap_state {
 	ap_event_func_t event_func;
 	ap_stopped_func_t stopped_func;
 	void *user_data;
-	char *ssid;
-	uint8_t channel;
-	uint8_t *authorized_macs;
-	int authorized_macs_num;
+	struct ap_config *config;
 
 	unsigned int ciphers;
 	enum ie_rsn_cipher_suite group_cipher;
@@ -73,7 +70,6 @@ struct ap_state {
 
 	bool started : 1;
 	bool gtk_set : 1;
-	bool no_cck_rates : 1;
 };
 
 struct sta_state {
@@ -94,6 +90,30 @@ struct sta_state {
 
 static uint32_t netdev_watch;
 
+void ap_config_free(struct ap_config *config)
+{
+	if (unlikely(!config))
+		return;
+
+	l_free(config->ssid);
+
+	if (config->psk) {
+		explicit_bzero(config->psk, strlen(config->psk));
+		l_free(config->psk);
+	}
+
+	if (config->authorized_macs) {
+		uint8_t **mac;
+
+		for (mac = config->authorized_macs; *mac; mac++)
+			l_free(mac);
+
+		l_free(config->authorized_macs);
+	}
+
+	l_free(config);
+}
+
 static void ap_sta_free(void *data)
 {
 	struct sta_state *sta = data;
@@ -121,8 +141,6 @@ static void ap_reset(struct ap_state *ap)
 {
 	struct netdev *netdev = ap->netdev;
 
-	l_free(ap->ssid);
-
 	explicit_bzero(ap->pmk, sizeof(ap->pmk));
 
 	if (ap->mlme_watch)
@@ -138,7 +156,9 @@ static void ap_reset(struct ap_state *ap)
 	if (ap->rates)
 		l_uintset_free(ap->rates);
 
-	l_free(ap->authorized_macs);
+	ap_config_free(ap->config);
+	ap->config = NULL;
+
 	ap->started = false;
 }
 
@@ -318,7 +338,8 @@ static size_t ap_build_beacon_pr_head(struct ap_state *ap,
 
 	/* SSID IE */
 	ie_tlv_builder_next(&builder, IE_TYPE_SSID);
-	ie_tlv_builder_set_data(&builder, ap->ssid, strlen(ap->ssid));
+	ie_tlv_builder_set_data(&builder, ap->config->ssid,
+				strlen(ap->config->ssid));
 
 	/* Supported Rates IE */
 	ie_tlv_builder_next(&builder, IE_TYPE_SUPPORTED_RATES);
@@ -343,7 +364,7 @@ static size_t ap_build_beacon_pr_head(struct ap_state *ap,
 
 	/* DSSS Parameter Set IE for DSSS, HR, ERP and HT PHY rates */
 	ie_tlv_builder_next(&builder, IE_TYPE_DSSS_PARAMETER_SET);
-	ie_tlv_builder_set_data(&builder, &ap->channel, 1);
+	ie_tlv_builder_set_data(&builder, &ap->config->channel, 1);
 
 	ie_tlv_builder_finalize(&builder, &len);
 	return 36 + len;
@@ -375,7 +396,8 @@ static uint32_t ap_send_mgmt_frame(struct ap_state *ap,
 	struct l_genl_msg *msg;
 	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
 	uint32_t id;
-	uint32_t ch_freq = scan_channel_to_freq(ap->channel, SCAN_BAND_2_4_GHZ);
+	uint32_t ch_freq = scan_channel_to_freq(ap->config->channel,
+						SCAN_BAND_2_4_GHZ);
 
 	msg = l_genl_msg_new_sized(NL80211_CMD_FRAME, 128 + frame_len);
 
@@ -386,7 +408,7 @@ static uint32_t ap_send_mgmt_frame(struct ap_state *ap,
 		l_genl_msg_append_attr(msg, NL80211_ATTR_DONT_WAIT_FOR_ACK,
 					0, NULL);
 
-	if (ap->no_cck_rates)
+	if (ap->config->no_cck_rates)
 		l_genl_msg_append_attr(msg, NL80211_ATTR_TX_NO_CCK_RATE, 0,
 					NULL);
 
@@ -446,7 +468,8 @@ static void ap_start_rsna(struct sta_state *sta, const uint8_t *gtk_rsc)
 	sta->hs = netdev_handshake_state_new(netdev);
 
 	handshake_state_set_event_func(sta->hs, ap_handshake_event, sta);
-	handshake_state_set_ssid(sta->hs, (void *)ap->ssid, strlen(ap->ssid));
+	handshake_state_set_ssid(sta->hs, (void *) ap->config->ssid,
+					strlen(ap->config->ssid));
 	handshake_state_set_authenticator(sta->hs, true);
 	handshake_state_set_authenticator_ie(sta->hs, bss_rsne);
 	handshake_state_set_supplicant_ie(sta->hs, sta->assoc_rsne);
@@ -900,8 +923,8 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
 			break;
 		}
 
-	if (!rates || !ssid || !rsn || ssid_len != strlen(ap->ssid) ||
-			memcmp(ssid, ap->ssid, ssid_len)) {
+	if (!rates || !ssid || !rsn || ssid_len != strlen(ap->config->ssid) ||
+			memcmp(ssid, ap->config->ssid, ssid_len)) {
 		err = MMPDU_REASON_CODE_INVALID_IE;
 		goto bad_frame;
 	}
@@ -1125,8 +1148,8 @@ static void ap_probe_req_cb(const struct mmpdu_header *hdr, const void *body,
 
 	if (!ssid || ssid_len == 0) /* Wildcard SSID */
 		match = true;
-	else if (ssid && ssid_len == strlen(ap->ssid) && /* Specific SSID */
-			!memcmp(ssid, ap->ssid, ssid_len))
+	else if (ssid && ssid_len == strlen(ap->config->ssid) && /* One SSID */
+			!memcmp(ssid, ap->config->ssid, ssid_len))
 		match = true;
 	else if (ssid_list) { /* SSID List */
 		ie_tlv_iter_init(&iter, ssid_list, ssid_list_len);
@@ -1138,15 +1161,16 @@ static void ap_probe_req_cb(const struct mmpdu_header *hdr, const void *body,
 			ssid = (const char *) ie_tlv_iter_get_data(&iter);
 			ssid_len = ie_tlv_iter_get_length(&iter);
 
-			if (ssid_len == strlen(ap->ssid) &&
-					!memcmp(ssid, ap->ssid, ssid_len)) {
+			if (ssid_len == strlen(ap->config->ssid) &&
+					!memcmp(ssid, ap->config->ssid,
+						ssid_len)) {
 				match = true;
 				break;
 			}
 		}
 	}
 
-	if (dsss_channel != 0 && dsss_channel != ap->channel)
+	if (dsss_channel != 0 && dsss_channel != ap->config->channel)
 		match = false;
 
 	if (!match)
@@ -1247,14 +1271,14 @@ static void ap_auth_cb(const struct mmpdu_header *hdr, const void *body,
 			memcmp(hdr->address_3, bssid, 6))
 		return;
 
-	if (ap->authorized_macs) {
-		int i;
+	if (ap->config->authorized_macs) {
+		uint8_t *const *mac;
 
-		for (i = 0; i < ap->authorized_macs_num; i++)
-			if (!memcmp(from, ap->authorized_macs + i * 6, 6))
+		for (mac = ap->config->authorized_macs; *mac; mac++)
+			if (!memcmp(from, *mac, 6))
 				break;
 
-		if (i == ap->authorized_macs_num) {
+		if (!*mac) {
 			ap_auth_reply(ap, from, MMPDU_REASON_CODE_UNSPECIFIED);
 			return;
 		}
@@ -1374,7 +1398,8 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 	uint32_t nl_akm = CRYPTO_AKM_PSK;
 	uint32_t wpa_version = NL80211_WPA_VERSION_2;
 	uint32_t auth_type = NL80211_AUTHTYPE_OPEN_SYSTEM;
-	uint32_t ch_freq = scan_channel_to_freq(ap->channel, SCAN_BAND_2_4_GHZ);
+	uint32_t ch_freq = scan_channel_to_freq(ap->config->channel,
+						SCAN_BAND_2_4_GHZ);
 	uint32_t ch_width = NL80211_CHAN_WIDTH_20;
 
 	static const uint8_t bcast_addr[6] = {
@@ -1389,7 +1414,7 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 		return NULL;
 
 	cmd = l_genl_msg_new_sized(NL80211_CMD_START_AP, 256 + head_len +
-					tail_len + strlen(ap->ssid));
+					tail_len + strlen(ap->config->ssid));
 
 	/* SET_BEACON attrs */
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_BEACON_HEAD, head_len, head);
@@ -1403,8 +1428,8 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 				&ap->beacon_interval);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_DTIM_PERIOD, 4, &dtim_period);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_IFINDEX, 4, &ifindex);
-	l_genl_msg_append_attr(cmd, NL80211_ATTR_SSID, strlen(ap->ssid),
-				ap->ssid);
+	l_genl_msg_append_attr(cmd, NL80211_ATTR_SSID, strlen(ap->config->ssid),
+				ap->config->ssid);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_HIDDEN_SSID, 4,
 				&hidden_ssid);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_CIPHER_SUITES_PAIRWISE, 4,
@@ -1462,11 +1487,14 @@ static void ap_mlme_notify(struct l_genl_msg *msg, void *user_data)
  * @event_func is required and must react to AP_EVENT_START_FAILED
  * and AP_EVENT_STOPPING by forgetting the ap_state struct, which
  * is going to be freed automatically.
- * @channel is optional.
+ * In the @config struct only .ssid and .psk need to be non-NUL,
+ * other fields are optional.  If @ap_start succeeds, the returned
+ * ap_state takes ownership of @config and the caller shouldn't
+ * free it or any of the memory pointed to by its members (they
+ * also can't be static).  In principle there's no reason that the
+ * caller shouldn't read/update some of the members though.
  */
-struct ap_state *ap_start(struct netdev *netdev, const char *ssid,
-				const char *psk, int channel, bool no_cck_rates,
-				const uint8_t **authorized_macs,
+struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
 				ap_event_func_t event_func, void *user_data)
 {
 	struct ap_state *ap;
@@ -1476,30 +1504,14 @@ struct ap_state *ap_start(struct netdev *netdev, const char *ssid,
 
 	ap = l_new(struct ap_state, 1);
 	ap->nl80211 = l_genl_family_new(iwd_get_genl(), NL80211_GENL_NAME);
-	ap->ssid = l_strdup(ssid);
+	ap->config = config;
 	ap->netdev = netdev;
-	ap->no_cck_rates = no_cck_rates;
 	ap->event_func = event_func;
 	ap->user_data = user_data;
 
-	if (channel)
-		ap->channel = channel;
-	else {
+	if (!config->channel)
 		/* TODO: Start a Get Survey to decide the channel */
-		ap->channel = 6;
-	}
-
-	if (authorized_macs) {
-		int i;
-
-		for (i = 0; authorized_macs[i]; i++);
-		ap->authorized_macs = l_malloc(i * 6);
-		ap->authorized_macs_num = i;
-
-		for (i = 0; authorized_macs[i]; i++)
-			memcpy(ap->authorized_macs + i * 6, authorized_macs[i],
-				6);
-	}
+		config->channel = 6;
 
 	/* TODO: Add all ciphers supported by wiphy */
 	ap->ciphers = wiphy_select_cipher(wiphy, 0xffff);
@@ -1507,7 +1519,7 @@ struct ap_state *ap_start(struct netdev *netdev, const char *ssid,
 	ap->beacon_interval = 100;
 
 	/* TODO: Pick from actual supported rates */
-	if (no_cck_rates) {
+	if (config->no_cck_rates) {
 		l_uintset_put(ap->rates, 12); /* 6 Mbps*/
 		l_uintset_put(ap->rates, 18); /* 9 Mbps*/
 		l_uintset_put(ap->rates, 24); /* 12 Mbps*/
@@ -1523,8 +1535,8 @@ struct ap_state *ap_start(struct netdev *netdev, const char *ssid,
 		l_uintset_put(ap->rates, 22); /* 11 Mbps*/
 	}
 
-	if (crypto_psk_from_passphrase(psk, (uint8_t *) ssid, strlen(ssid),
-					ap->pmk) < 0)
+	if (crypto_psk_from_passphrase(config->psk, (uint8_t *) config->ssid,
+					strlen(config->ssid), ap->pmk) < 0)
 		goto error;
 
 	if (!frame_watch_add(wdev_id, 0, 0x0000 |
@@ -1757,6 +1769,7 @@ static struct l_dbus_message *ap_dbus_start(struct l_dbus *dbus,
 {
 	struct ap_if_data *ap_if = user_data;
 	const char *ssid, *wpa2_psk;
+	struct ap_config *config;
 
 	if (ap_if->ap && ap_if->ap->started)
 		return dbus_error_already_exists(message);
@@ -1767,10 +1780,15 @@ static struct l_dbus_message *ap_dbus_start(struct l_dbus *dbus,
 	if (!l_dbus_message_get_arguments(message, "ss", &ssid, &wpa2_psk))
 		return dbus_error_invalid_args(message);
 
-	ap_if->ap = ap_start(ap_if->netdev, ssid, wpa2_psk, 0, false, NULL,
-				ap_if_event_func, ap_if);
-	if (!ap_if->ap)
+	config = l_new(struct ap_config, 1);
+	config->ssid = l_strdup(ssid);
+	config->psk = l_strdup(wpa2_psk);
+
+	ap_if->ap = ap_start(ap_if->netdev, config, ap_if_event_func, ap_if);
+	if (!ap_if->ap) {
+		ap_config_free(config);
 		return dbus_error_invalid_args(message);
+	}
 
 	ap_if->pending = l_dbus_message_ref(message);
 	return NULL;
diff --git a/src/ap.h b/src/ap.h
index b007b657..3ca9467f 100644
--- a/src/ap.h
+++ b/src/ap.h
@@ -41,13 +41,30 @@ struct ap_event_station_removed_data {
 	enum mmpdu_reason_code reason;
 };
 
+struct ap_event_registration_started_data {
+	const uint8_t *mac;
+	const struct iovec *assoc_ies;
+};
+
+struct ap_event_registration_success_data {
+	const uint8_t *mac;
+};
+
 typedef void (*ap_event_func_t)(enum ap_event_type type, const void *event_data,
 				void *user_data);
 typedef void (*ap_stopped_func_t)(void *user_data);
 
-struct ap_state *ap_start(struct netdev *netdev, const char *ssid,
-				const char *psk, int channel, bool no_cck_rates,
-				const uint8_t **authorized_macs,
+struct ap_config {
+	char *ssid;
+	char *psk;
+	uint8_t channel;
+	uint8_t **authorized_macs;
+	bool no_cck_rates : 1;
+};
+
+void ap_config_free(struct ap_config *config);
+
+struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
 				ap_event_func_t event_func, void *user_data);
 void ap_shutdown(struct ap_state *ap, ap_stopped_func_t stopped_func,
 			void *user_data);
-- 
2.25.1

[PATCH 2/2] ap: Add an optional client count limit parameter

[Andrew Zaborowski]

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

Add ap_config.max_stations, we'll probably want to set it to 1 for P2P
groups but this setting is also available in Android etc. when doing
tethering.
---
 src/ap.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/ap.h |  1 +
 2 files changed, 65 insertions(+)

diff --git a/src/ap.c b/src/ap.c
index ff8ced18..bae7508f 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -86,6 +86,7 @@ struct sta_state {
 	struct eapol_sm *sm;
 	struct handshake_state *hs;
 	uint32_t gtk_query_cmd_id;
+	struct l_idle *remove_work;
 };
 
 static uint32_t netdev_watch;
@@ -134,6 +135,9 @@ static void ap_sta_free(void *data)
 	if (sta->hs)
 		handshake_state_free(sta->hs);
 
+	if (sta->remove_work)
+		l_idle_remove(sta->remove_work);
+
 	l_free(sta);
 }
 
@@ -421,6 +425,37 @@ static uint32_t ap_send_mgmt_frame(struct ap_state *ap,
 	return id;
 }
 
+static unsigned int ap_get_sta_count(struct ap_state *ap)
+{
+	const struct l_queue_entry *entry;
+	unsigned int cnt = 0;
+
+	for (entry = l_queue_get_entries(ap->sta_states); entry;
+			entry = entry->next) {
+		struct sta_state *sta = entry->data;
+
+		if (sta->rsna)
+			cnt++;
+	}
+
+	return cnt;
+}
+
+static void ap_sta_remove_work(struct l_idle *idle, void *user_data)
+{
+	struct sta_state *sta = user_data;
+
+	l_idle_remove(idle);
+	sta->remove_work = NULL;
+
+	if (!sta->hs)
+		return;
+
+	netdev_handshake_failed(sta->hs, MMPDU_REASON_CODE_DISASSOC_AP_BUSY);
+	sta->sm = NULL;
+	ap_remove_sta(sta);
+}
+
 static void ap_handshake_event(struct handshake_state *hs,
 		enum handshake_event event, void *user_data, ...)
 {
@@ -430,6 +465,28 @@ static void ap_handshake_event(struct handshake_state *hs,
 	va_start(args, user_data);
 
 	switch (event) {
+	case HANDSHAKE_EVENT_SETTING_KEYS:
+		/*
+		 * config->max_stations limits the number of fully connected
+		 * STAs, i.e. the clients who have the credentials, so that
+		 * those that don't have them can't keep the legit ones from
+		 * connecting.  We've checked that we're below the limit when
+		 * processing the Authentication frame, just so that we don't
+		 * start a new handshake knowing that we can't accept another
+		 * STA, but the definitive check is here just before
+		 * authorizing the STA.
+		 */
+		if (sta->ap->config->max_stations &&
+				ap_get_sta_count(sta->ap) >=
+				sta->ap->config->max_stations) {
+			l_debug("STA over max_stations limit being removed "
+				"after successfull handshake");
+			sta->remove_work = l_idle_create(ap_sta_remove_work,
+								sta, NULL);
+			return;
+		}
+
+		break;
 	case HANDSHAKE_EVENT_COMPLETE:
 		ap_new_rsna(sta);
 		break;
@@ -1311,6 +1368,13 @@ static void ap_auth_cb(const struct mmpdu_header *hdr, const void *body,
 	if (sta)
 		goto done;
 
+	if (ap->config->max_stations &&
+			ap_get_sta_count(ap) >= ap->config->max_stations) {
+		l_debug("STA rejected because we're at max_station");
+		ap_auth_reply(ap, from, MMPDU_REASON_CODE_DISASSOC_AP_BUSY);
+		return;
+	}
+
 	/*
 	 * Per 12.3.3.2.3 with Open System the state change is immediate,
 	 * no waiting for the response to be ACKed as with the association
diff --git a/src/ap.h b/src/ap.h
index 3ca9467f..ebd84864 100644
--- a/src/ap.h
+++ b/src/ap.h
@@ -59,6 +59,7 @@ struct ap_config {
 	char *psk;
 	uint8_t channel;
 	uint8_t **authorized_macs;
+	unsigned int max_stations;
 	bool no_cck_rates : 1;
 };
 
-- 
2.25.1

Re: [PATCH 2/2] ap: Add an optional client count limit parameter

[Denis Kenzior]

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

Hi Andrew,

On 8/25/20 7:18 PM, Andrew Zaborowski wrote:
> Add ap_config.max_stations, we'll probably want to set it to 1 for P2P
> groups but this setting is also available in Android etc. when doing
> tethering.
> ---
>   src/ap.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/ap.h |  1 +
>   2 files changed, 65 insertions(+)
> 

<snip>

> @@ -430,6 +465,28 @@ static void ap_handshake_event(struct handshake_state *hs,
>   	va_start(args, user_data);
>   
>   	switch (event) {
> +	case HANDSHAKE_EVENT_SETTING_KEYS:
> +		/*
> +		 * config->max_stations limits the number of fully connected
> +		 * STAs, i.e. the clients who have the credentials, so that
> +		 * those that don't have them can't keep the legit ones from
> +		 * connecting.  We've checked that we're below the limit when
> +		 * processing the Authentication frame, just so that we don't
> +		 * start a new handshake knowing that we can't accept another
> +		 * STA, but the definitive check is here just before
> +		 * authorizing the STA.
> +		 */
> +		if (sta->ap->config->max_stations &&
> +				ap_get_sta_count(sta->ap) >=
> +				sta->ap->config->max_stations) {
> +			l_debug("STA over max_stations limit being removed "
> +				"after successfull handshake");
> +			sta->remove_work = l_idle_create(ap_sta_remove_work,
> +								sta, NULL);

Not sure I really like this.  We generate a bunch of messages to the kernel and 
only after the keys setting messages are sent / acked by the kernel would the 
del_station actually get sent.

Perhaps we should introduce a pre-setting_keys state and have eapol.c check 
whether handshake_state should proceed after the event has been fired.

Regards,
-Denis

Re: [PATCH 1/2] ap: Move AP parameters to a struct

[Denis Kenzior]

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

Hi Andrew,

On 8/25/20 7:18 PM, Andrew Zaborowski wrote:
> To limit the number of ap_start parameters, group basic AP config
> parameters in the ap_config struct that is passed as a pointer and owned
> by the ap_state.
> ---
>   src/ap.c | 134 +++++++++++++++++++++++++++++++------------------------
>   src/ap.h |  23 ++++++++--
>   2 files changed, 96 insertions(+), 61 deletions(-)
> 

<snip>

> @@ -94,6 +90,30 @@ struct sta_state {
>   
>   static uint32_t netdev_watch;
>   
> +void ap_config_free(struct ap_config *config)
> +{
> +	if (unlikely(!config))
> +		return;
> +
> +	l_free(config->ssid);
> +
> +	if (config->psk) {
> +		explicit_bzero(config->psk, strlen(config->psk));
> +		l_free(config->psk);
> +	}
> +
> +	if (config->authorized_macs) {
> +		uint8_t **mac;
> +
> +		for (mac = config->authorized_macs; *mac; mac++)
> +			l_free(mac);

Do you mean l_free(*mac) here?

> +
> +		l_free(config->authorized_macs);

But really, this is probably way better written as a while loop with array indexing.

int i = 0;
uint8_t *mac;
while (mac = config->authorized_macs[i++))
	l_free(mac);

l_free(config->authorized_macs);

I do wonder why you decided to make authorized_macs into a double-level array 
instead of just a flat one...  You can always realloc if you need to grow it..

> +	}
> +
> +	l_free(config);
> +}
> +
>   static void ap_sta_free(void *data)
>   {
>   	struct sta_state *sta = data;

<snip>

> @@ -1247,14 +1271,14 @@ static void ap_auth_cb(const struct mmpdu_header *hdr, const void *body,
>   			memcmp(hdr->address_3, bssid, 6))
>   		return;
>   
> -	if (ap->authorized_macs) {
> -		int i;
> +	if (ap->config->authorized_macs) {
> +		uint8_t *const *mac;
>   
> -		for (i = 0; i < ap->authorized_macs_num; i++)
> -			if (!memcmp(from, ap->authorized_macs + i * 6, 6))
> +		for (mac = ap->config->authorized_macs; *mac; mac++)
> +			if (!memcmp(from, *mac, 6))
>   				break;

Maybe consider a while loop here as well...

>   
> -		if (i == ap->authorized_macs_num) {
> +		if (!*mac) {
>   			ap_auth_reply(ap, from, MMPDU_REASON_CODE_UNSPECIFIED);
>   			return;
>   		}

<snip>

> @@ -1462,11 +1487,14 @@ static void ap_mlme_notify(struct l_genl_msg *msg, void *user_data)
>    * @event_func is required and must react to AP_EVENT_START_FAILED
>    * and AP_EVENT_STOPPING by forgetting the ap_state struct, which
>    * is going to be freed automatically.
> - * @channel is optional.
> + * In the @config struct only .ssid and .psk need to be non-NUL,
> + * other fields are optional.  If @ap_start succeeds, the returned
> + * ap_state takes ownership of @config and the caller shouldn't
> + * free it or any of the memory pointed to by its members (they
> + * also can't be static).  In principle there's no reason that the
> + * caller shouldn't read/update some of the members though.
>    */

I think eventually we probably want to make this a proper API.  like 
ap_config_new, ap_config_set_ssid, ap_config_set_passphrase, etc.

> -struct ap_state *ap_start(struct netdev *netdev, const char *ssid,
> -				const char *psk, int channel, bool no_cck_rates,
> -				const uint8_t **authorized_macs,
> +struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
>   				ap_event_func_t event_func, void *user_data)

But this now looks much more manageable...

>   {
>   	struct ap_state *ap;

<snip>

> diff --git a/src/ap.h b/src/ap.h
> index b007b657..3ca9467f 100644
> --- a/src/ap.h
> +++ b/src/ap.h
> @@ -41,13 +41,30 @@ struct ap_event_station_removed_data {
>   	enum mmpdu_reason_code reason;
>   };
>   
> +struct ap_event_registration_started_data {
> +	const uint8_t *mac;
> +	const struct iovec *assoc_ies;
> +};
> +
> +struct ap_event_registration_success_data {
> +	const uint8_t *mac;
> +};
> +

These don't seem to belong to this patch...

>   typedef void (*ap_event_func_t)(enum ap_event_type type, const void *event_data,
>   				void *user_data);
>   typedef void (*ap_stopped_func_t)(void *user_data);
>   
> -struct ap_state *ap_start(struct netdev *netdev, const char *ssid,
> -				const char *psk, int channel, bool no_cck_rates,
> -				const uint8_t **authorized_macs,
> +struct ap_config {
> +	char *ssid;
> +	char *psk;
> +	uint8_t channel;
> +	uint8_t **authorized_macs;
> +	bool no_cck_rates : 1;
> +};
> +

Adding a ap_config_new and setters would allow making this structure opaque.

> +void ap_config_free(struct ap_config *config);
> +
> +struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
>   				ap_event_func_t event_func, void *user_data);
>   void ap_shutdown(struct ap_state *ap, ap_stopped_func_t stopped_func,
>   			void *user_data);
> 

Regards,
-Denis

Re: [PATCH 1/2] ap: Move AP parameters to a struct

[Andrew Zaborowski]

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

Hi Denis,

On Thu, 27 Aug 2020 at 19:09, Denis Kenzior <denkenz@gmail.com> wrote:
> On 8/25/20 7:18 PM, Andrew Zaborowski wrote:
> > To limit the number of ap_start parameters, group basic AP config
> > parameters in the ap_config struct that is passed as a pointer and owned
> > by the ap_state.
> > ---
> >   src/ap.c | 134 +++++++++++++++++++++++++++++++------------------------
> >   src/ap.h |  23 ++++++++--
> >   2 files changed, 96 insertions(+), 61 deletions(-)
> >
>
> <snip>
>
> > @@ -94,6 +90,30 @@ struct sta_state {
> >
> >   static uint32_t netdev_watch;
> >
> > +void ap_config_free(struct ap_config *config)
> > +{
> > +     if (unlikely(!config))
> > +             return;
> > +
> > +     l_free(config->ssid);
> > +
> > +     if (config->psk) {
> > +             explicit_bzero(config->psk, strlen(config->psk));
> > +             l_free(config->psk);
> > +     }
> > +
> > +     if (config->authorized_macs) {
> > +             uint8_t **mac;
> > +
> > +             for (mac = config->authorized_macs; *mac; mac++)
> > +                     l_free(mac);
>
> Do you mean l_free(*mac) here?

Oops, yes.

>
> > +
> > +             l_free(config->authorized_macs);
>
> But really, this is probably way better written as a while loop with array indexing.
>
> int i = 0;
> uint8_t *mac;
> while (mac = config->authorized_macs[i++))
>         l_free(mac);
>
> l_free(config->authorized_macs);

Ok.

>
> I do wonder why you decided to make authorized_macs into a double-level array
> instead of just a flat one...  You can always realloc if you need to grow it..

Right, I thought it's just more natural when you can access n'th
element with array[n], or you can iterate like over a list like I did
here.
And you don't need to make assumptions about the length of the array.

>
> > +     }
> > +
> > +     l_free(config);
> > +}
> > +
> >   static void ap_sta_free(void *data)
> >   {
> >       struct sta_state *sta = data;
>
> <snip>
>
> > @@ -1247,14 +1271,14 @@ static void ap_auth_cb(const struct mmpdu_header *hdr, const void *body,
> >                       memcmp(hdr->address_3, bssid, 6))
> >               return;
> >
> > -     if (ap->authorized_macs) {
> > -             int i;
> > +     if (ap->config->authorized_macs) {
> > +             uint8_t *const *mac;
> >
> > -             for (i = 0; i < ap->authorized_macs_num; i++)
> > -                     if (!memcmp(from, ap->authorized_macs + i * 6, 6))
> > +             for (mac = ap->config->authorized_macs; *mac; mac++)
> > +                     if (!memcmp(from, *mac, 6))
> >                               break;
>
> Maybe consider a while loop here as well...

Ok, if you prefer that.

>
> >
> > -             if (i == ap->authorized_macs_num) {
> > +             if (!*mac) {
> >                       ap_auth_reply(ap, from, MMPDU_REASON_CODE_UNSPECIFIED);
> >                       return;
> >               }
>
> <snip>
>
> > @@ -1462,11 +1487,14 @@ static void ap_mlme_notify(struct l_genl_msg *msg, void *user_data)
> >    * @event_func is required and must react to AP_EVENT_START_FAILED
> >    * and AP_EVENT_STOPPING by forgetting the ap_state struct, which
> >    * is going to be freed automatically.
> > - * @channel is optional.
> > + * In the @config struct only .ssid and .psk need to be non-NUL,
> > + * other fields are optional.  If @ap_start succeeds, the returned
> > + * ap_state takes ownership of @config and the caller shouldn't
> > + * free it or any of the memory pointed to by its members (they
> > + * also can't be static).  In principle there's no reason that the
> > + * caller shouldn't read/update some of the members though.
> >    */
>
> I think eventually we probably want to make this a proper API.  like
> ap_config_new, ap_config_set_ssid, ap_config_set_passphrase, etc.

Do you mean eventually, or now?  I personally would prefer l_settings
over that, once you add accessors it loses the benefit of a nice
struct that can be read and written easily.

I'm gonna fix the other problems and resend this with the public struct for now.

>
> > -struct ap_state *ap_start(struct netdev *netdev, const char *ssid,
> > -                             const char *psk, int channel, bool no_cck_rates,
> > -                             const uint8_t **authorized_macs,
> > +struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
> >                               ap_event_func_t event_func, void *user_data)
>
> But this now looks much more manageable...
>
> >   {
> >       struct ap_state *ap;
>
> <snip>
>
> > diff --git a/src/ap.h b/src/ap.h
> > index b007b657..3ca9467f 100644
> > --- a/src/ap.h
> > +++ b/src/ap.h
> > @@ -41,13 +41,30 @@ struct ap_event_station_removed_data {
> >       enum mmpdu_reason_code reason;
> >   };
> >
> > +struct ap_event_registration_started_data {
> > +     const uint8_t *mac;
> > +     const struct iovec *assoc_ies;
> > +};
> > +
> > +struct ap_event_registration_success_data {
> > +     const uint8_t *mac;
> > +};
> > +
>
> These don't seem to belong to this patch...

Oh right, my error.

Best regards

Re: [PATCH 1/2] ap: Move AP parameters to a struct

[Denis Kenzior]

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

Hi Andrew,

>> I do wonder why you decided to make authorized_macs into a double-level array
>> instead of just a flat one...  You can always realloc if you need to grow it..
> 
> Right, I thought it's just more natural when you can access n'th
> element with array[n], or you can iterate like over a list like I did
> here.
> And you don't need to make assumptions about the length of the array.

That just seems so wasteful though?  I mean each pointer is 8 bytes on 64-bit, 
more than you pay for the mac address itself.  Plus god knows how much you pay 
in malloc overhead.  No point being wasteful unnecessarily.

>>
>> I think eventually we probably want to make this a proper API.  like
>> ap_config_new, ap_config_set_ssid, ap_config_set_passphrase, etc.
> 
> Do you mean eventually, or now?  I personally would prefer l_settings
> over that, once you add accessors it loses the benefit of a nice
> struct that can be read and written easily.
> 
But this can be defined inside ap.c.  So opaque only to clients of ap.c.  Do you 
need to access the struct elsewhere?  If so, then the whole blurb in the 
documentation about ap_start taking ownership isn't really true...

Regards,
-Denis

Re: [PATCH 1/2] ap: Move AP parameters to a struct

[Andrew Zaborowski]

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

On Thu, 27 Aug 2020 at 19:37, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Andrew,
>
> >> I do wonder why you decided to make authorized_macs into a double-level array
> >> instead of just a flat one...  You can always realloc if you need to grow it..
> >
> > Right, I thought it's just more natural when you can access n'th
> > element with array[n], or you can iterate like over a list like I did
> > here.
> > And you don't need to make assumptions about the length of the array.
>
> That just seems so wasteful though?  I mean each pointer is 8 bytes on 64-bit,
> more than you pay for the mac address itself.  Plus god knows how much you pay
> in malloc overhead.  No point being wasteful unnecessarily.

In theory that makes sense, so yes, let me switch to an uint8_t array.

In practice there's going to be zero or one address and there can be
only one such array per netdev.

>
> >>
> >> I think eventually we probably want to make this a proper API.  like
> >> ap_config_new, ap_config_set_ssid, ap_config_set_passphrase, etc.
> >
> > Do you mean eventually, or now?  I personally would prefer l_settings
> > over that, once you add accessors it loses the benefit of a nice
> > struct that can be read and written easily.
> >
> But this can be defined inside ap.c.  So opaque only to clients of ap.c.

I still can't see a benefit though (it's more code for the ap.c
developer and more code for the user, possibly more bureaucracy (more
back and forth on the mailing list to add a parameter).  I guess it's
a question of preference, you may like a specific style enough that it
outweights that.

Also l_settings is aonther option.

> Do you
> need to access the struct elsewhere?

I was thinking hypothetically a user could access config->channel
after ap_start to learn what channel has been selected.

> If so, then the whole blurb in the
> documentation about ap_start taking ownership isn't really true...

If your structs aren't opaque then you don't need to own the struct to
access it, you only need to adhere to instructions.  For example,
don't acccess the struct after the owner has been freed.

Best regards

Re: [PATCH 1/2] ap: Move AP parameters to a struct

[Denis Kenzior]

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

Hi Andrew,

On 8/27/20 1:00 PM, Andrew Zaborowski wrote:
> On Thu, 27 Aug 2020 at 19:37, Denis Kenzior <denkenz@gmail.com> wrote:
>> Hi Andrew,
>>
>>>> I do wonder why you decided to make authorized_macs into a double-level array
>>>> instead of just a flat one...  You can always realloc if you need to grow it..
>>>
>>> Right, I thought it's just more natural when you can access n'th
>>> element with array[n], or you can iterate like over a list like I did
>>> here.
>>> And you don't need to make assumptions about the length of the array.
>>
>> That just seems so wasteful though?  I mean each pointer is 8 bytes on 64-bit,
>> more than you pay for the mac address itself.  Plus god knows how much you pay
>> in malloc overhead.  No point being wasteful unnecessarily.
> 
> In theory that makes sense, so yes, let me switch to an uint8_t array.
> 
> In practice there's going to be zero or one address and there can be
> only one such array per netdev.

Right, which is why I'd just use a single mac for now until you actually have a 
use for multiple ones.  Keeps the code simple as well.

> 
>>
>>>>
>>>> I think eventually we probably want to make this a proper API.  like
>>>> ap_config_new, ap_config_set_ssid, ap_config_set_passphrase, etc.
>>>
>>> Do you mean eventually, or now?  I personally would prefer l_settings
>>> over that, once you add accessors it loses the benefit of a nice
>>> struct that can be read and written easily.
>>>
>> But this can be defined inside ap.c.  So opaque only to clients of ap.c.
> 
> I still can't see a benefit though (it's more code for the ap.c
> developer and more code for the user, possibly more bureaucracy (more
> back and forth on the mailing list to add a parameter).  I guess it's
> a question of preference, you may like a specific style enough that it
> outweights that.

I'd argue the opposite.  In the longer term you can transform the ap_config 
internals without affecting any clients.  Anyhow, this is not really important 
right now.  Feel free to proceed with the setup you have now.

> 
> Also l_settings is aonther option.

I know, and I told you to just pick one with the warning that ap_settings will 
likely end up having a more complex API in the long term.

> 
>> Do you
>> need to access the struct elsewhere?
> 
> I was thinking hypothetically a user could access config->channel
> after ap_start to learn what channel has been selected.

That seems like a candidate for ap_foo API.

> 
>> If so, then the whole blurb in the
>> documentation about ap_start taking ownership isn't really true...
> 
> If your structs aren't opaque then you don't need to own the struct to
> access it, you only need to adhere to instructions.  For example,
> don't acccess the struct after the owner has been freed.

Don't even think about it ;)  iwd is now roughly ~80kloc.  What you suggest will 
break down quick at this level of complexity.

Regards,
Denis