[PATCH v2 2/4] HS20: Add support for configuring frame filters

[Peer, Ilan]

On א', 2016-03-20 at 22:05 +0200, Jouni Malinen wrote:
> On Tue, Mar 08, 2016 at 01:25:41PM +0200, Ilan Peer wrote:
> > When a station has associated to a HS2.0 network, request
> > the driver to do the following, based on the BSS capabilities:
> > 
> > 1. Enable gratuitous ARP filtering
> > 2. Enable unsolicited Neighbor Advertisement filtering
> > 3. Enable GTK filtering if DGAF disabled bit is zero
> > 
> > HS2.0 requires the filtering to start only after an IP
> > address is obtained. However, the configuration is done
> > regardless of obtaining an IP address, so this might
> > not be good enough.
> 
> That "and obtained an IP address" part looks a bit strange.. Why would
> this filtering not be enabled before IP address assignment completes?
> 

That's coming from the HS20 rel. 2 specification, section 6.5 that states
"Once the mobile device has associated to an HS2.0 network and obtained an IP
address, the mobile device ..."

> > diff --git a/wpa_supplicant/hs20_supplicant.c b/wpa_supplicant/hs20_supplicant.c
> > +void hs20_configure_frame_filters(struct wpa_supplicant *wpa_s)
> > +	if (filter)
> > +		wpa_drv_configure_frame_filters(wpa_s, filter);
> 
> Why is this call skipped for filter == 0? Wouldn't it be fine and
> potentially even preferred to clear the setting here if for any reason
> the driver setting was not cleared previously?
> 

This is a bug. Will fix. 

> > diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
> > @@ -751,6 +751,9 @@ void wpa_supplicant_set_state(struct wpa_supplicant *wpa_s,
> >  		wpas_connect_work_done(wpa_s);
> >  		/* Reinitialize normal_scan counter */
> >  		wpa_s->normal_scans = 0;
> > +#ifdef CONFIG_HS20
> > +		hs20_configure_frame_filters(wpa_s);
> > +#endif /* CONFIG_HS20 */
> 
> While this might be closer to the "once .. obtained an IP address",
> wouldn't it be more robust to set these filter before even starting the
> association? There might be a race condition here (even if very unlikely
> to be hit) of IP packets getting exchanged before this call is executed.
> 

The motivation was to adhere to the "IP assignment" requirement. I do not think
that this would be a real issue as the port is not yet authorized, so lower driver
should not pass any IP packets anyway.

> > @@ -816,8 +819,15 @@ void wpa_supplicant_set_state(struct wpa_supplicant *wpa_s,
> >  	if (state == WPA_COMPLETED)
> >  		wpa_supplicant_start_bgscan(wpa_s);
> > -	else if (state < WPA_ASSOCIATED)
> > +	else if (state < WPA_ASSOCIATED) {
> >  		wpa_supplicant_stop_bgscan(wpa_s);
> > +
> > +#ifdef CONFIG_HS20
> > +		/* clear possibly configured frame filters */
> > +		if (old_state == WPA_COMPLETED)
> > +			wpa_drv_configure_frame_filters(wpa_s, 0);
> > +#endif /* CONFIG_HS20 */
> > +	}
> 
> What is this trying to do by requiring state < WPA_ASSOCIATED and
> old_state == WPA_COMPLETED? What if the STA gets disconnected when going
> through rekeying (state is WPA_4WAY_HANDSHAKE or WPA_GROUP_HANDSHAKE)?
> I'm not sure wpa_suppliant_set_state() is really the best location for
> either of these frame filter calls. In other words, these sound more
> like items that should be done when starting an association and when
> processing a disassociation. In addition, there should likely be code to
> clear the configured values when deinitializing the interface.
>  

I'll move the clearing of the flags as you suggested, and also clear these settings
on interface de-init. Do you still want the setting of the flags to be earlier although
it does not adhere to the "IP assignment" requirement?

Thanks,

Ilan.