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
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
