Re: [PATCH v2 3/4] ap: support PTK rekeys

[Denis Kenzior <denkenz@gmail.com>]

Hi James,

On 1/12/23 13:32, James Prestwood wrote:
> This adds support for rekeys to AP mode. A single timer is used and
> reset to the next station needing a rekey. A default rekey timer of
> 600 seconds is used unless the profile sets a timeout.
> ---
>   src/ap.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 114 insertions(+)
> 

<snip>

> @@ -439,6 +452,89 @@ static void ap_del_station(struct sta_state *sta, uint16_t reason,
>   
>   		ap_event_done(ap, prev);
>   	}
> +
> +	ap_reset_rekey_timeout(ap);

Shouldn't you be cleaning up the timeout here?

> +}
> +
> +static void ap_start_rekey(struct ap_state *ap, struct sta_state *sta)
> +{
> +	l_debug("Rekey STA "MAC, MAC_STR(sta->addr));
> +
> +	eapol_start(sta->sm);
> +}
> +
> +static void ap_rekey_timeout(struct l_timeout *timeout, void *user_data)
> +{
> +	struct ap_state *ap = user_data;
> +
> +	l_timeout_remove(timeout);
> +
> +	ap_reset_rekey_timeout(ap);
> +}
> +
> +/*
> + * Used to initiate any rekeys which are due and reset the rekey timer to the
> + * next soonest station needing a rekey.
> + *
> + * TODO: Could adapt this to also take into account the next GTK rekey and
> + * service that as well. But GTK rekeys are not yet supported in AP mode.
> + */
> +static void ap_reset_rekey_timeout(struct ap_state *ap)
> +{
> +	const struct l_queue_entry *e;
> +	uint64_t now = l_time_now();
> +	uint64_t next = 0;
> +
> +	if (!ap->rekey_time)
> +		return;
> +
> +	/* Find the station(s) that need a rekey and start it */
> +	for (e = l_queue_get_entries(ap->sta_states); e; e = e->next) {
> +		struct sta_state *sta = e->data;
> +
> +		if (!sta->associated || !sta->rsna)
> +			continue;

Would checking sta->rekey_time == 0 also be worthwhile?  For stas that haven't 
authenticated yet?

> +
> +		if (l_time_before(now, sta->rekey_time)) {
> +			uint64_t diff = l_time_diff(now, sta->rekey_time);
> +
> +			/* Finding the next rekey time */
> +			if (next < diff)
> +				next = diff;
> +
> +			continue;

Ok, so you try to find the next soonest (absolute) rekey_time to schedule the 
next timeout.  Might be easier to just set next to ~0 and loop over the stations 
using l_time_before(sta->rekey_time, next), setting next as needed.

> +		}
> +
> +		ap_start_rekey(ap, sta);

And looks like this starts a rekey for any stations that we somehow missed?  How 
does this happen?

> +	}
> +
> +	/*
> +	 * Set the next rekey to the station needing it the soonest, or NULL
> +	 * if a single station and wait until the rekey is complete to reset
> +	 * the timer.
> +	 */
> +	if (next)
> +		ap->rekey_timeout = l_timeout_create(l_time_to_secs(next),
> +						ap_rekey_timeout, ap, NULL);
> +	else
> +		ap->rekey_timeout = NULL;

Are you sure the rekey_timeout is destroyed here?

Might be easier to use l_timeout_modify instead of creating/destroying it all 
the time?

> +}
> +
Regards,
-Denis