[PATCH 1/5] resolve: Exit methods if resolve is NULL

[PATCH 1/5] resolve: Exit methods if resolve is NULL

[Andrew Zaborowski]

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

If no resolve method gets configured, the resolve_new() call in
netconfig returns NULL.  To avoid adding checks netconfig every time
netconfig->resolve is used, add these checks directly in the resolve_*
methods.  Fixes the following crash:

src/netconfig.c:netconfig_destroy()
Aborting (signal 11) [/path/iwd]
++++++++ backtrace ++++++++
 #0  0x7f3ee427f210 in /lib/x86_64-linux-gnu/libc.so.6
 #1  0x43bcf4 in resolve_revert() at src/resolve.c:85
 #2  0x43ae2d in netconfig_destroy() at src/netconfig.c:1206
 #3  0x440cd6 in p2p_connection_reset() at src/p2p.c:662
 #4  0x47200f in process_unicast() at ell/genl.c:979
 #5  0x46e807 in io_callback() at ell/io.c:126
 #6  0x46d9bd in l_main_iterate() at ell/main.c:467 (discriminator 2)
 #7  0x46da8c in l_main_run() at ell/main.c:516
 #8  0x4047c6 in main() at src/main.c:506
 #9  0x7f3ee42600b3 in /lib/x86_64-linux-gnu/libc.so.6
+++++++++++++++++++++++++++
---
 src/resolve.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/resolve.c b/src/resolve.c
index 066e4c87..07eee5a9 100644
--- a/src/resolve.c
+++ b/src/resolve.c
@@ -60,7 +60,7 @@ static inline void _resolve_init(struct resolve *resolve, uint32_t ifindex,
 
 void resolve_add_dns(struct resolve *resolve, uint8_t type, char **dns_list)
 {
-	if (!dns_list || !*dns_list)
+	if (!resolve || !dns_list || !*dns_list)
 		return;
 
 	if (!resolve->ops->add_dns)
@@ -71,7 +71,7 @@ void resolve_add_dns(struct resolve *resolve, uint8_t type, char **dns_list)
 
 void resolve_add_domain_name(struct resolve *resolve, const char *domain_name)
 {
-	if (!domain_name)
+	if (!resolve || !domain_name)
 		return;
 
 	if (!resolve->ops->add_domain_name)
@@ -82,7 +82,7 @@ void resolve_add_domain_name(struct resolve *resolve, const char *domain_name)
 
 void resolve_revert(struct resolve *resolve)
 {
-	if (!resolve->ops->revert)
+	if (!resolve || !resolve->ops->revert)
 		return;
 
 	resolve->ops->revert(resolve);
@@ -90,6 +90,9 @@ void resolve_revert(struct resolve *resolve)
 
 void resolve_free(struct resolve *resolve)
 {
+	if (!resolve)
+		return;
+
 	resolve->ops->destroy(resolve);
 }
 
-- 
2.25.1

[PATCH 2/5] p2p: Free peer->wfd in p2p_peer_free

[Andrew Zaborowski]

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

---
 src/p2p.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/p2p.c b/src/p2p.c
index 5ee887d3..cc24b92f 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -256,6 +256,7 @@ static void p2p_peer_free(void *user_data)
 	struct p2p_peer *peer = user_data;
 
 	scan_bss_free(peer->bss);
+	l_free(peer->wfd);
 	l_free(peer->name);
 	l_free(peer);
 }
-- 
2.25.1

Re: [PATCH 2/5] p2p: Free peer->wfd in p2p_peer_free

[Denis Kenzior]

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

Hi Andrew,

On 9/21/20 2:04 PM, Andrew Zaborowski wrote:
> ---
>   src/p2p.c | 1 +
>   1 file changed, 1 insertion(+)
> 

Patches 2-5 applied, thanks.

Regards,
-Denis

[PATCH 3/5] p2p: Free parsed frame data in p2p_go_negotiation_confirm_cb

[Andrew Zaborowski]

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

---
 src/p2p.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/p2p.c b/src/p2p.c
index cc24b92f..49a57eb0 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -1810,19 +1810,19 @@ static bool p2p_go_negotiation_confirm_cb(const struct mmpdu_header *mpdu,
 	if (info.dialog_token != dev->conn_go_dialog_token) {
 		l_error("GO Negotiation Response dialog token doesn't match");
 		p2p_connect_failed(dev);
-		return true;
+		goto cleanup;
 	}
 
 	if (info.status != P2P_STATUS_SUCCESS) {
 		l_error("GO Negotiation Confirmation status %i", info.status);
 		p2p_connect_failed(dev);
-		return true;
+		goto cleanup;
 	}
 
 	/* Check whether WFD IE is required, validate it if present */
 	if (!p2p_device_validate_conn_wfd(dev, info.wfd, info.wfd_size)) {
 		p2p_connect_failed(dev);
-		return true;
+		goto cleanup;
 	}
 
 	/*
@@ -1833,7 +1833,7 @@ static bool p2p_go_negotiation_confirm_cb(const struct mmpdu_header *mpdu,
 	if (!p2p_device_validate_channel_list(dev, &info.channel_list,
 						&info.operating_channel)) {
 		p2p_connect_failed(dev);
-		return true;
+		goto cleanup;
 	}
 
 	/*
@@ -1852,7 +1852,7 @@ static bool p2p_go_negotiation_confirm_cb(const struct mmpdu_header *mpdu,
 			l_error("Bad operating channel in GO Negotiation "
 				"Confirmation");
 			p2p_connect_failed(dev);
-			return true;
+			goto cleanup;
 		}
 
 		/*
@@ -1873,7 +1873,7 @@ static bool p2p_go_negotiation_confirm_cb(const struct mmpdu_header *mpdu,
 			l_error("Bad operating channel in GO Negotiation "
 				"Confirmation");
 			p2p_connect_failed(dev);
-			return true;
+			goto cleanup;
 		}
 
 		dev->conn_go_oper_freq = frequency;
@@ -1892,6 +1892,8 @@ static bool p2p_go_negotiation_confirm_cb(const struct mmpdu_header *mpdu,
 						p2p_config_timeout_destroy);
 	}
 
+cleanup:
+	p2p_clear_go_negotiation_confirmation(&info);
 	return true;
 }
 
-- 
2.25.1

[PATCH 4/5] p2p: Free response frame payloads

[Andrew Zaborowski]

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

---
 src/p2p.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/p2p.c b/src/p2p.c
index 49a57eb0..7e0fcf4d 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -2169,6 +2169,7 @@ respond:
 					p2p_go_negotiation_resp_err_done, NULL);
 
 	l_debug("GO Negotiation Response sent with status %i", status);
+	l_free(resp_body);
 }
 
 static void p2p_go_negotiation_confirm_done(int error, void *user_data)
@@ -2437,6 +2438,7 @@ static bool p2p_go_negotiation_resp_cb(const struct mmpdu_header *mpdu,
 	p2p_peer_frame_xchg(dev->conn_peer, iov, dev->conn_peer->device_addr,
 				0, 0, 0, false, FRAME_GROUP_CONNECT,
 				p2p_go_negotiation_confirm_done, NULL);
+	l_free(confirm_body);
 
 p2p_free:
 	p2p_clear_go_negotiation_resp(&resp_info);
@@ -2526,6 +2528,7 @@ static void p2p_start_go_negotiation(struct p2p_device *dev)
 				p2p_go_negotiation_req_done,
 				&p2p_frame_go_neg_resp,
 				p2p_go_negotiation_resp_cb, NULL);
+	l_free(req_body);
 }
 
 static bool p2p_provision_disc_resp_cb(const struct mmpdu_header *mpdu,
@@ -2666,6 +2669,7 @@ static void p2p_start_provision_discovery(struct p2p_device *dev)
 				p2p_provision_disc_req_done,
 				&p2p_frame_pd_resp, p2p_provision_disc_resp_cb,
 				NULL);
+	l_free(req_body);
 }
 
 static bool p2p_peer_get_info(struct p2p_peer *peer,
-- 
2.25.1

[PATCH 5/5] ap: Use frame-xchg when sending frames

[Andrew Zaborowski]

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

Convert ap_send_mgmt_frame() to use frame_xchg_start for sending frames,
this fixes among other things the ACK-received checks.

One side effect is that we're no longer sending Probe Responses with the
don't-wait-for-ack flag because frame-xchg doesn't support it, but other
AP implementations don't use that flag either.

Another side-effect is that we do use the no-cck-rate flag
unconditionally, something we may want to fix but would need to add
another parameter to frame-xchg.
---
 src/ap.c | 86 ++++++++++++++++++++++++++------------------------------
 1 file changed, 40 insertions(+), 46 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index da2a3a82..3c4ae907 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -543,36 +543,20 @@ static void ap_wsc_exit_pbc(struct ap_state *ap)
 
 static uint32_t ap_send_mgmt_frame(struct ap_state *ap,
 					const struct mmpdu_header *frame,
-					size_t frame_len, bool wait_ack,
-					l_genl_msg_func_t callback,
+					size_t frame_len,
+					frame_xchg_cb_t callback,
 					void *user_data)
 {
-	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->config->channel,
 						SCAN_BAND_2_4_GHZ);
+	uint64_t wdev_id = netdev_get_wdev_id(ap->netdev);
+	struct iovec iov[2];
 
-	msg = l_genl_msg_new_sized(NL80211_CMD_FRAME, 128 + frame_len);
-
-	l_genl_msg_append_attr(msg, NL80211_ATTR_IFINDEX, 4, &ifindex);
-	l_genl_msg_append_attr(msg, NL80211_ATTR_WIPHY_FREQ, 4, &ch_freq);
-	l_genl_msg_append_attr(msg, NL80211_ATTR_FRAME, frame_len, frame);
-	if (!wait_ack)
-		l_genl_msg_append_attr(msg, NL80211_ATTR_DONT_WAIT_FOR_ACK,
-					0, NULL);
-
-	if (ap->config->no_cck_rates)
-		l_genl_msg_append_attr(msg, NL80211_ATTR_TX_NO_CCK_RATE, 0,
-					NULL);
-
-
-	id = l_genl_family_send(ap->nl80211, msg, callback, user_data, NULL);
-
-	if (!id)
-		l_genl_msg_unref(msg);
-
-	return id;
+	iov[0].iov_base = (void *) frame;
+	iov[0].iov_len = frame_len;
+	iov[1].iov_base = NULL;
+	return frame_xchg_start(wdev_id, iov, ch_freq, 0, 0, 0, 0,
+					callback, user_data, NULL, NULL);
 }
 
 static void ap_start_handshake(struct sta_state *sta, bool use_eapol_start)
@@ -1013,16 +997,19 @@ static bool ap_common_rates(struct l_uintset *ap_rates,
 	return false;
 }
 
-static void ap_success_assoc_resp_cb(struct l_genl_msg *msg, void *user_data)
+static void ap_success_assoc_resp_cb(int err, void *user_data)
 {
 	struct sta_state *sta = user_data;
 	struct ap_state *ap = sta->ap;
 
 	sta->assoc_resp_cmd_id = 0;
 
-	if (l_genl_msg_get_error(msg) < 0) {
-		l_error("AP (Re)Association Response not sent or not ACKed: %i",
-			l_genl_msg_get_error(msg));
+	if (err) {
+		if (err == -ECOMM)
+			l_error("AP (Re)Association Response received no ACK");
+		else
+			l_error("AP (Re)Association Response not sent %s (%i)",
+				strerror(-err), -err);
 
 		/* If we were in State 3 or 4 go to back to State 2 */
 		if (sta->associated)
@@ -1038,11 +1025,14 @@ static void ap_success_assoc_resp_cb(struct l_genl_msg *msg, void *user_data)
 	l_info("AP (Re)Association Response ACK received");
 }
 
-static void ap_fail_assoc_resp_cb(struct l_genl_msg *msg, void *user_data)
+static void ap_fail_assoc_resp_cb(int err, void *user_data)
 {
-	if (l_genl_msg_get_error(msg) < 0)
-		l_error("AP (Re)Association Response with an error status not "
-			"sent or not ACKed: %i", l_genl_msg_get_error(msg));
+	if (err == -ECOMM)
+		l_error("AP (Re)Association Response with an error status "
+			"received no ACK");
+	else if (err)
+		l_error("AP (Re)Association Response with an error status "
+			"not sent: %s (%i)", strerror(-err), -err);
 	else
 		l_info("AP (Re)Association Response with an error status "
 			"delivered OK");
@@ -1051,7 +1041,7 @@ static void ap_fail_assoc_resp_cb(struct l_genl_msg *msg, void *user_data)
 static uint32_t ap_assoc_resp(struct ap_state *ap, struct sta_state *sta,
 				const uint8_t *dest,
 				enum mmpdu_reason_code status_code,
-				bool reassoc, l_genl_msg_func_t callback)
+				bool reassoc, frame_xchg_cb_t callback)
 {
 	const uint8_t *addr = netdev_get_address(ap->netdev);
 	uint8_t mpdu_buf[128];
@@ -1132,7 +1122,7 @@ static uint32_t ap_assoc_resp(struct ap_state *ap, struct sta_state *sta,
 
 send_frame:
 	return ap_send_mgmt_frame(ap, mpdu, resp->ies + ies_len - mpdu_buf,
-					true, callback, sta);
+					callback, sta);
 }
 
 static int ap_parse_supported_rates(struct ie_tlv_iter *iter,
@@ -1606,13 +1596,15 @@ static void ap_process_wsc_probe_req(struct ap_state *ap, const uint8_t *from,
 	}
 }
 
-static void ap_probe_resp_cb(struct l_genl_msg *msg, void *user_data)
+static void ap_probe_resp_cb(int err, void *user_data)
 {
-	if (l_genl_msg_get_error(msg) < 0)
-		l_error("AP Probe Response not sent: %i",
-			l_genl_msg_get_error(msg));
+	if (err == -ECOMM)
+		l_error("AP Probe Response received no ACK");
+	else if (err)
+		l_error("AP Probe Response not sent: %s (%i)",
+			strerror(-err), -err);
 	else
-		l_info("AP Probe Response sent OK");
+		l_info("AP Probe Response delivered OK");
 }
 
 /*
@@ -1723,7 +1715,7 @@ static void ap_probe_req_cb(const struct mmpdu_header *hdr, const void *body,
 					hdr->address_2, resp, sizeof(resp));
 	len += ap_build_beacon_pr_tail(ap, true, resp + len);
 
-	ap_send_mgmt_frame(ap, (struct mmpdu_header *) resp, len, false,
+	ap_send_mgmt_frame(ap, (struct mmpdu_header *) resp, len,
 				ap_probe_resp_cb, NULL);
 }
 
@@ -1757,11 +1749,13 @@ static void ap_disassoc_cb(const struct mmpdu_header *hdr, const void *body,
 	ap_del_station(sta, L_LE16_TO_CPU(disassoc->reason_code), true);
 }
 
-static void ap_auth_reply_cb(struct l_genl_msg *msg, void *user_data)
+static void ap_auth_reply_cb(int err, void *user_data)
 {
-	if (l_genl_msg_get_error(msg) < 0)
-		l_error("AP Authentication frame 2 not sent or not ACKed: %i",
-			l_genl_msg_get_error(msg));
+	if (err == -ECOMM)
+		l_error("AP Authentication frame 2 received no ACK");
+	else if (err)
+		l_error("AP Authentication frame 2 not sent: %s (%i)",
+			strerror(-err), -err);
 	else
 		l_info("AP Authentication frame 2 ACKed by STA");
 }
@@ -1790,7 +1784,7 @@ static void ap_auth_reply(struct ap_state *ap, const uint8_t *dest,
 	auth->transaction_sequence = L_CPU_TO_LE16(2);
 	auth->status = L_CPU_TO_LE16(status_code);
 
-	ap_send_mgmt_frame(ap, mpdu, (uint8_t *) auth + 6 - mpdu_buf, true,
+	ap_send_mgmt_frame(ap, mpdu, (uint8_t *) auth + 6 - mpdu_buf,
 				ap_auth_reply_cb, NULL);
 }
 
-- 
2.25.1

Re: [PATCH 1/5] resolve: Exit methods if resolve is NULL

[Denis Kenzior]

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

Hi Andrew,

On 9/21/20 2:04 PM, Andrew Zaborowski wrote:
> If no resolve method gets configured, the resolve_new() call in
> netconfig returns NULL.  To avoid adding checks netconfig every time
> netconfig->resolve is used, add these checks directly in the resolve_*
> methods.  Fixes the following crash:

Hmm, how exactly is this triggered?  I'm fairly certain I made it so that 
resolve module fails to start if the resolve method is mis-configured.  See 
resolv.c:568 or so.

> 
> src/netconfig.c:netconfig_destroy()
> Aborting (signal 11) [/path/iwd]
> ++++++++ backtrace ++++++++
>   #0  0x7f3ee427f210 in /lib/x86_64-linux-gnu/libc.so.6
>   #1  0x43bcf4 in resolve_revert() at src/resolve.c:85

I wonder why you crash here and not in .add_dns or .add_domain_name

>   #2  0x43ae2d in netconfig_destroy() at src/netconfig.c:1206
>   #3  0x440cd6 in p2p_connection_reset() at src/p2p.c:662
>   #4  0x47200f in process_unicast() at ell/genl.c:979
>   #5  0x46e807 in io_callback() at ell/io.c:126
>   #6  0x46d9bd in l_main_iterate() at ell/main.c:467 (discriminator 2)
>   #7  0x46da8c in l_main_run() at ell/main.c:516
>   #8  0x4047c6 in main() at src/main.c:506
>   #9  0x7f3ee42600b3 in /lib/x86_64-linux-gnu/libc.so.6
> +++++++++++++++++++++++++++
> ---
>   src/resolve.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 

Regards,
-Denis

Re: [PATCH 1/5] resolve: Exit methods if resolve is NULL

[Andrew Zaborowski]

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

Hi Denis,

On Tue, 22 Sep 2020 at 05:32, Denis Kenzior <denkenz@gmail.com> wrote:
> On 9/21/20 2:04 PM, Andrew Zaborowski wrote:
> > If no resolve method gets configured, the resolve_new() call in
> > netconfig returns NULL.  To avoid adding checks netconfig every time
> > netconfig->resolve is used, add these checks directly in the resolve_*
> > methods.  Fixes the following crash:
>
> Hmm, how exactly is this triggered?  I'm fairly certain I made it so that
> resolve module fails to start if the resolve method is mis-configured.  See
> resolv.c:568 or so.

Good point, it's because I don't have
General.EnableNetworkConfiguration or
General.enable_network_configuration so resolve_init() exits early.
I'm still using netconfig from P2P, I guess we shouldn't require
General.EnableNetworkConfiguration to be set for P2P.  P2P doesn't
(usually) have DNS so this should still work but maybe the checks
should be in netconfig.c after all, not sure.

>
> >
> > src/netconfig.c:netconfig_destroy()
> > Aborting (signal 11) [/path/iwd]
> > ++++++++ backtrace ++++++++
> >   #0  0x7f3ee427f210 in /lib/x86_64-linux-gnu/libc.so.6
> >   #1  0x43bcf4 in resolve_revert() at src/resolve.c:85
>
> I wonder why you crash here and not in .add_dns or .add_domain_name

Because the dhcp lease doesn't have DNS information.

Best regards

Re: [PATCH 1/5] resolve: Exit methods if resolve is NULL

[Denis Kenzior]

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

Hi Andrew,

On 9/21/20 11:18 PM, Andrew Zaborowski wrote:
> Hi Denis,
> 
> On Tue, 22 Sep 2020 at 05:32, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 9/21/20 2:04 PM, Andrew Zaborowski wrote:
>>> If no resolve method gets configured, the resolve_new() call in
>>> netconfig returns NULL.  To avoid adding checks netconfig every time
>>> netconfig->resolve is used, add these checks directly in the resolve_*
>>> methods.  Fixes the following crash:
>>
>> Hmm, how exactly is this triggered?  I'm fairly certain I made it so that
>> resolve module fails to start if the resolve method is mis-configured.  See
>> resolv.c:568 or so.
> 
> Good point, it's because I don't have
> General.EnableNetworkConfiguration or
> General.enable_network_configuration so resolve_init() exits early.

Right...

> I'm still using netconfig from P2P, I guess we shouldn't require
> General.EnableNetworkConfiguration to be set for P2P.  P2P doesn't
> (usually) have DNS so this should still work but maybe the checks
> should be in netconfig.c after all, not sure.

I don't think it makes sense to have this asymmetry.  Why is P2P special 
compared to normal station client?

Right now my feeling is that either iwd should handle all ip configuration 
details for all interface types or leave it up to external services such as 
systemd.  So I would think that P2P should honor the EnableNetworkConfiguration 
setting?  Alternatively, we maybe should not enable P2P if 
EnableNetworkConfiguration is not enabled.

> 
>>
>>>
>>> src/netconfig.c:netconfig_destroy()
>>> Aborting (signal 11) [/path/iwd]
>>> ++++++++ backtrace ++++++++
>>>    #0  0x7f3ee427f210 in /lib/x86_64-linux-gnu/libc.so.6
>>>    #1  0x43bcf4 in resolve_revert() at src/resolve.c:85
>>
>> I wonder why you crash here and not in .add_dns or .add_domain_name
> 
> Because the dhcp lease doesn't have DNS information.
> 

Okay, another improvement might be for netconfig_reset to not call into 
resolve_revert if no config options were set in the first place.

> Best regards
> 

Regards,
-Denis

Re: [PATCH 1/5] resolve: Exit methods if resolve is NULL

[Andrew Zaborowski]

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

Hi Denis,

On Tue, 22 Sep 2020 at 16:19, Denis Kenzior <denkenz@gmail.com> wrote:
> On 9/21/20 11:18 PM, Andrew Zaborowski wrote:
> > I'm still using netconfig from P2P, I guess we shouldn't require
> > General.EnableNetworkConfiguration to be set for P2P.  P2P doesn't
> > (usually) have DNS so this should still work but maybe the checks
> > should be in netconfig.c after all, not sure.
>
> I don't think it makes sense to have this asymmetry.  Why is P2P special
> compared to normal station client?

We also don't have any configuration for P2P netconfig because the
spec mandates the use of DHCP.  There's also no legacy to worry about,
so I don't see the point of making DHCP optional for P2P.

Best regards

Re: [PATCH 1/5] resolve: Exit methods if resolve is NULL

[Denis Kenzior]

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

Hi Andrew,

On 9/22/20 9:28 AM, Andrew Zaborowski wrote:
> Hi Denis,
> 
> On Tue, 22 Sep 2020 at 16:19, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 9/21/20 11:18 PM, Andrew Zaborowski wrote:
>>> I'm still using netconfig from P2P, I guess we shouldn't require
>>> General.EnableNetworkConfiguration to be set for P2P.  P2P doesn't
>>> (usually) have DNS so this should still work but maybe the checks
>>> should be in netconfig.c after all, not sure.
>>
>> I don't think it makes sense to have this asymmetry.  Why is P2P special
>> compared to normal station client?
> 
> We also don't have any configuration for P2P netconfig because the
> spec mandates the use of DHCP.  There's also no legacy to worry about,
> so I don't see the point of making DHCP optional for P2P.

That isn't exactly the point I was trying to make.  P2P would always use DHCP or 
network config KDEs, no disagreement there.  But from a user / system 
configuration perspective I don't think this asymmetry is something that one 
would expect or would want.

So if the user has not enabled EnableNetworkConfiguration setting then 
systemd-networkd (for example) expects to perform DHCP for all WiFi interfaces. 
How would you tell systemd-networkd not to configure P2P devices?  They're just 
WiFi as far as it is concerned.

Regards,
-Denis

Re: [PATCH 1/5] resolve: Exit methods if resolve is NULL

[Andrew Zaborowski]

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

Hi Denis,

On Tue, 22 Sep 2020 at 16:46, Denis Kenzior <denkenz@gmail.com> wrote:
> On 9/22/20 9:28 AM, Andrew Zaborowski wrote:
> > On Tue, 22 Sep 2020 at 16:19, Denis Kenzior <denkenz@gmail.com> wrote:
> >> On 9/21/20 11:18 PM, Andrew Zaborowski wrote:
> >>> I'm still using netconfig from P2P, I guess we shouldn't require
> >>> General.EnableNetworkConfiguration to be set for P2P.  P2P doesn't
> >>> (usually) have DNS so this should still work but maybe the checks
> >>> should be in netconfig.c after all, not sure.
> >>
> >> I don't think it makes sense to have this asymmetry.  Why is P2P special
> >> compared to normal station client?
> >
> > We also don't have any configuration for P2P netconfig because the
> > spec mandates the use of DHCP.  There's also no legacy to worry about,
> > so I don't see the point of making DHCP optional for P2P.
>
> That isn't exactly the point I was trying to make.  P2P would always use DHCP or
> network config KDEs, no disagreement there.  But from a user / system
> configuration perspective I don't think this asymmetry is something that one
> would expect or would want.us
>
> So if the user has not enabled EnableNetworkConfiguration setting then
> systemd-networkd (for example) expects to perform DHCP for all WiFi interfaces.
> How would you tell systemd-networkd not to configure P2P devices?  They're just
> WiFi as far as it is concerned.

Ok, I wouldn't have expected that.  I wonder then if we should wait
for the IP configuration to finish before returning from dbus calls so
that the clients can expect consistent behaviour.  And the client
still needs a way to get the peer's IP.

Best regards

Re: [PATCH 1/5] resolve: Exit methods if resolve is NULL

[Denis Kenzior]

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

On 9/22/20 4:34 PM, Andrew Zaborowski wrote:
> Hi Denis,
> 
> On Tue, 22 Sep 2020 at 16:46, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 9/22/20 9:28 AM, Andrew Zaborowski wrote:
>>> On Tue, 22 Sep 2020 at 16:19, Denis Kenzior <denkenz@gmail.com> wrote:
>>>> On 9/21/20 11:18 PM, Andrew Zaborowski wrote:
>>>>> I'm still using netconfig from P2P, I guess we shouldn't require
>>>>> General.EnableNetworkConfiguration to be set for P2P.  P2P doesn't
>>>>> (usually) have DNS so this should still work but maybe the checks
>>>>> should be in netconfig.c after all, not sure.
>>>>
>>>> I don't think it makes sense to have this asymmetry.  Why is P2P special
>>>> compared to normal station client?
>>>
>>> We also don't have any configuration for P2P netconfig because the
>>> spec mandates the use of DHCP.  There's also no legacy to worry about,
>>> so I don't see the point of making DHCP optional for P2P.
>>
>> That isn't exactly the point I was trying to make.  P2P would always use DHCP or
>> network config KDEs, no disagreement there.  But from a user / system
>> configuration perspective I don't think this asymmetry is something that one
>> would expect or would want.us
>>
>> So if the user has not enabled EnableNetworkConfiguration setting then
>> systemd-networkd (for example) expects to perform DHCP for all WiFi interfaces.
>> How would you tell systemd-networkd not to configure P2P devices?  They're just
>> WiFi as far as it is concerned.
> 
> Ok, I wouldn't have expected that.  I wonder then if we should wait
> for the IP configuration to finish before returning from dbus calls so
> that the clients can expect consistent behaviour.  And the client
> still needs a way to get the peer's IP.
> 

I think what you're doing now makes sense (i.e methods succeed once IP 
configuration has finished).  I'm just worried about how we interact with 
services that are not NL80211 aware or ones that don't filter by IFTYPE.  Not 
sure how big of an issue this will turn out to be, but I expect this to be a bit 
of a mess at first.

Perhaps a mitigating strategy might be for netconfig not to invoke resolve_* or 
set default routes if EnableNetworkConfiguration is disabled?  Or maybe just 
disable P2P if EnableNetworkConfiguration is off?

Long term I think that it makes sense for iwd to do everything related to IP 
configuration for P2P.  There might be cases where some external entity might 
still want to manage this (I'm thinking NM and to a lesser extent ConnMan).  But 
until that is actually an issue I wouldn't worry too much about this yet.

Regards,
-Denis

[PATCH 1/5] resolve: Exit methods if resolve is NULL

[Andrew Zaborowski]

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

If no resolve method gets configured, the resolve_new() call in
netconfig returns NULL.  To avoid adding checks netconfig every time
netconfig->resolve is used, add these checks directly in the resolve_*
methods.  Fixes the following crash:

src/netconfig.c:netconfig_destroy()
Aborting (signal 11) [/path/iwd]
++++++++ backtrace ++++++++
 #0  0x7f3ee427f210 in /lib/x86_64-linux-gnu/libc.so.6
 #1  0x43bcf4 in resolve_revert() at src/resolve.c:85
 #2  0x43ae2d in netconfig_destroy() at src/netconfig.c:1206
 #3  0x440cd6 in p2p_connection_reset() at src/p2p.c:662
 #4  0x47200f in process_unicast() at ell/genl.c:979
 #5  0x46e807 in io_callback() at ell/io.c:126
 #6  0x46d9bd in l_main_iterate() at ell/main.c:467 (discriminator 2)
 #7  0x46da8c in l_main_run() at ell/main.c:516
 #8  0x4047c6 in main() at src/main.c:506
 #9  0x7f3ee42600b3 in /lib/x86_64-linux-gnu/libc.so.6
+++++++++++++++++++++++++++
---
 src/resolve.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/resolve.c b/src/resolve.c
index 066e4c87..07eee5a9 100644
--- a/src/resolve.c
+++ b/src/resolve.c
@@ -60,7 +60,7 @@ static inline void _resolve_init(struct resolve *resolve, uint32_t ifindex,
 
 void resolve_add_dns(struct resolve *resolve, uint8_t type, char **dns_list)
 {
-	if (!dns_list || !*dns_list)
+	if (!resolve || !dns_list || !*dns_list)
 		return;
 
 	if (!resolve->ops->add_dns)
@@ -71,7 +71,7 @@ void resolve_add_dns(struct resolve *resolve, uint8_t type, char **dns_list)
 
 void resolve_add_domain_name(struct resolve *resolve, const char *domain_name)
 {
-	if (!domain_name)
+	if (!resolve || !domain_name)
 		return;
 
 	if (!resolve->ops->add_domain_name)
@@ -82,7 +82,7 @@ void resolve_add_domain_name(struct resolve *resolve, const char *domain_name)
 
 void resolve_revert(struct resolve *resolve)
 {
-	if (!resolve->ops->revert)
+	if (!resolve || !resolve->ops->revert)
 		return;
 
 	resolve->ops->revert(resolve);
@@ -90,6 +90,9 @@ void resolve_revert(struct resolve *resolve)
 
 void resolve_free(struct resolve *resolve)
 {
+	if (!resolve)
+		return;
+
 	resolve->ops->destroy(resolve);
 }
 
-- 
2.25.1

[PATCH 2/5] p2p: Free peer->wfd in p2p_peer_free

[Andrew Zaborowski]

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

---
 src/p2p.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/p2p.c b/src/p2p.c
index 5ee887d3..cc24b92f 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -256,6 +256,7 @@ static void p2p_peer_free(void *user_data)
 	struct p2p_peer *peer = user_data;
 
 	scan_bss_free(peer->bss);
+	l_free(peer->wfd);
 	l_free(peer->name);
 	l_free(peer);
 }
-- 
2.25.1