lkml.org 
[lkml]   [2017]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] staging: rtl8188eu: Fix private WEXT IOCTL calls
On Thu, Nov 23, 2017 at 02:29:06AM +0100, ishraq.i.ashraf@gmail.com wrote:
> From: Ishraq Ibne Ashraf <ishraq.i.ashraf@gmail.com>
>
> Commit 8bfb36766064 ("wireless: wext: remove ndo_do_ioctl fallback") breaks private WEXT
> IOCTL calls of this driver as these are not invoked through ndo_do_ioctl
> interface anymore. As a result hostapd stops working with this driver. In
> this patch this problem is solved by implementing equivalent private IOCTL
> functions of the existing ones which are accessed via iw_handler_def
> interface.
>
> Signed-off-by: Ishraq Ibne Ashraf <ishraq.i.ashraf@gmail.com>


It's great to fix this, but new code should be at normal kernel quality.

> ---
> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 1042 ++++++++++++++++++++++++
> 1 file changed, 1042 insertions(+)
>
> diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> index c0664dc..7503751 100644
> --- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> +++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> @@ -3061,6 +3061,1046 @@ static iw_handler rtw_handlers[] = {
> NULL, /*---hole---*/
> };
>
> +static int get_private_handler_ieee_param(struct adapter *padapter,
> + union iwreq_data *wrqu,
> + void *param)

Indent these more.

> +{
> + /*
> + * This function is expected to be called in master mode, which allows no
> + * power saving. So we just check hw_init_completed.
> + */
> +
> + if (!padapter->hw_init_completed)
> + return -EPERM;
> +
> + if (!wrqu->data.pointer)
> + return -EINVAL;

You could leave this out and it will return -EFAULT when copy_from_user()
fails. That's probably the right error code.

> +
> + /*
> + * Since we don't allocate memory for param in this function, we assume
> + * the caller of this function will properly allocate and deallocate memory
> + * for param.
> + */

This is an obvious comment. Remove it.

> + if (copy_from_user(param, wrqu->data.pointer, wrqu->data.length))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static int rtw_hostapd_sta_flush_pvt(struct net_device *dev,
> + struct iw_request_info *info,
> + union iwreq_data *wrqu,
> + char *extra)
> +{
> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> +
> + DBG_88E("%s\n", __func__);

Remove this line.

> +
> + flush_all_cam_entry(padapter); // Clear CAM.

Comment style.

> +
> + return rtw_sta_flush(padapter);
> +}
> +
> +static int rtw_add_sta_pvt(struct net_device *dev,
> + struct iw_request_info *info,
> + union iwreq_data *wrqu,
> + char *extra)
> +{
> + int ret = 0;
> + struct sta_info *psta = NULL;
> + struct ieee_param *param = NULL;

Don't initialize these to useless values. It turns off the static
checker for finding uninitialized variables.

> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> + struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
> + struct sta_priv *pstapriv = &padapter->stapriv;
> +
> + param = (struct ieee_param *)rtw_malloc(wrqu->data.length);

rtw_malloc() is buggy. It ignores locking. Let's not use it in new
code.

> +
> + if (!param) {
> + DBG_88E(" rtw_add_sta: ieee_param allocate fail !!!\n");

No need to print this debug message.

> +
> + return -ENOMEM;
> + }
> +
> + ret = get_private_handler_ieee_param(padapter, wrqu, param);
> +
> + if (ret != 0) {

if (ret). "ret" isn't a number zero which can be used for math like
"ret + 2", it's an error code. The != 0 is a double negative which
hurts readability. != 0 is appropriate for numbers and strcmp()
functions.

> + kfree(param);

Free this at the end of the function.

> + DBG_88E(" rtw_add_sta: ieee_param get fail !!!\n");

These messages are so ugly !!!

> +
> + return ret;
> + }
> +
> + DBG_88E("rtw_add_sta(aid =%d) =%pM\n", param->u.add_sta.aid, (param->sta_addr));
> +
> + if (!check_fwstate(pmlmepriv, (_FW_LINKED|WIFI_AP_STATE)))
> + return -EINVAL;

ret = -EINVAL;
goto err_free_param;

> +
> + if (param->sta_addr[0] == 0xff && param->sta_addr[1] == 0xff &&
> + param->sta_addr[2] == 0xff && param->sta_addr[3] == 0xff &&
> + param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff)
> + return -EINVAL;

ret = -EINVAL;
goto err_free_param;


> +
> + psta = rtw_get_stainfo(pstapriv, param->sta_addr);
> + if (psta) {

Always do failure handling. Never do success handling. So this becomes:

if (!psta)
goto err_free_param;



> + int flags = param->u.add_sta.flags;
> + psta->aid = param->u.add_sta.aid; // aid = 1~2007.
> +
> + memcpy(psta->bssrateset, param->u.add_sta.tx_supp_rates, 16);
> +
> + // Check WMM cap.

Comment style

> + if (WLAN_STA_WME&flags)
> + psta->qos_option = 1;
> + else
> + psta->qos_option = 0;
> +
> + if (pmlmepriv->qospriv.qos_option == 0)
> + psta->qos_option = 0;
> +
> + // Check 802.11n HT cap.
> + if (WLAN_STA_HT&flags) {
> + psta->htpriv.ht_option = true;
> + psta->qos_option = 1;
> + memcpy(&psta->htpriv.ht_cap,
> + &param->u.add_sta.ht_cap,
> + sizeof(struct ieee80211_ht_cap));
> + } else {
> + psta->htpriv.ht_option = false;
> + }
> +
> + if (pmlmepriv->htpriv.ht_option == false)
> + psta->htpriv.ht_option = false;
> +
> + update_sta_info_apmode(padapter, psta);
> + } else {
> + ret = -ENOMEM;
> + }
> +
> + if (ret == 0 && (copy_to_user(wrqu->data.pointer, param, wrqu->data.length)))
> + ret = -EFAULT;

We need to free param.

> +
> + return ret;


The end of the function could look like this:

update_sta_info_apmode(padapter, psta);

if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length))
ret = -EFAULT;

err_free_param:
kfree(param);

return ret;


> +}
> +
> +static int rtw_del_sta_pvt(struct net_device *dev,
> + struct iw_request_info *info,
> + union iwreq_data *wrqu,
> + char *extra)
> +{
> + int ret = 0;
> + struct sta_info *psta = NULL;
> + struct ieee_param *param = NULL;

Remove initialization.

> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> + struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
> + struct sta_priv *pstapriv = &padapter->stapriv;
> + int updated = 0;
> +
> + param = (struct ieee_param *)rtw_malloc(wrqu->data.length);

Use kmalloc(); Basically all the same stuff as the previous function.

regards,
dan carpenter

\
 
 \ /
  Last update: 2017-11-23 14:25    [W:0.117 / U:0.356 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site