[PATCHv2] hostapd: reload bss mac files on SIGUSR2

[Antonio Quartulli]

On Sun, Jun 17, 2012 at 11:44:54 +0300, Jouni Malinen wrote:
> On Mon, Jun 11, 2012 at 12:44:30AM +0200, Antonio Quartulli wrote:
> > This patch allows the administrator to modify the provided MAC list for the
> > BSS ACL mechanism at runtime. In the current implementation no runtime change
> > is possible: if the MAC list is modified, hostapd has to be restart so
> > disconnecting all the current clients.
> > 
> > After manually modify the MAC files, the USR2 signal will make hostapd reload
> > them and possibly disconnect new not allowed stations.
> 
> Why is this needed? Doesn't SIGHUP already reload the files? Using a
> USR2 signal would require more justification since it is the only
> generic signal that remains available and I'm not sure whether this
> functionality is really in need of a global signal (i.e., per-interface
> ctrl_iface would be cleaner).

Clients disconenctions is what I exactly would like to avoid.
If the mac-list is modified such that I added a new allowed station,
why should I disconnect all the other while updating the list?
Imagine hostapd is offering internet connectivity in a public hotspot...If I
were a client I would not be that happy ;P


> 
> > diff --git a/hostapd/config_file.c b/hostapd/config_file.c
> > @@ -1391,8 +1394,12 @@ static int hostapd_config_fill(struct hostapd_config *conf,
> >  				wpa_printf(MSG_ERROR, "Line %d: unknown "
> >  					   "macaddr_acl %d",
> >  					   line, bss->macaddr_acl);
> > +				bss->accept_mac_filename[0] = '\0';
> > +				bss->deny_mac_filename[0] = '\0';
> >  			}
> >  		} else if (os_strcmp(buf, "accept_mac_file") == 0) {
> > +			os_strncpy(bss->accept_mac_filename, pos, 256);
> > +			bss->accept_mac_filename[255] = '\0';
> 
> os_strlcpy() should be used instead of those two calls.

oh, thanks

> 
> > +int hostapd_reload_macfile(struct hostapd_iface *iface)
> ...
> > +		/* reload accept mac file if any */
> > +		if (bss->accept_mac_filename[0] != '\0') {
> > +			wpa_printf(MSG_DEBUG, "Reading %s",
> > +				   bss->accept_mac_filename);
> > +			os_free(bss->accept_mac);
> > +			bss->accept_mac = NULL;
> > +			bss->num_accept_mac = 0;
> > +			if (hostapd_config_read_maclist(
> > +						bss->accept_mac_filename,
> > +						&bss->accept_mac,
> > +						&bss->num_accept_mac))
> > +				wpa_printf(MSG_ERROR, "Failed to read "
> > +					   "accept_mac_file '%s'",
> > +					   bss->accept_mac_filename);
> > +		}
> 
> Will that error leave hostapd running with empty MAC ACL list? If so, it
> would be better to either leave the old list in use or prevent the use
> of the BSS after such failure.

I think that preventing this BSS to be used would be better. thanks

> 
> > +		/* deauthenticate listed stations */
> > +		if (bss->macaddr_acl == ACCEPT_UNLESS_DENIED)
> > +			for (j = 0; j < bss->num_deny_mac; j++)
> > +				hostapd_deauth_station(iface->bss[i],
> > +					bss->deny_mac[j].addr,
> > +					WLAN_REASON_PREV_AUTH_NOT_VALID);
> 
> Is this sending Deauthentication frame to every MAC address that is in
> the deny list even if those STAs are not authenticated at the moment?
> That would expose the list by broadcasting it.. This does not sound
> reasonable, i.e., only the STAs that are actually authenticated should
> be deauthenticated.

I definitely agree. Here comes the fact that I don't have a deep knowledge of
the code yet :(

> 
> > diff --git a/src/ap/ap_config.h b/src/ap/ap_config.h
> > @@ -207,14 +207,15 @@ struct hostapd_bss_config {
> >  	int ieee802_11f; /* use IEEE 802.11f (IAPP) */
> >  	char iapp_iface[IFNAMSIZ + 1]; /* interface used with IAPP broadcast
> >  					* frames */
> > -
> >  	enum {
> 
> Please do not include unrelated changes..

sorry, will remove

> 
> > diff --git a/src/ap/hostapd.c b/src/ap/hostapd.c
> > @@ -106,7 +106,6 @@ static void hostapd_reload_bss(struct hostapd_data *hapd)
> >  	wpa_printf(MSG_DEBUG, "Reconfigured interface %s", hapd->conf->iface);
> >  }
> >  
> > -
> >  int hostapd_reload_config(struct hostapd_iface *iface)
> 
> ... same here..
> 
> > diff --git a/src/ap/sta_info.c b/src/ap/sta_info.c
> > +void hostapd_free_sta(struct hostapd_data *hapd, u8 *addr, u16 reason)
> > +{
> > +	struct sta_info *sta, *prev;
> > +
> > +	prev = sta = hapd->sta_list;
> > +
> > +	while (sta && memcmp(sta->addr, addr, ETH_ALEN)) {
> > +		prev = sta;
> > +		sta = sta->next;
> > +	}
> 
> Should use ap_get_sta() instead of another loop to go through the STA
> list.
> 
> > +	if (sta == hapd->sta_list)
> > +		hapd->sta_list = sta->next;
> > +	else
> > +		prev->next = sta->next;
> > +
> > +	hostapd_free_sta_info(hapd, sta, reason);
> 
> Why are the STA list pointers modified here instead of just leaving this
> to ap_free_sta() -> ap_sta_list_del() that gets called from
> hostapd_free_sta_info()?

The idea here was to optimise, and instead of letting ap_free_sta() iterate over
the list each time, I was going through the list manually and deleting the sta
when needed

Thank you very much for your advise Jouni.

Regards,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
Url : http://lists.shmoo.com/pipermail/hostap/attachments/20120618/2180cb66/attachment.pgp