[PATCHv2 01/12] Separate station grouping and uplink configuration

[M. Braun]

Am 18.12.2015 um 18:07 schrieb Jouni Malinen:
> On Sun, Dec 06, 2015 at 09:47:32PM +0100, Michael Braun wrote:
>> diff --git a/hostapd/ctrl_iface.c b/hostapd/ctrl_iface.c
>> @@ -1334,7 +1334,9 @@ static int hostapd_ctrl_iface_set(struct hostapd_data *hapd, char *cmd)
>> -				    (!vlan_id || vlan_id == sta->vlan_id))
>> +				    (!vlan_id.notempty ||
>> +				     !os_memcmp(&vlan_id, &sta->vlan_desc,
>> +						sizeof(vlan_id))))
> 
> This style of comparing structures with memcmp does not look safe. While
> most compilers are unlikely to pad the two int variables in the struct
> in this first patch, they would be allowed to do so. Wouldn't it be
> clearer to add a helper function for comparing two instances of struct
> vlan_description?

agreed.

Shall this helper function would become static inline and be included in
the vlan_description header file?

> 
>> diff --git a/src/ap/ap_config.c b/src/ap/ap_config.c
>> -int hostapd_vlan_id_valid(struct hostapd_vlan *vlan, int vlan_id)
>> +int hostapd_vlan_id_valid(struct hostapd_vlan *vlan,
>> +			  struct vlan_description vlan_id)
> 
> Why would this pass a copy of the full struct instead of just a pointer
> to the data? It looks unnecessary to copy all the data for each function
> call.

agreed

> 
>> diff --git a/src/ap/pmksa_cache_auth.h b/src/ap/pmksa_cache_auth.h
>> @@ -28,7 +28,7 @@ struct rsn_pmksa_cache_entry {
>> -	int vlan_id;
>> +	struct vlan_description vlan_id;
> 
> Does this really need a full copy of the struct data (272 bytes on
> 64-bit builds at the end of this patch series) for each PMKSA cache
> entry?

A PMKSA cache entry can exist without a corresponding struct sta_info or
struct hostapd_vlan, so struct rsn_pmksa_cache_entry needs its own
struct vlan_description copy.

In order to avoid the memory penalty for non-VLAN cache entries, this
could become a pointer. If the vlan_escription is empty, the pointer is
set to NULL. Else, a private copy of struct vlan_description is
allocated and free together with the rsn_pmksa_cache_entry.

Would this suffice?

>> +int ap_sta_set_vlan(struct hostapd_data *hapd, struct sta_info *sta,
>> +		    struct vlan_description vlan_desc)
> 
> That same comment about function calls using a full copy vs. pointer..

agreed

> 
>> diff --git a/src/ap/sta_info.h b/src/ap/sta_info.h
>> @@ -44,8 +45,10 @@
>>   * Supported Rates IEs). */
>>  #define WLAN_SUPP_RATES_MAX 32
>>  
>> +struct hostapd_data;
>>  
>>  struct sta_info {
>> +	struct hostapd_data *bss; /* required for set vlan in preauth */
> 
> Cannot say that I'm exactly fond of adding sta->bss pointer,

alternative approach: move get/set vlan for station out of pmksa2eapol
and eapol2pmksa. That is struct eapol_state_machine would hold a struct
*vlan_description instead of void *sta (to be filled during creation and
updated by ap_sta_set_vlan, just referencing the struct sta_info
member), so pmksa2eapol and eapol2pmksa can just copy the
vlan_description data. ieee802_1x_new_station would then call
ap_sta_set_vlan (using the eapol_state_machine member) in addition to
the existing ap_sta_bind_vlan call afer pmksa2eapol, and
ieee802_1x_new_station already has a hapd pointer available.

> ... but that's
> small compared to this:
> 
>> @@ -119,6 +122,7 @@ struct sta_info {
>>  	int vlan_id; /* 0: none, >0: VID */
>> +	struct vlan_description vlan_desc;
> 
> Does this really need that full copy of the 272 byte structure for each
> STA entry? Why would this not be a pointer to the data stored in
> interface configuration?
> 

I was trying to avoid pointers that require referencing counting.

But given that hostapd_vlan already has reference counting in place, it
would work.

So agreed.

Regards,
 M. Braun