[PATCH 1/2] ap: Implement P2P GO-side 4-way handshake IP Allocation

[PATCH 1/2] ap: Implement P2P GO-side 4-way handshake IP Allocation

[Andrew Zaborowski]

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

Use the struct handshake_state::support_ip_allocation field already
supported in eapol.c authenticator side to enable the P2P IP Allocation
mechanism in ap.c.  Add the P2P_GROUP_CAP_IP_ALLOCATION bit in P2P group
capabilities to signal the feature is now supported.

There's no harm in enabling this feature in every AP (not just P2P Group
Owner) but the clients won't know whether we support it other than
through that P2P-specific group capability bit.  We might want to start
sending the IP Allocation Requests in our station mode 4-Way Handshakes
just in case the AP is IWD-based or implements the same logic.
Including the extra KDE would add 7 bytes (potentially wasted) in the
handshake frame 2/4.
---
 src/ap.c  | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/p2p.c |  1 +
 2 files changed, 57 insertions(+)

diff --git a/src/ap.c b/src/ap.c
index aa0f2511..cf52e144 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -118,6 +118,7 @@ struct sta_state {
 	struct l_settings *wsc_settings;
 	uint8_t wsc_uuid_e[16];
 	bool wsc_v2;
+	struct l_dhcp_lease *ip_alloc_lease;
 };
 
 struct ap_wsc_pbc_probe_record {
@@ -177,6 +178,10 @@ static void ap_sta_free(void *data)
 	if (sta->gtk_query_cmd_id)
 		l_genl_family_cancel(ap->nl80211, sta->gtk_query_cmd_id);
 
+	if (sta->ip_alloc_lease)
+		l_dhcp_server_release_lease(ap->netconfig_dhcp,
+						sta->ip_alloc_lease);
+
 	ap_stop_handshake(sta);
 
 	l_free(sta);
@@ -830,6 +835,12 @@ static uint32_t ap_send_mgmt_frame(struct ap_state *ap,
 					callback, user_data, NULL, NULL);
 }
 
+#define IP4_FROM_STR(str)						\
+	(__extension__ ({						\
+		struct in_addr ia;					\
+		inet_pton(AF_INET, str, &ia) == 1 ? ntohl(ia.s_addr) : 0; \
+	}))
+
 static void ap_start_handshake(struct sta_state *sta, bool use_eapol_start,
 				const uint8_t *gtk_rsc)
 {
@@ -854,6 +865,34 @@ static void ap_start_handshake(struct sta_state *sta, bool use_eapol_start,
 		handshake_state_set_gtk(sta->hs, sta->ap->gtk,
 					sta->ap->gtk_index, gtk_rsc);
 
+	if (ap->netconfig_dhcp) {
+		L_AUTO_FREE_VAR(char *, lease_addr_str) = NULL;
+		L_AUTO_FREE_VAR(char *, lease_netmask_str) = NULL;
+		char own_addr_str[INET_ADDRSTRLEN];
+
+		if (!sta->ip_alloc_lease)
+			sta->ip_alloc_lease = l_dhcp_server_new_lease(
+							ap->netconfig_dhcp,
+							sta->addr);
+
+		if (!sta->ip_alloc_lease) {
+			l_error("l_dhcp_server_new_lease failed, see "
+				"IWD_DHCP_DEBUG output");
+			ap_stop_handshake(sta);
+			goto error;
+		}
+
+		lease_addr_str = l_dhcp_lease_get_address(sta->ip_alloc_lease);
+		lease_netmask_str =
+			l_dhcp_lease_get_netmask(sta->ip_alloc_lease);
+		l_rtnl_address_get_address(ap->netconfig_addr4, own_addr_str);
+
+		sta->hs->support_ip_allocation = true;
+		sta->hs->client_ip_addr = IP4_FROM_STR(lease_addr_str);
+		sta->hs->subnet_mask = IP4_FROM_STR(lease_netmask_str);
+		sta->hs->go_ip_addr = IP4_FROM_STR(own_addr_str);
+	}
+
 	sta->sm = eapol_sm_new(sta->hs);
 	if (!sta->sm) {
 		ap_stop_handshake(sta);
@@ -877,12 +916,18 @@ static void ap_handshake_event(struct handshake_state *hs,
 		enum handshake_event event, void *user_data, ...)
 {
 	struct sta_state *sta = user_data;
+	struct ap_state *ap = sta->ap;
 	va_list args;
 
 	va_start(args, user_data);
 
 	switch (event) {
 	case HANDSHAKE_EVENT_COMPLETE:
+		if (sta->ip_alloc_lease && !sta->hs->support_ip_allocation)
+			/* Client sent no IP Address Request */
+			l_dhcp_server_release_lease(ap->netconfig_dhcp,
+					l_steal_ptr(sta->ip_alloc_lease));
+
 		ap_new_rsna(sta);
 		break;
 	case HANDSHAKE_EVENT_FAILED:
@@ -2044,6 +2089,14 @@ static void ap_start_failed(struct ap_state *ap, int err)
 	l_free(ap);
 }
 
+static void ap_sta_lease_expired(void *data, void *user_data)
+{
+	struct sta_state *sta = data;
+
+	if (sta->ip_alloc_lease == user_data)
+		sta->ip_alloc_lease = NULL;
+}
+
 static void ap_dhcp_event_cb(struct l_dhcp_server *server,
 				enum l_dhcp_server_event event, void *user_data,
 				const struct l_dhcp_lease *lease)
@@ -2057,6 +2110,9 @@ static void ap_dhcp_event_cb(struct l_dhcp_server *server,
 		break;
 
 	case L_DHCP_SERVER_EVENT_LEASE_EXPIRED:
+		l_queue_foreach(ap->sta_states, ap_sta_lease_expired,
+				(void *) lease);
+
 		ap->ops->handle_event(AP_EVENT_DHCP_LEASE_EXPIRED, lease,
 					ap->user_data);
 		break;
diff --git a/src/p2p.c b/src/p2p.c
index 124372e6..3fb08009 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -1276,6 +1276,7 @@ static void p2p_group_start(struct p2p_device *dev)
 
 	dev->capability.group_caps |= P2P_GROUP_CAP_GO;
 	dev->capability.group_caps |= P2P_GROUP_CAP_GROUP_FORMATION;
+	dev->capability.group_caps |= P2P_GROUP_CAP_IP_ALLOCATION;
 
 	dev->group = ap_start(dev->conn_netdev, config, &p2p_go_ops, NULL, dev);
 	l_settings_free(config);
-- 
2.30.2

[PATCH 2/2] autotests: Test GO-side IP Allocation in testP2P

[Andrew Zaborowski]

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

Check if our wpa_supplicant P2P Client has received the allocated
Clieant IP/netmask/GO IP values we sent in the 4-Way Handshake.
---
 autotests/testP2P/connection_test.py | 4 ++++
 autotests/util/wpas.py               | 1 +
 2 files changed, 5 insertions(+)

diff --git a/autotests/testP2P/connection_test.py b/autotests/testP2P/connection_test.py
index 22871547..e145a295 100644
--- a/autotests/testP2P/connection_test.py
+++ b/autotests/testP2P/connection_test.py
@@ -91,6 +91,7 @@ class Test(unittest.TestCase):
 
         wd.wait_for_object_condition(wpas, 'obj.p2p_group is not None', max_wait=3)
         peer_ifname = wpas.p2p_group['ifname']
+        self.assertEqual(wpas.p2p_group['role'], 'GO' if not go else 'client')
 
         if not go:
             ctx.start_process(['ifconfig', peer_ifname, '192.168.1.20', 'netmask', '255.255.255.0'], wait=True)
@@ -102,6 +103,9 @@ class Test(unittest.TestCase):
             client = wpas.p2p_clients[request['peer_iface']]
             self.assertEqual(client['p2p_dev_addr'], wpas_peer['p2p_dev_addr'])
         else:
+            self.assertEqual(wpas.p2p_group['ip_addr'], '192.168.1.2')
+            self.assertEqual(wpas.p2p_group['ip_mask'], '255.255.255.240')
+            self.assertEqual(wpas.p2p_group['go_ip_addr'], '192.168.1.1')
             dhcp = ctx.start_process(['dhclient', '-v', '-d', '--no-pid', '-cf', '/dev/null', '-lf', '/tmp/dhcp.leases',
                 '-sf', '/tmp/dhclient-script', peer_ifname])
             self.dhcp = dhcp
diff --git a/autotests/util/wpas.py b/autotests/util/wpas.py
index 1758eede..8fb725f8 100644
--- a/autotests/util/wpas.py
+++ b/autotests/util/wpas.py
@@ -159,6 +159,7 @@ class Wpas:
         elif event['event'] == 'P2P-GROUP-STARTED':
             event.pop('event')
             event['ifname'] = event.pop('arg1')
+            event['role'] = event.pop('arg2')
             self.p2p_group = event
         elif event['event'] == 'P2P-GROUP-REMOVED':
             self.p2p_group = None
-- 
2.30.2

Re: [PATCH 1/2] ap: Implement P2P GO-side 4-way handshake IP Allocation

[Denis Kenzior]

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

Hi Andrew,

On 7/9/21 6:49 AM, Andrew Zaborowski wrote:
> Use the struct handshake_state::support_ip_allocation field already
> supported in eapol.c authenticator side to enable the P2P IP Allocation
> mechanism in ap.c.  Add the P2P_GROUP_CAP_IP_ALLOCATION bit in P2P group
> capabilities to signal the feature is now supported.
> 
> There's no harm in enabling this feature in every AP (not just P2P Group
> Owner) but the clients won't know whether we support it other than
> through that P2P-specific group capability bit.  We might want to start
> sending the IP Allocation Requests in our station mode 4-Way Handshakes
> just in case the AP is IWD-based or implements the same logic.
> Including the extra KDE would add 7 bytes (potentially wasted) in the
> handshake frame 2/4.

I fully agree with the last sentence here, we should be doing this by default. 
Not sure this last sentence belongs in the commit description though.

> ---
>   src/ap.c  | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/p2p.c |  1 +
>   2 files changed, 57 insertions(+)
> 
> diff --git a/src/ap.c b/src/ap.c
> index aa0f2511..cf52e144 100644
> --- a/src/ap.c
> +++ b/src/ap.c
> @@ -118,6 +118,7 @@ struct sta_state {
>   	struct l_settings *wsc_settings;
>   	uint8_t wsc_uuid_e[16];
>   	bool wsc_v2;
> +	struct l_dhcp_lease *ip_alloc_lease;
>   };
>   
>   struct ap_wsc_pbc_probe_record {
> @@ -177,6 +178,10 @@ static void ap_sta_free(void *data)
>   	if (sta->gtk_query_cmd_id)
>   		l_genl_family_cancel(ap->nl80211, sta->gtk_query_cmd_id);
>   
> +	if (sta->ip_alloc_lease)
> +		l_dhcp_server_release_lease(ap->netconfig_dhcp,
> +						sta->ip_alloc_lease);
> +

Why are we tracking the lease past the handshake phase?  It seems a little 
asymmetric since we're not tracking the lease obtained from the 'other' path 
(i.e. normal DHCP request)

>   	ap_stop_handshake(sta);
>   
>   	l_free(sta);
> @@ -830,6 +835,12 @@ static uint32_t ap_send_mgmt_frame(struct ap_state *ap,
>   					callback, user_data, NULL, NULL);
>   }
>   
> +#define IP4_FROM_STR(str)						\
> +	(__extension__ ({						\
> +		struct in_addr ia;					\
> +		inet_pton(AF_INET, str, &ia) == 1 ? ntohl(ia.s_addr) : 0; \

Why the ntohl?  Why is handshake_state storing those parameters in host order 
anyway?

> +	}))
> +
>   static void ap_start_handshake(struct sta_state *sta, bool use_eapol_start,
>   				const uint8_t *gtk_rsc)
>   {
> @@ -854,6 +865,34 @@ static void ap_start_handshake(struct sta_state *sta, bool use_eapol_start,
>   		handshake_state_set_gtk(sta->hs, sta->ap->gtk,
>   					sta->ap->gtk_index, gtk_rsc);
>   
> +	if (ap->netconfig_dhcp) {
> +		L_AUTO_FREE_VAR(char *, lease_addr_str) = NULL;
> +		L_AUTO_FREE_VAR(char *, lease_netmask_str) = NULL;
> +		char own_addr_str[INET_ADDRSTRLEN];
> +
> +		if (!sta->ip_alloc_lease)
> +			sta->ip_alloc_lease = l_dhcp_server_new_lease(
> +							ap->netconfig_dhcp,
> +							sta->addr);
> +
> +		if (!sta->ip_alloc_lease) {
> +			l_error("l_dhcp_server_new_lease failed, see "
> +				"IWD_DHCP_DEBUG output");
> +			ap_stop_handshake(sta);

Really?  Can't we just go on without sending the IP Alloc KDE?

> +			goto error;
> +		}
> +
> +		lease_addr_str = l_dhcp_lease_get_address(sta->ip_alloc_lease);
> +		lease_netmask_str =
> +			l_dhcp_lease_get_netmask(sta->ip_alloc_lease);
> +		l_rtnl_address_get_address(ap->netconfig_addr4, own_addr_str);
> +
> +		sta->hs->support_ip_allocation = true;
> +		sta->hs->client_ip_addr = IP4_FROM_STR(lease_addr_str);
> +		sta->hs->subnet_mask = IP4_FROM_STR(lease_netmask_str);
> +		sta->hs->go_ip_addr = IP4_FROM_STR(own_addr_str);
> +	}
> +
>   	sta->sm = eapol_sm_new(sta->hs);
>   	if (!sta->sm) {
>   		ap_stop_handshake(sta);
> @@ -877,12 +916,18 @@ static void ap_handshake_event(struct handshake_state *hs,
>   		enum handshake_event event, void *user_data, ...)
>   {
>   	struct sta_state *sta = user_data;
> +	struct ap_state *ap = sta->ap;
>   	va_list args;
>   
>   	va_start(args, user_data);
>   
>   	switch (event) {
>   	case HANDSHAKE_EVENT_COMPLETE:
> +		if (sta->ip_alloc_lease && !sta->hs->support_ip_allocation)
> +			/* Client sent no IP Address Request */
> +			l_dhcp_server_release_lease(ap->netconfig_dhcp,
> +					l_steal_ptr(sta->ip_alloc_lease));
> +

I'm not sure about this part.  Is there any harm in just leaving the lease be? 
Shouldn't l_dhcp_server just select the same lease if the client tries 
re-running DHCP?  You could also try to mess with the lease timeout in case 
you're worried about clients connecting and not running DHCP...

>   		ap_new_rsna(sta);
>   		break;
>   	case HANDSHAKE_EVENT_FAILED:

What do you do with the lease in the failure case?

> @@ -2044,6 +2089,14 @@ static void ap_start_failed(struct ap_state *ap, int err)
>   	l_free(ap);
>   }
>   
> +static void ap_sta_lease_expired(void *data, void *user_data)
> +{
> +	struct sta_state *sta = data;
> +
> +	if (sta->ip_alloc_lease == user_data)
> +		sta->ip_alloc_lease = NULL;
> +}
> +
>   static void ap_dhcp_event_cb(struct l_dhcp_server *server,
>   				enum l_dhcp_server_event event, void *user_data,
>   				const struct l_dhcp_lease *lease)
> @@ -2057,6 +2110,9 @@ static void ap_dhcp_event_cb(struct l_dhcp_server *server,
>   		break;
>   
>   	case L_DHCP_SERVER_EVENT_LEASE_EXPIRED:
> +		l_queue_foreach(ap->sta_states, ap_sta_lease_expired,
> +				(void *) lease);
> +

If you're not tracking leases past handshake time, then this part would be unneeded?

>   		ap->ops->handle_event(AP_EVENT_DHCP_LEASE_EXPIRED, lease,
>   					ap->user_data);
>   		break;
> diff --git a/src/p2p.c b/src/p2p.c
> index 124372e6..3fb08009 100644
> --- a/src/p2p.c
> +++ b/src/p2p.c
> @@ -1276,6 +1276,7 @@ static void p2p_group_start(struct p2p_device *dev)
>   
>   	dev->capability.group_caps |= P2P_GROUP_CAP_GO;
>   	dev->capability.group_caps |= P2P_GROUP_CAP_GROUP_FORMATION;
> +	dev->capability.group_caps |= P2P_GROUP_CAP_IP_ALLOCATION;
>   
>   	dev->group = ap_start(dev->conn_netdev, config, &p2p_go_ops, NULL, dev);
>   	l_settings_free(config);
> 

Regards,
-Denis

Re: [PATCH 1/2] ap: Implement P2P GO-side 4-way handshake IP Allocation

[Andrew Zaborowski]

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

Hi Denis,

On Fri, 9 Jul 2021 at 17:40, Denis Kenzior <denkenz@gmail.com> wrote:
> On 7/9/21 6:49 AM, Andrew Zaborowski wrote:
> > @@ -177,6 +178,10 @@ static void ap_sta_free(void *data)
> >       if (sta->gtk_query_cmd_id)
> >               l_genl_family_cancel(ap->nl80211, sta->gtk_query_cmd_id);
> >
> > +     if (sta->ip_alloc_lease)
> > +             l_dhcp_server_release_lease(ap->netconfig_dhcp,
> > +                                             sta->ip_alloc_lease);
> > +
>
> Why are we tracking the lease past the handshake phase?  It seems a little
> asymmetric since we're not tracking the lease obtained from the 'other' path
> (i.e. normal DHCP request)

The idea was to use the same lease object for new associations but
there's little benefit from this so yeah, for simplicity I'll just
drop the reference after the handshake (if not immediately after use
-- but I think not -- we need to release the lease if the handshake
times out to make DoSing the DHCP server more difficult.)

>
> >       ap_stop_handshake(sta);
> >
> >       l_free(sta);
> > @@ -830,6 +835,12 @@ static uint32_t ap_send_mgmt_frame(struct ap_state *ap,
> >                                       callback, user_data, NULL, NULL);
> >   }
> >
> > +#define IP4_FROM_STR(str)                                            \
> > +     (__extension__ ({                                               \
> > +             struct in_addr ia;                                      \
> > +             inet_pton(AF_INET, str, &ia) == 1 ? ntohl(ia.s_addr) : 0; \
>
> Why the ntohl?  Why is handshake_state storing those parameters in host order
> anyway?

I guess at this point it's just because why not.  We can use either
order.  I still think it would be good to use a typedef if we want to
store a network byte-order value, like struct in_addr uses in_addr_t.

>
> > +     }))
> > +
> >   static void ap_start_handshake(struct sta_state *sta, bool use_eapol_start,
> >                               const uint8_t *gtk_rsc)
> >   {
> > @@ -854,6 +865,34 @@ static void ap_start_handshake(struct sta_state *sta, bool use_eapol_start,
> >               handshake_state_set_gtk(sta->hs, sta->ap->gtk,
> >                                       sta->ap->gtk_index, gtk_rsc);
> >
> > +     if (ap->netconfig_dhcp) {
> > +             L_AUTO_FREE_VAR(char *, lease_addr_str) = NULL;
> > +             L_AUTO_FREE_VAR(char *, lease_netmask_str) = NULL;
> > +             char own_addr_str[INET_ADDRSTRLEN];
> > +
> > +             if (!sta->ip_alloc_lease)
> > +                     sta->ip_alloc_lease = l_dhcp_server_new_lease(
> > +                                                     ap->netconfig_dhcp,
> > +                                                     sta->addr);
> > +
> > +             if (!sta->ip_alloc_lease) {
> > +                     l_error("l_dhcp_server_new_lease failed, see "
> > +                             "IWD_DHCP_DEBUG output");
> > +                     ap_stop_handshake(sta);
>
> Really?  Can't we just go on without sending the IP Alloc KDE?

This is basically an L_WARN for an internal error but I can try to
handle it anyway.

>
> > +                     goto error;
> > +             }
> > +
> > +             lease_addr_str = l_dhcp_lease_get_address(sta->ip_alloc_lease);
> > +             lease_netmask_str =
> > +                     l_dhcp_lease_get_netmask(sta->ip_alloc_lease);
> > +             l_rtnl_address_get_address(ap->netconfig_addr4, own_addr_str);
> > +
> > +             sta->hs->support_ip_allocation = true;
> > +             sta->hs->client_ip_addr = IP4_FROM_STR(lease_addr_str);
> > +             sta->hs->subnet_mask = IP4_FROM_STR(lease_netmask_str);
> > +             sta->hs->go_ip_addr = IP4_FROM_STR(own_addr_str);
> > +     }
> > +
> >       sta->sm = eapol_sm_new(sta->hs);
> >       if (!sta->sm) {
> >               ap_stop_handshake(sta);
> > @@ -877,12 +916,18 @@ static void ap_handshake_event(struct handshake_state *hs,
> >               enum handshake_event event, void *user_data, ...)
> >   {
> >       struct sta_state *sta = user_data;
> > +     struct ap_state *ap = sta->ap;
> >       va_list args;
> >
> >       va_start(args, user_data);
> >
> >       switch (event) {
> >       case HANDSHAKE_EVENT_COMPLETE:
> > +             if (sta->ip_alloc_lease && !sta->hs->support_ip_allocation)
> > +                     /* Client sent no IP Address Request */
> > +                     l_dhcp_server_release_lease(ap->netconfig_dhcp,
> > +                                     l_steal_ptr(sta->ip_alloc_lease));
> > +
>
> I'm not sure about this part.  Is there any harm in just leaving the lease be?
> Shouldn't l_dhcp_server just select the same lease if the client tries
> re-running DHCP?

Yeah, I think it would re-use the memory (after memzeroing it) and the
IP.  Ideally we shouldn't assume this about the DHCP implementation,
is there any reason to not release the lease here?

> You could also try to mess with the lease timeout in case
> you're worried about clients connecting and not running DHCP...

Thinking about this there may also be future UIs that will display
active/expired DHCP leases like a lot of APs do.  Ideally the lease
list would really reflect the leases requested/acked.

Speaking of this I wonder if instead of relying on only the lease
timeouts for AP clients that have disassociated, instead ap.c should
notify the l_dhcp_server to automatically move the client's lease to
the expired list.  If the client reconnects before the IP pool becomes
full it can still re-use the lease, assuming it's using a permanent
BSSID.  I wonder if automatic reconnects by clients that randomize
BSSIDs could dry out to the IP pool too quickly.

>
> >               ap_new_rsna(sta);
> >               break;
> >       case HANDSHAKE_EVENT_FAILED:
>
> What do you do with the lease in the failure case?

We end up freeing it in ap_sta_free().

>
> > @@ -2044,6 +2089,14 @@ static void ap_start_failed(struct ap_state *ap, int err)
> >       l_free(ap);
> >   }
> >
> > +static void ap_sta_lease_expired(void *data, void *user_data)
> > +{
> > +     struct sta_state *sta = data;
> > +
> > +     if (sta->ip_alloc_lease == user_data)
> > +             sta->ip_alloc_lease = NULL;
> > +}
> > +
> >   static void ap_dhcp_event_cb(struct l_dhcp_server *server,
> >                               enum l_dhcp_server_event event, void *user_data,
> >                               const struct l_dhcp_lease *lease)
> > @@ -2057,6 +2110,9 @@ static void ap_dhcp_event_cb(struct l_dhcp_server *server,
> >               break;
> >
> >       case L_DHCP_SERVER_EVENT_LEASE_EXPIRED:
> > +             l_queue_foreach(ap->sta_states, ap_sta_lease_expired,
> > +                             (void *) lease);
> > +
>
> If you're not tracking leases past handshake time, then this part would be unneeded?

Yep, then I guess we can skip this but we might as well keep it to
avoid future surprises.

Best regards

Re: [PATCH 1/2] ap: Implement P2P GO-side 4-way handshake IP Allocation

[Denis Kenzior]

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

Hi Andrew,

>>> +#define IP4_FROM_STR(str)                                            \
>>> +     (__extension__ ({                                               \
>>> +             struct in_addr ia;                                      \
>>> +             inet_pton(AF_INET, str, &ia) == 1 ? ntohl(ia.s_addr) : 0; \
>>
>> Why the ntohl?  Why is handshake_state storing those parameters in host order
>> anyway?
> 
> I guess at this point it's just because why not.  We can use either

So I really don't agree with such point of view.  IP addresses are usually 
assumed to be stored in network order.  Consistency is king.  We should always 
be consistent unless there's a good reason not to be, and then you want to keep 
that inconsistency to a minimum.

> order.  I still think it would be good to use a typedef if we want to
> store a network byte-order value, like struct in_addr uses in_addr_t.
> 

We could.  We also have __be32/16 types.  But all of these are just annotations. 
  They're still not that helpful compared to just being consistent.

>>> @@ -877,12 +916,18 @@ static void ap_handshake_event(struct handshake_state *hs,
>>>                enum handshake_event event, void *user_data, ...)
>>>    {
>>>        struct sta_state *sta = user_data;
>>> +     struct ap_state *ap = sta->ap;
>>>        va_list args;
>>>
>>>        va_start(args, user_data);
>>>
>>>        switch (event) {
>>>        case HANDSHAKE_EVENT_COMPLETE:
>>> +             if (sta->ip_alloc_lease && !sta->hs->support_ip_allocation)
>>> +                     /* Client sent no IP Address Request */
>>> +                     l_dhcp_server_release_lease(ap->netconfig_dhcp,
>>> +                                     l_steal_ptr(sta->ip_alloc_lease));
>>> +
>>
>> I'm not sure about this part.  Is there any harm in just leaving the lease be?
>> Shouldn't l_dhcp_server just select the same lease if the client tries
>> re-running DHCP?
> 
> Yeah, I think it would re-use the memory (after memzeroing it) and the
> IP.  Ideally we shouldn't assume this about the DHCP implementation,
> is there any reason to not release the lease here?
> 

I was wondering about potential race conditions here.  By the time we receive 
message 4, the client might already be running dhcp...  Would there be any 
issues with just hard-pulling a lease like this?

>> You could also try to mess with the lease timeout in case
>> you're worried about clients connecting and not running DHCP...
> 
> Thinking about this there may also be future UIs that will display
> active/expired DHCP leases like a lot of APs do.  Ideally the lease
> list would really reflect the leases requested/acked.

Maybe allocate the lease like we received a DISCOVER?  And only confirm it once 
the handshake is done and the IP Allocation KDE was used?

> 
> Speaking of this I wonder if instead of relying on only the lease
> timeouts for AP clients that have disassociated, instead ap.c should
> notify the l_dhcp_server to automatically move the client's lease to
> the expired list.  If the client reconnects before the IP pool becomes
> full it can still re-use the lease, assuming it's using a permanent
> BSSID.  I wonder if automatic reconnects by clients that randomize
> BSSIDs could dry out to the IP pool too quickly.

I think that would be fine for the type of use cases we're targeting.

Regards,
-Denis

Re: [PATCH 1/2] ap: Implement P2P GO-side 4-way handshake IP Allocation

[Andrew Zaborowski]

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

Hi Denis,

On Fri, 9 Jul 2021 at 20:51, Denis Kenzior <denkenz@gmail.com> wrote:
> >>> +#define IP4_FROM_STR(str)                                            \
> >>> +     (__extension__ ({                                               \
> >>> +             struct in_addr ia;                                      \
> >>> +             inet_pton(AF_INET, str, &ia) == 1 ? ntohl(ia.s_addr) : 0; \
> >>
> >> Why the ntohl?  Why is handshake_state storing those parameters in host order
> >> anyway?
> >
> > I guess at this point it's just because why not.  We can use either
>
> So I really don't agree with such point of view.  IP addresses are usually
> assumed to be stored in network order.  Consistency is king.  We should always
> be consistent unless there's a good reason not to be, and then you want to keep
> that inconsistency to a minimum.

I can switch these three fields in handshake_state to network
byte-order but ftr IWD itself doesn't store raw IPs as literal
uint32_t's except in ap/ip-pool/p2p all of which are host byte-order
now (you could say there's consistency).

>
> > order.  I still think it would be good to use a typedef if we want to
> > store a network byte-order value, like struct in_addr uses in_addr_t.
> >
>
> We could.  We also have __be32/16 types.  But all of these are just annotations.
>   They're still not that helpful compared to just being consistent.
>
> >>> @@ -877,12 +916,18 @@ static void ap_handshake_event(struct handshake_state *hs,
> >>>                enum handshake_event event, void *user_data, ...)
> >>>    {
> >>>        struct sta_state *sta = user_data;
> >>> +     struct ap_state *ap = sta->ap;
> >>>        va_list args;
> >>>
> >>>        va_start(args, user_data);
> >>>
> >>>        switch (event) {
> >>>        case HANDSHAKE_EVENT_COMPLETE:
> >>> +             if (sta->ip_alloc_lease && !sta->hs->support_ip_allocation)
> >>> +                     /* Client sent no IP Address Request */
> >>> +                     l_dhcp_server_release_lease(ap->netconfig_dhcp,
> >>> +                                     l_steal_ptr(sta->ip_alloc_lease));
> >>> +
> >>
> >> I'm not sure about this part.  Is there any harm in just leaving the lease be?
> >> Shouldn't l_dhcp_server just select the same lease if the client tries
> >> re-running DHCP?
> >
> > Yeah, I think it would re-use the memory (after memzeroing it) and the
> > IP.  Ideally we shouldn't assume this about the DHCP implementation,
> > is there any reason to not release the lease here?
> >
>
> I was wondering about potential race conditions here.  By the time we receive
> message 4, the client might already be running dhcp...  Would there be any
> issues with just hard-pulling a lease like this?

Looks like there could be a race condition indeed.  AFAIK DHCP can't
start before we send the SET_STATION to authorize the "data port" but
the HANDSHAKE_EVENT_COMPLETE is signalled inside the SET_STATION
callback (netdev_set_station_cb) and I guess l_dhcp_server could
receive a frame callback after we sent the SET_STATION command but
before we got the callback.

>
> >> You could also try to mess with the lease timeout in case
> >> you're worried about clients connecting and not running DHCP...
> >
> > Thinking about this there may also be future UIs that will display
> > active/expired DHCP leases like a lot of APs do.  Ideally the lease
> > list would really reflect the leases requested/acked.
>
> Maybe allocate the lease like we received a DISCOVER?  And only confirm it once
> the handshake is done and the IP Allocation KDE was used?

This might work, I actually considered using two methods instead of
just l_dhcp_server_new_lease.  We'd need to start tracking the offered
lease expiry times though.  Right now we set a 5s timeout for offered
leases (lease->lifetime = OFFER_TIME) but
dhcp-server.c:set_next_expire_timer() disables all timeouts whenever
there's an offered lease so they're not tracked.

Another option would be to not re-use the l_dhcp_lease pointers in
get_lease(), so when the client requests a lease through DHCP the
other lease would be automatically released and an event emitted.
Even calling l_dhcp_server_release_lease() on it would not interfere
with anything then.

Best regards

Re: [PATCH 1/2] ap: Implement P2P GO-side 4-way handshake IP Allocation

[Andrew Zaborowski]

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

On Sat, 10 Jul 2021 at 02:52, Andrew Zaborowski
<andrew.zaborowski@intel.com> wrote:
> We'd need to start tracking the offered
> lease expiry times though.  Right now we set a 5s timeout for offered
> leases (lease->lifetime = OFFER_TIME) but
> dhcp-server.c:set_next_expire_timer() disables all timeouts whenever
> there's an offered lease so they're not tracked.

I misread that check, it actually disables the timeout if there are
*only* "offered" leases on the list.  This still seems wrong.  We
probably also need to skip the LEASE_EXPIRED event in
lease_expired_cb() for "offered" leases because we don't emit
NEW_LEASE for these.

Best regards