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

[Johannes Berg]

Hi,

So I removed those call-through place because private wext operations
are a really bad idea and should just die. We don't want drivers to
require specially patched hostapd versions to work right, like here.

I suppose in staging you might want to work around that like here, but
that really means you see no future in mainline for this code, or
otherwise you should fix it the proper way.

johannes

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

[Johannes Berg]

Hi,

So I removed those call-through place because private wext operations
are a really bad idea and should just die. We don't want drivers to
require specially patched hostapd versions to work right, like here.

I suppose in staging you might want to work around that like here, but
that really means you see no future in mainline for this code, or
otherwise you should fix it the proper way.

johannes

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

[Ishraq Ibne Ashraf]

Hi,

What was broken was private/device specific IOCTL calls implemented by this 
driver. The standard IOCTL calls worked and the driver worked as it was in 
client mode.

But in AP mode with hostapd (https://w1.fi/hostapd/) the rtl871xdrv driver of 
hostapd (which is required for using devices that use this driver in AP mode) 
invokes private/device specific IOCTL calls and in the existing driver they 
could only be invoked through the ndo_do_ioctl interface of the driver. Support 
for which was removed in the mentioned commit by Johannes Berg. Hence the 
driver stopped working in AP mode with hostapd using rtl871xdrv driver.

The solution was to implement equivalent versions of the existing 
private/device specific IOCTLs which will be invoked by the iw_handler_def 
interface.


Cheers,
Ishraq


On 11/25/2017 05:40 AM, Dan Carpenter wrote:
> I added Johannes to the CC list again because this is sort of his
> fault...  To be honest, I'm a little bit puzzled.  I would have thought
> Johannes's change would have made some code unused and that we could
> delete it now.  I haven't looked at this.

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

[Ishraq Ibne Ashraf]

Hi,

What was broken was private/device specific IOCTL calls implemented by this 
driver. The standard IOCTL calls worked and the driver worked as it was in 
client mode.

But in AP mode with hostapd (https://w1.fi/hostapd/) the rtl871xdrv driver of 
hostapd (which is required for using devices that use this driver in AP mode) 
invokes private/device specific IOCTL calls and in the existing driver they 
could only be invoked through the ndo_do_ioctl interface of the driver. Support 
for which was removed in the mentioned commit by Johannes Berg. Hence the 
driver stopped working in AP mode with hostapd using rtl871xdrv driver.

The solution was to implement equivalent versions of the existing 
private/device specific IOCTLs which will be invoked by the iw_handler_def 
interface.


Cheers,
Ishraq


On 11/25/2017 05:40 AM, Dan Carpenter wrote:
> I added Johannes to the CC list again because this is sort of his
> fault...  To be honest, I'm a little bit puzzled.  I would have thought
> Johannes's change would have made some code unused and that we could
> delete it now.  I haven't looked at this.

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

[Ishraq Ibne Ashraf]

Hi,

What was broken was private/device specific IOCTL calls implemented by this 
driver. The standard IOCTL calls worked and the driver worked as it was in 
client mode.

But in AP mode with hostapd (https://w1.fi/hostapd/) the rtl871xdrv driver of 
hostapd (which is required for using devices that use this driver in AP mode) 
invokes private/device specific IOCTL calls and in the existing driver they 
could only be invoked through the ndo_do_ioctl interface of the driver. Support 
for which was removed in the mentioned commit by Johannes Berg. Hence the 
driver stopped working in AP mode with hostapd using rtl871xdrv driver.

The solution was to implement equivalent versions of the existing 
private/device specific IOCTLs which will be invoked by the iw_handler_def 
interface.


Cheers,
Ishraq


On 11/25/2017 05:40 AM, Dan Carpenter wrote:
> I added Johannes to the CC list again because this is sort of his
> fault...  To be honest, I'm a little bit puzzled.  I would have thought
> Johannes's change would have made some code unused and that we could
> delete it now.  I haven't looked at this.

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

[Ishraq Ibne Ashraf]

Hi,

What was broken was private/device specific IOCTL calls implemented by this 
driver. The standard IOCTL calls worked and the driver worked as it was in 
client mode.

But in AP mode with hostapd (https://w1.fi/hostapd/) the rtl871xdrv driver of 
hostapd (which is required for using devices that use this driver in AP mode) 
invokes private/device specific IOCTL calls and in the existing driver they 
could only be invoked through the ndo_do_ioctl interface of the driver. Support 
for which was removed in the mentioned commit by Johannes Berg. Hence the 
driver stopped working in AP mode with hostapd using rtl871xdrv driver.

The solution was to implement equivalent versions of the existing 
private/device specific IOCTLs which will be invoked by the iw_handler_def 
interface.


Cheers,
Ishraq


On 11/25/2017 05:40 AM, Dan Carpenter wrote:
> I added Johannes to the CC list again because this is sort of his
> fault...  To be honest, I'm a little bit puzzled.  I would have thought
> Johannes's change would have made some code unused and that we could
> delete it now.  I haven't looked at this.

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

[Dan Carpenter]

Thanks for doing this.

This needs to be folded in with the earlier patch you send and then
resend it as a V2 patch.
https://kernelnewbies.org/FirstKernelPatch#head-5c81b3c517a1d0bbc24f92594cb734e155fcbbcb

I added Johannes to the CC list again because this is sort of his
fault...  To be honest, I'm a little bit puzzled.  I would have thought
Johannes's change would have made some code unused and that we could
delete it now.  I haven't looked at this.

>  
> - if (ret == 0 && (copy_to_user(wrqu->data.pointer, param, 
> wrqu->data.length)))
> + if (pmlmepriv->htpriv.ht_option == false)
> + psta->htpriv.ht_option = false;
> +
> + update_sta_info_apmode(padapter, psta);
> +
> + ret = 0;
> +
> + if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length))
>   ret = -EFAULT;
>  
> - return ret;
> + err_free_param:
> + kfree(param);
> + return ret;
>  }

Please keep the success path and failure paths separate like I did in
the example code that I wrote in my email.  Don't indent the labels.  It
should look like this:

update_sta_info_apmode(padapter, psta);

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

kfree(param);
return 0;

err_free_param:
kfree(param);
return ret;

regards,
dan carpenter

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

[Dan Carpenter]

Thanks for doing this.

This needs to be folded in with the earlier patch you send and then
resend it as a V2 patch.
https://kernelnewbies.org/FirstKernelPatch#head-5c81b3c517a1d0bbc24f92594cb734e155fcbbcb

I added Johannes to the CC list again because this is sort of his
fault...  To be honest, I'm a little bit puzzled.  I would have thought
Johannes's change would have made some code unused and that we could
delete it now.  I haven't looked at this.

>  
> - if (ret == 0 && (copy_to_user(wrqu->data.pointer, param, 
> wrqu->data.length)))
> + if (pmlmepriv->htpriv.ht_option == false)
> + psta->htpriv.ht_option = false;
> +
> + update_sta_info_apmode(padapter, psta);
> +
> + ret = 0;
> +
> + if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length))
>   ret = -EFAULT;
>  
> - return ret;
> + err_free_param:
> + kfree(param);
> + return ret;
>  }

Please keep the success path and failure paths separate like I did in
the example code that I wrote in my email.  Don't indent the labels.  It
should look like this:

update_sta_info_apmode(padapter, psta);

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

kfree(param);
return 0;

err_free_param:
kfree(param);
return ret;

regards,
dan carpenter

[PATCH 2/2] staging: rtl8188eu: Fix private WEXT IOCTL calls

[ishraq . i . ashraf]

From: Ishraq Ibne Ashraf 

Apply changes suggested by Dan Carpenter
---
 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 1052 
 1 file changed, 536 insertions(+), 516 deletions(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c 
b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
index 7503751..e871344 100644
--- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
@@ -3062,25 +3062,16 @@ static iw_handler rtw_handlers[] = {
 };
 
 static int get_private_handler_ieee_param(struct adapter *padapter,
-   union iwreq_data *wrqu,
-   void *param)
+ union iwreq_data *wrqu,
+ void *param)
 {
/*
 * 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;
-
-   /*
-* 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.
-*/
if (copy_from_user(param, wrqu->data.pointer, wrqu->data.length))
return -EFAULT;
 
@@ -3088,305 +3079,310 @@ static int get_private_handler_ieee_param(struct 
adapter *padapter,
 }
 
 static int rtw_hostapd_sta_flush_pvt(struct net_device *dev,
-   struct iw_request_info *info,
-   union iwreq_data *wrqu,
-   char *extra)
+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__);
-
-   flush_all_cam_entry(padapter); // Clear CAM.
-
+   flush_all_cam_entry(padapter); /* Clear CAM. */
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)
+  struct iw_request_info *info,
+  union iwreq_data *wrqu,
+  char *extra)
 {
-   int ret = 0;
-   struct sta_info *psta = NULL;
-   struct ieee_param *param = NULL;
+   int ret;
+   int flags;
+   struct sta_info *psta;
+   struct ieee_param *param;
struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
-   struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
struct sta_priv *pstapriv = >stapriv;
+   struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
 
-   param = (struct ieee_param *)rtw_malloc(wrqu->data.length);
-
-   if (!param) {
-   DBG_88E(" rtw_add_sta: ieee_param allocate fail !!!\n");
-
+   param = (struct ieee_param *)kmalloc(wrqu->data.length, GFP_KERNEL);
+   if (!param)
return -ENOMEM;
-   }
 
ret = get_private_handler_ieee_param(padapter, wrqu, param);
+   if (ret)
+   goto err_free_param;
 
-   if (ret != 0) {
-   kfree(param);
-   DBG_88E(" rtw_add_sta: ieee_param get fail !!!\n");
+   DBG_88E("rtw_add_sta(aid =%d) =%pM\n",
+   param->u.add_sta.aid,
+   (param->sta_addr));
 
-   return ret;
+   if (!check_fwstate(pmlmepriv, (_FW_LINKED|WIFI_AP_STATE))) {
+   ret = -EINVAL;
+   goto err_free_param;
}
 
-   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;
-
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;
+   param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff) {
+   ret = -EINVAL;
+   goto err_free_param;
+   }
 
psta = rtw_get_stainfo(pstapriv, param->sta_addr);
-   if (psta) {
-   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);
+   if (!psta) {
+   ret = -ENOMEM;
+   goto err_free_param;
+   }
 
-   // Check WMM cap.
-   if (WLAN_STA_WME)
-   psta->qos_option = 1;
-   else
-   psta->qos_option = 0;
+   flags = param->u.add_sta.flags;
+   psta->aid = param->u.add_sta.aid; /* aid = 1~2007. */
 
-   if 

[PATCH 2/2] staging: rtl8188eu: Fix private WEXT IOCTL calls

[ishraq . i . ashraf]

From: Ishraq Ibne Ashraf 

Apply changes suggested by Dan Carpenter
---
 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 1052 
 1 file changed, 536 insertions(+), 516 deletions(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c 
b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
index 7503751..e871344 100644
--- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
@@ -3062,25 +3062,16 @@ static iw_handler rtw_handlers[] = {
 };
 
 static int get_private_handler_ieee_param(struct adapter *padapter,
-   union iwreq_data *wrqu,
-   void *param)
+ union iwreq_data *wrqu,
+ void *param)
 {
/*
 * 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;
-
-   /*
-* 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.
-*/
if (copy_from_user(param, wrqu->data.pointer, wrqu->data.length))
return -EFAULT;
 
@@ -3088,305 +3079,310 @@ static int get_private_handler_ieee_param(struct 
adapter *padapter,
 }
 
 static int rtw_hostapd_sta_flush_pvt(struct net_device *dev,
-   struct iw_request_info *info,
-   union iwreq_data *wrqu,
-   char *extra)
+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__);
-
-   flush_all_cam_entry(padapter); // Clear CAM.
-
+   flush_all_cam_entry(padapter); /* Clear CAM. */
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)
+  struct iw_request_info *info,
+  union iwreq_data *wrqu,
+  char *extra)
 {
-   int ret = 0;
-   struct sta_info *psta = NULL;
-   struct ieee_param *param = NULL;
+   int ret;
+   int flags;
+   struct sta_info *psta;
+   struct ieee_param *param;
struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
-   struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
struct sta_priv *pstapriv = >stapriv;
+   struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
 
-   param = (struct ieee_param *)rtw_malloc(wrqu->data.length);
-
-   if (!param) {
-   DBG_88E(" rtw_add_sta: ieee_param allocate fail !!!\n");
-
+   param = (struct ieee_param *)kmalloc(wrqu->data.length, GFP_KERNEL);
+   if (!param)
return -ENOMEM;
-   }
 
ret = get_private_handler_ieee_param(padapter, wrqu, param);
+   if (ret)
+   goto err_free_param;
 
-   if (ret != 0) {
-   kfree(param);
-   DBG_88E(" rtw_add_sta: ieee_param get fail !!!\n");
+   DBG_88E("rtw_add_sta(aid =%d) =%pM\n",
+   param->u.add_sta.aid,
+   (param->sta_addr));
 
-   return ret;
+   if (!check_fwstate(pmlmepriv, (_FW_LINKED|WIFI_AP_STATE))) {
+   ret = -EINVAL;
+   goto err_free_param;
}
 
-   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;
-
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;
+   param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff) {
+   ret = -EINVAL;
+   goto err_free_param;
+   }
 
psta = rtw_get_stainfo(pstapriv, param->sta_addr);
-   if (psta) {
-   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);
+   if (!psta) {
+   ret = -ENOMEM;
+   goto err_free_param;
+   }
 
-   // Check WMM cap.
-   if (WLAN_STA_WME)
-   psta->qos_option = 1;
-   else
-   psta->qos_option = 0;
+   flags = param->u.add_sta.flags;
+   psta->aid = param->u.add_sta.aid; /* aid = 1~2007. */
 
-   if (pmlmepriv->qospriv.qos_option ==