=====================================================
CODE REVIEWER: [EMAIL PROTECTED]
WEBREV: http://cr.grommit.com/~meem/wifi-1018
FILES: net80211 crypto + ioctl
NOTES: Description of feedbacks:
ACCEPT Request accepted
REJECT Request rejected
EXPLAIN Explanation given
DISCUSS Request requires further discussion to resolve
DEFER Request deferred (e.g. because work is out-of-scope)
=====================================================
Kacheong, thanks for the review and sorry for the late reply. Below is our
response to your comments. Please let us know if you have any additional
questions.
______________________________________________________________________________
net80211_impl.h:
46: Is net80211.h a new file? Is it reviewed?
| EXPLAIN
| Yes. The code review request has been updated with this header file
| and other two files:
| sys/net80211_proto.h
| sys/net80211_crypto.h
| The updated code review request is at:
| http://www.opensolaris.org/jive/thread.jspa?messageID=67839𐣿
| The updated webrev is at:
| http://cr.grommit.com/~meem/wifi-1018
95-100: It is unclear from reading the comment why those values need
to be divided by IEEE80211_INACT_WAIT. Please explain this
in the comment also.
| ACCEPT
| Added comments as below to explain why '/IEEE80211_INACT_WAIT':
|
| * IEEE80211_INACT_WAIT defines node table's inactivity interval in
| * seconds. On timeout, node table's registered nt_timeout callback
| * function is executed. Each node in the node table has a timeout
| * set in its in_inact field with IEEE80211_INACT_<state>. In
| * nt_timeout function, node table is iterated and each node's
| * in_inact is decremented. So IEEE80211_INACT_<state> is defined in
| * the form [inact_sec]/IEEE80211_INACT_WAIT.
97-98: Nit. It is not intuitive to have "IEEE80211_INACT_AUTH"
represent "associated but not authorized." Maybe
IEEE80211_INACT_ASSOC? Or IEEE80211_INACT_WAIT_AUTH?
| ACCEPT
| Renamed this macro to IEEE80211_INACT_ASSOC.
206: Please move this macro up, after IEEE80211_MSG_CONFIG.
| ACCEPT
______________________________________________________________________________
net80211_ioctl.c:
50: Should we add a DDI strnlen() instead?
| DEFER
| To track this issue,
| CR6490662 add strnlen() function to DDI
| has been filed and it will be fixed after this putback.
53: Please add an empty line.
| ACCEPT
75: Do we need to NULL terminate ow_essid->wl_essid_essid? And
do we need to make sure that the wldp_buf is big enough before
copying? If not, please add a comment explaining that.
| ACCEPT & EXPLAIN
| No need to NULL terminate ow_essid->wl_essid_essid. Because the
| maximum length of essid is 32 chars, while wl_essid_essid is
| defined as:
| char wl_essid_essid[34]; /* wifi_ioctl.h */
| And the output buffer is zeroed when it's been allcated. Then
| actually ow_essid->wl_essid_essid is NULL terminated.
|
| As to the size of wldp_buf, after the changes made to accept
| comments on line 963, each wifi_cfg_xx() will allocate its
| own output buffer, so apparently the wldp_buf is big enough now.
91: Do we really need to zero out the whole buffer? Can we just
NULL terminate the string after copy?
| ACCEPT
| Modified as below
| - bzero(ic->ic_des_essid, IEEE80211_NWID_LEN);
| if (ic->ic_des_esslen != 0)
| bcopy(essid, ic->ic_des_essid, ic->ic_des_esslen);
| + if (ic->ic_des_esslen < IEEE80211_NWID_LEN)
| + ic->ic_des_essid[ic->ic_des_esslen] = 0;
94: So if the wl_essid_length is 0 and the ic_des_essid is zero'ed
out, we won't see the debug message? I think the debug message
is useful when the ic_des_essid is actually changed.
| ACCEPT
| Moved ieee80211_dbg() out of the 'if' clause.
98, 130: It is quite strange that the ioctl succeeds but err is set.
I know that in ieee80211_ioctl(), this particular error value
is checked. But is there a better to communicate this? Or
is it necessary to return this value in this function? If the
desired ESSID is changed, should the driver do something about
it? It seems that the ENETRESET "error" is just ignored. I
that these questions also apply to the other wifi_cfg_XX().
| EXPLAIN
| ENETRESET, as the return value of ieee80211_ioctl(), is checked
| and used by wifi drivers. For example, if the desired ESSID is
| changed, ENETRESET is returned by ieee80211_ioctl(), wifi driver
| then is supposed to re-start connecting to an AP with the specified
| ESSID.
| ENETRESET is ignored when ieee80211_ioctl() sending back ioctl ACK
| message to config tools (dladm or wificonfig).
124: Do we need to make sure that outp->wldp_buf is big enough?
If not, please add a comment explaining that.
| EXPLAIN
| After the changes made by accepting comments on L963, each
| wifi_cfg_xxx() allocates its own output message buffer. So
| apparently wldp_buf is big enough now.
143-189: This function is almost identical to wifi_cfg_essid(). So
the above questions also apply to this function.
| ACCEPT
| 1. Moved ieee80211_dbg() out of the 'if' clause.
| 2. Removed bzero() as below:
| - bzero(ic->ic_nickname, IEEE80211_NWID_LEN);
| if (len > 0)
| bcopy(iw_name->wl_nodename_name, ic->ic_nickname, len);
| + if (len < IEEE80211_NWID_LEN)
| + ic-ic_nickname[len] = 0;
206, 212, 219, 226: Please add an empty line.
| ACCEPT
301: Do we need to make sure that the passed in inp->wldp_buf has
enough room for MAX_NWEPKEYS? If not, please add a comment
explaining that.
| ACCEPT
| Added below codes:
| if (inp ->wldp_length < sizeof (wl_wep_key_tab_t)) {
| ieee80211_err("wifi_cfg_wepkey: "
| "parameter too short, %d, expected %d\n",
| inp->wldp_length, sizeof (wl_wep_key_tab_t));
| outp->wldp_result = WL_NOTSUPPORTED;
| return (EINVAL);
| }
576: Please add a comment explaining that each "rate" is a uint8_t.
This is why the memory allocation is done that way.
| ACCEPT
| Added comments:
| /* type of rate value is char */
595-608: I suspect that the code can be simplified. Firstly, I think
it does not need to allocate srates. Just use the destination
buffer wl_rates_rates as the scratch. Go over ic_sup_rates
and check if the rate is already in the scratch. If not, add
it in sequence. If yet, skip. Then the loop 584-591 can
be avoided.
| ACCEPT
| The changes looks like:
| case WLAN_GET_PARAM:
| /* all rates supported by the device */
| ow_rates->wl_rates_num = 0;
| drates = (uint8_t *)ow_rates->wl_rates_rates;
| for (i = 0; i < IEEE80211_MODE_MAX; i++) {
| srs = &ic->ic_sup_rates[i];
| if (srs->ir_nrates == 0)
| continue;
|
| for (j = 0; j < srs->ir_nrates; j++) {
| srates = IEEE80211_RV(srs->ir_rates[j]);
| /* sort and skip duplicated rates */
| for (k = 0; k < ow_rates->wl_rates_num; k++) {
| if (srates <= drates[k])
| break;
| }
| if (srates == drates[k])
| continue; /* duplicate, skip */
| /* sort */
| for (l = ow_rates->wl_rates_num; l > k; l--)
| drates[l] = drates[l-1];
| drates[k] = srates;
| ow_rates->wl_rates_num++;
| }
| }
BTW, does the code need to make sure that wldp_buf can hold
all the rates? If not, please explain.
| EXPLAIN
| After the changes made by accepting comments on L963, each
| wifi_cfg_xxx() allocates its own output message buffer. So
| apparently wldp_buf is big enough now.
672: So if drate is not supported in the negotiated rate set (in_rates)
and ic->ic_state == IEEE80211_S_RUN, the code will continue in 676.
And if it happens that the drate is in the supported rate set by
the driver, the ioctl will return with no error. Is this the correct
behavior? The driver cannot really use the rate being set.
| EXPLAIN
| Yes, it's the desired behavior. The desired rate is only for data
| transfer. So usually the desired rate is checked and used when the
| driver changes to RUN state.
| If the driver is SCAN, the desired rate is set but driver don't have
| to do anything.
| If the driver is AUTH or ASSOC, the drate is checked against rates
| supported by current ESS, nothing will be done if the drate is
| supported, and re-connecting is required to check if another ESS
| with specified drate can be found when the drate is not supported
| by the current ESS.
681: Can found be a boolean_t?
| ACCEPT
| Changed type of found to be boolean_t and modified code accordingly.
721: I guess a comment is needed to explain what the code is trying
to do here. Why rescale the RSSI value to [0, MAX_RSSI]?
| ACCEPT & EXPLAIN
| Each wifi device has its own range of RSSI value. The wifi driver
| IOCTLs defines the range of printed RSSI value between 0 and 15
| (MAX_RSSI). So the device's RSSI value is rescaled.
| Added comments to function wifi_getrssi() as below:
|/*
| * Rescale device's RSSI value to (0, 15) as required by WiFi
| * driver IOCTLs (PSARC/2003/722)
| */
749: Will WAIT_SCAN_MAX be better to be a global so that it can be
changed in case of a problem in production?
| DISCUSS
| Please refer to explaination and modifications made below (L760).
760: Should 200000 be configurable also? And why can't
cv_timedwait_sig() be used for this? Please add some comment
explaining the reason.
| ACCEPT
| Changed to use cv_timedwait_sig(), thus 200000 is removed. And
| WAIT_SCAN_MAX is re-defined to be maximum total scan wait time
| as below.
| /*
| * maximum scan wait time in second.
| * Time spent on scaning one channel is usually 100~200ms. The maximum
| * number of channels defined in wifi_ioctl.h is 99 (MAX_CHANNEL_NUM).
| * As a result the maximum total scan time is defined to be
| * (200ms * 100 = 20s)
| */
| #define WAIT_SCAN_MAX 20
| Actually the maximum channel number 99 is bigger than number of
| standard 802.11 channels. And since this value is big enough, I'd
| prefer to keep it staticly defined.
760: And is it better for wifi_wait_scan() to take an argument
which specifies the max time to wait?
| DISCUSS
| wifi_wait_scan() is a static function called internally by net80211
| ioctl code. It's not supposed to be called by other module or wifi
| drivers, and the wait time has been made long enough (as is explained
| previously). So I'd prefer to keep it as it is now.
768: Please add a comment explaining that this function is the call back
for ieee80211_iterate_nodes() used in wifi_cfg_esslist() to get
all the nodes. The comment should also explain the argument passed
in.
| ACCEPT
| Added comments as below:
|/*
| * Callback function used by ieee80211_iterate_nodes() in
| * wifi_cfg_esslist() to get info of each node in a node table
| * arg output buffer, pointer to wl_ess_list_t
| * in each node in the node table
| */
777: Please add a comment saying that it is assumed the passed in
buffer is MAX_BUF_LEN large.
| EXPLAIN
| After the changes made by accepting comments on L963, each
| wifi_cfg_xxx() allocates its own output message buffer. So
| apparently wldp_buf is big enough now.
810, 818, 831: Please add an empty line.
| ACCEPT
887: Please add an empty line.
| ACCEPT
887: The code does nothing to change the state if the current state
is not IEEE80211_S_INIT. Is it correct? So if the current state
is IEEE80211_S_RUN, should the code just return? Or should the
code continue to initiate a scan? Please add some comment
explaining what the code tried to do.
| ACCEPT & EXPLAIN
| If current state is RUN, the code just returns for a known bug.
| 6212098 Ath driver can not scan out the new network name when
| it is already connect with a wlan
| Added comments and codes as below:
| /* Return when current state is RUN for CR6212098 */
| if (ostate == IEEE80211_S_RUN)
| return (0);
|
| And fixed to start SCAN in other states by removing first
| 'if (ostate == IEEE80211_S_INIT) {'
895: Please explain why the state needs to be changed back to
IEEE80211_S_INIT.
| EXPLAIN
| The state is changed back to INIT only when original state is
| also INIT. The reason is that at the end of SCAN stage, if an
| AP is selected, the station will consequently connect to that
| AP. Then it looks unreasonable that for a disconnected device,
| a SCAN command causes it connected. So the state is changed back
| to INIT.
963, 1030-1032: Why can't an mblk be allocated instead and pass in each
wifi_cfg_XX()? Then there is no need to kmem_zalloc() and
them kmem_free(). If this means that a MAX_BUF_LEN mblk
needs
to be allocated, why can't each wifi_cfg_XX() allocate its
own mblk and pass back to the wifi_cfg_getset()? I think
mi_mpprintf_putc() should not be used here. It is intended
to add one more character to an mblk, but not to be used
as a copy function.
| ACCEPT
| Removed kmem_zalloc(), kmem_free() and mi_mpprintf_putc(), instead
| each wifi_cfg_xxx() allocates its own output mblk and pass back to
| wifi_cfg_getset().
1082: Can secpolicy_net_config be NULL?
| ACCEPT
| Removed 'if (secpolicy_net_config != NULL)' and 'else ...'
______________________________________________________________________________
net80211_crypto.h:
56-58: I don't quite understand this note. Why will these
IEEE80211_CIPHER_XXX conflict with IEEE80211_F_PRIVACY?
Should they be in different name spaces? Isn't
IEEE80211_F_XXX supposed to be used in ic_flags?
| ACCEPT & EXPLAIN
| The reason for this comment is that corresponding to each
| IEEE80211_CIPHER_XXX, there is a IEEE80211_C_XXX where
| IEEE80211_C_XXX = 1 << IEEE80211_CIPHER_XXX
| IEEE80211_C_XXX is used to describe hardware capability.
| Consequently, corresponding to some IEEE80211_C_XXX, there
| is a IEEE80211_F_XXX which is used to describe whether the
| capability is enabled and is defined the same as IEEE80211_C_XXX.
| For example,
| #define IEEE80211_CIPHER_WEP 0
| #define IEEE80211_C_WEP 0x00000001 /* 1 << 0 */
| and
| #define IEEE80211_C_WPA1 0x00800000
| #define IEEE80211_F_WPA1 0x00800000
| Then since IEEE80211_F_PRIVACY is defined to be 0x10,
| IEEE80211_PRIVACY_XXX is defined to skip 4.
|
| But it's also OK to define IEEE80211_F_PRIVACY and one of
| IEEE80211_C_XXX the same, because IEEE80211_F_XXX is used by ic_flags,
| and IEEE80211_C_XXX is used by ic_caps. The comment here seems
| confusing.
|
| Then changed the comments to:
| /*
| * NB: these values are ordered carefully; there are lots of
| * of implications in any reordering.
| */
|
| And re-define as below:
|------- usr/src/uts/common/sys/net80211_crypto.h -------
| #define IEEE80211_CIPHER_CKIP 4
| #define IEEE80211_CIPHER_NONE 5 /* pseudo value */
|
|------- usr/src/uts/common/sys/net80211.h -------
| #define IEEE80211_C_CKIP 0x00000010 /* CAPABILITY:
CKIP available */
67: The max number of ciphers available is hard coded. Why?
So when there is a new cipher module, does it mean that we
need to re-compile the 802.11 crypto code to accommodate that?
This does not sound extensible.
| DISCUSS
| Originally in BSD, each cipher is made a seperate module. But
| the codes are not so flexible, as you've pointed out - max number
| is hard coded thus when a new cipher is added, re-compiling of
| net80211 is always required. During porting, the cipher is made
| part of net80211 instead of a seperate module since currently
| 802.11/802.11i standardized ciphers are limited (only WEP/TKIP/CCMP).
69: I suppose the size is in bytes. Maybe a comment? Will we
ever have a key larger than 16 bytes? Is it limited by
the 802.11 standard? A comment?
| ACCEPT
| Added comments as below:
|/*
| * Maxmium length of key in bytes
| * WEP key length present in the 802.11 standard is 40-bit.
| * Many implementations also support 104-bit WEP keys.
| * 802.11i standardize TKIP/CCMP use 128-bit key
| */
82: Please move this #define below the typedef of ieee80211_keyix
for clarity.
| ACCEPT
| Moved #define below typedef.
105: Please add a comment to explain the reason to have a null
cipher always available.
| ACCEPT
| The comment isn't correct and is changed to:
|/*
| * Template for a supported cipher. Ciphers register with the
| * crypto code.
| */
106: Is refcnt needed or not? We should remove all XXX in
production code.
| ACCEPT
| Removed the XXX comments
114-123: Is there a document explaining what these functions are?
If not, please add some comments.
| ACCEPT
| Added comments as below:
|
| * ic_attach - Initialize cipher. The return value is set to wk_private
| * ic_detach - Destruct a cipher.
| * ic_setkey - Validate key contents
| * ic_encap - Encrypt the 802.11 MAC payload
| * ic_decap - Decrypt the 802.11 MAC payload
| * ic_enmic - Add MIC
| * ic_demic - Check and remove MIC
| */
138, 139: Is it a problem to have separate buffers for TX mic and
RX mic? Then we don't have to do the #define. If it is
a problem, please comment.
| DISCUSS
| The structure definition is ported from BSD. I'd prefer to
| keep it unchanged to ease later wifi drivers' porting.
144: XXX...
| ACCEPT
| Removed XXX comments
147: Why is WEP crypto #define IEEE80211_WEP_NKID used in a general
structure for all 802.11 ciphers?
| ACCEPT & EXPLAIN
| 'struct ieee80211_crypto_state' is common for WEP|TKIP|CCMP. While
| TKIP|CCMP use one key, WEP uses 4 keys. So 4 (IEEE80211_WEP_NKID)
| is used as the maximum number of keys. The macro here is confusing.
| So added a new macro and changed the definition as below:
| /* Maximum number of keys */
| #define IEEE80211_KEY_MAX IEEE80211_WEP_NKID
|
| struct ieee80211_crypto_state{
| struct ieee80211_key cs_nw_keys[IEEE80211_KEY_MAX];
| ...
| }
164: XXX...
| ACCEPT
| Removed XXX comments
______________________________________________________________________________
net80211_crypto.c:
It seems that a lot of the functions return (0) and (1). Can
boolean_t be used?
| DISCUSS
| These are registered callback functions to 'struct
ieee80211_crypto_state'.
| I'd prefer to keep the interface definition the same as BSD's to
| ease later WiFi drivers' porting.
170: Can a key be used by two different ciphers? I ask because of
this check (key->wk_cipher != cip).
| EXPLAIN
| Yes, cs_nw_keys[i] can be used with different ciphers. When the
| device changes to use another cipher. The selected key's
| key->wk_cipher is checked, old cipher is detached and new cipher
| is attached if necessary.
226: Should the key be detached from the cipher on failure?
| EXPLAIN
| When re-initialize a key, the cipher is checked and replaced
| with the new cipher if necessary. So no need to detach the cipher
| on failure here.
______________________________________________________________________________
net80211_crypto_none.c:
43-67: Just a sanity check. Does compiling this file require so
many header files?
| ACCEPT
| Modified to only include:
| #include "net80211_impl.h"
______________________________________________________________________________
net80211_crypto_wep.c:
44-67: Just a sanity check. Does compiling this file require so
many header files?
| ACCEPT
| Modified to only include:
| #include <sys/byteorder.h>
| #include <sys/crypto/common.h>
| #include <sys/crypto/api.h>
| #include "net80211_impl.h"
124: Can random_get_pseudo_bytes() be used here? If not, please
add a comment.
| ACCEPT
| Changed as below:
| (void) random_get_pseudo_bytes((unsigned char *)&ctx->wc_iv,
| sizeof (uint32_t));
140: Is 40/NBBY minimum allowed key length? How about max? Please
add a comment.
| ACCEPT
| Added comments and changed as below:
| /*
| * WEP key length is standardized to 40-bit. Many
| * implementations support 104-bit WEP kwys.
| */
| return (k->wk_keylen == 40/NBBY || k->wk_keylen == 128/NBBY);
182: What is ATH_HOST_BIG_ENDIAN? If it should not be define, please
remove it from the source.
| ACCEPT
| Removed.
259-294: Can those macros in <sys/crc32.h> be used? If not, why not?
| ACCEPT
| Re-defined crc_table as below:
| static uint32_t crc_table[] = { CRC32_TABLE };
| Removed crc_init(). Replaced crc_update() with CRC32().
390-486: Can the KCF be used for RC4? If not, why not?
| EXPLAIN
| We're using KCF.
_______________________________________________
networking-discuss mailing list
[email protected]
