Re: [PATCH] staging: rtl8188eu: Fix private WEXT IOCTL calls

[Dan Carpenter]

On Thu, Nov 23, 2017 at 02:29:06AM +0100, ishraq.i.ashraf@xxxxxxxxx wrote:
> From: Ishraq Ibne Ashraf <ishraq.i.ashraf@xxxxxxxxx>
> 
> 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@xxxxxxxxx>


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