Re: [PATCH v4 05/15] ft: netdev: prep for FT isolation into ft.c

[Denis Kenzior <denkenz@gmail.com>]

Hi James,

On 9/21/22 17:31, James Prestwood wrote:
> Currently netdev handles caching FT auth information and uses FT
> parsers/auth-proto to manage the protocol. This sets up to remove
> this state machine from netdev and isolate it into ft.c.
> 
> This does not break the existing auth-proto (hence the slight
> modifications, which will be removed soon).
> 
> Eventually the auth-proto will be removed from FT entirely, replaced
> just by an FT state machine, similar to how EAPoL works (netdev hooks
> to TX/RX frames).
> ---
>   src/ft.c     | 312 ++++++++++++++++++++++++++++++++++++++++++++++++---
>   src/ft.h     |  22 +++-
>   src/netdev.c |  28 +++--
>   3 files changed, 329 insertions(+), 33 deletions(-)
> 

<snip>

> @@ -45,6 +69,9 @@ struct ft_sm {
>   	void *user_data;
>   
>   	bool over_ds : 1;
> +
> +	uint8_t prev_bssid[6];
> +	struct l_queue *ft_auths;

Is ft_auths still needed?

>   };
>   
>   /*

<snip>

> +int ft_associate(uint32_t ifindex, const uint8_t *addr)
> +{
> +	struct netdev *netdev = netdev_find(ifindex);
> +	struct handshake_state *hs = netdev_get_handshake(netdev);
> +	struct ft_info *info;
> +	int ret;
> +
> +	/*
> +	 * TODO: Since FT-over-DS is done early, before the time of roaming, it
> +	 *       may end up that a completely new BSS is the best candidate and
> +	 *       we haven't yet authenticated. We could actually authenticate
> +	 *       at this point, but for now just assume the caller will choose
> +	 *       a different BSS.
> +	 */
> +	info = ft_info_find(ifindex, addr);
> +	if (!info)
> +		return -ENOENT;
> +
> +	ft_prepare_handshake(info, hs);
> +
> +	ret = ft_tx_reassociate(ifindex, info->frequency, info->prev_bssid);
> +
> +	/* After this no previous auths will be valid */
> +	l_queue_clear(info_list, ft_info_destroy);

So why are we clearing the entire list?  Shouldn't we be clearing it only for 
the ifindex in question?

> +
> +	return ret;
> +}
> +
> +void ft_reset(uint32_t ifindex)

I think a more descriptive name is required.  Like 
ft_purge_authentications_for_ifindex() or something? :)

> +{
> +	struct ft_info *info;
> +
> +	while ((info = ft_info_find(ifindex, NULL))) {
> +		l_queue_remove(info_list, info);

This looks like a O(n^2) loop instead of doing the purge in linear time?

> +		ft_info_destroy(info);
> +	}
> +}
> +
> +static int ft_init(void)
> +{
> +	info_list = l_queue_new();
> +
> +	return 0;
> +}
> +
> +static void ft_exit(void)
> +{
> +	if (!l_queue_isempty(info_list))
> +		l_warn("stale FT info objects found!");
> +
> +	l_queue_destroy(info_list, ft_info_destroy);
> +}
> +
> +IWD_MODULE(ft, ft_init, ft_exit);

Does netdev need an IWD_MODULE_DEPENDS on ft now?

Regards,
-Denis