diff mbox

[OPW,kernel] staging: rtl8192u: use memdup_user to simplify code

Message ID 1382645270-731-1-git-send-email-teobaluta@gmail.com
State New, archived
Headers show

Commit Message

Teodora Baluta Oct. 24, 2013, 8:07 p.m. UTC
Use memdup_user rather than duplicating its implementation. Fix the
following coccinelle warnings:

drivers/staging/rtl8192u/r8192U_core.c:3792:7-14: WARNING opportunity for memdup_user
drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:3153:9-16: WARNING opportunity for memdup_user

Signed-off-by: Teodora Baluta <teobaluta@gmail.com>
---
 drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c |   12 ++++--------
 drivers/staging/rtl8192u/r8192U_core.c                 |   12 ++++--------
 2 files changed, 8 insertions(+), 16 deletions(-)

Comments

Rusty Russell Oct. 25, 2013, 12:56 a.m. UTC | #1
Teodora Baluta <teobaluta@gmail.com> writes:

> Use memdup_user rather than duplicating its implementation. Fix the
> following coccinelle warnings:
>
> drivers/staging/rtl8192u/r8192U_core.c:3792:7-14: WARNING opportunity for memdup_user
> drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:3153:9-16: WARNING opportunity for memdup_user
>
> Signed-off-by: Teodora Baluta <teobaluta@gmail.com>

Hi Teodora,

        This is a nice cleanup, but one minor comment:

> ---
>  drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c |   12 ++++--------
>  drivers/staging/rtl8192u/r8192U_core.c                 |   12 ++++--------
>  2 files changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> index 5fd6969..00b7143 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> @@ -3150,14 +3150,10 @@ int ieee80211_wpa_supplicant_ioctl(struct ieee80211_device *ieee, struct iw_poin
>  		goto out;
>  	}
>  
> -	param = kmalloc(p->length, GFP_KERNEL);
> -	if (param == NULL){
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -	if (copy_from_user(param, p->pointer, p->length)) {
> -		kfree(param);
> -		ret = -EFAULT;
> +	param = memdup_user(p->pointer, p->length);
> +
> +	if (IS_ERR(param)) {
> +		ret = PTR_ERR(param);
>  		goto out;
>  	}

It's usually neater to avoid a blank line between the param = and the
IS_ERR check (similar to the style in the code you removed).

The same in the other fragment.

Cheers,
Rusty.

>  
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
> index 63a4cdf..eeee256 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -3789,14 +3789,10 @@ int rtl8192_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>  		goto out;
>  	}
>  
> -	ipw = kmalloc(p->length, GFP_KERNEL);
> -	if (ipw == NULL) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -	if (copy_from_user(ipw, p->pointer, p->length)) {
> -		kfree(ipw);
> -		ret = -EFAULT;
> +	ipw = memdup_user(p->pointer, p->length);
> +
> +	if (IS_ERR(ipw)) {
> +		ret = PTR_ERR(ipw);
>  		goto out;
>  	}
>  
> -- 
> 1.7.10.4
>
> -- 
> You received this message because you are subscribed to the Google Groups "opw-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to opw-kernel+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
Teodora Baluta Oct. 25, 2013, 8:12 a.m. UTC | #2
On Fri, Oct 25, 2013 at 3:56 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Teodora Baluta <teobaluta@gmail.com> writes:
>
>> Use memdup_user rather than duplicating its implementation. Fix the
>> following coccinelle warnings:
>>
>> drivers/staging/rtl8192u/r8192U_core.c:3792:7-14: WARNING opportunity for memdup_user
>> drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:3153:9-16: WARNING opportunity for memdup_user
>>
>> Signed-off-by: Teodora Baluta <teobaluta@gmail.com>
>
> Hi Teodora,
>
>         This is a nice cleanup, but one minor comment:
>
>> ---
>>  drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c |   12 ++++--------
>>  drivers/staging/rtl8192u/r8192U_core.c                 |   12 ++++--------
>>  2 files changed, 8 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
>> index 5fd6969..00b7143 100644
>> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
>> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
>> @@ -3150,14 +3150,10 @@ int ieee80211_wpa_supplicant_ioctl(struct ieee80211_device *ieee, struct iw_poin
>>               goto out;
>>       }
>>
>> -     param = kmalloc(p->length, GFP_KERNEL);
>> -     if (param == NULL){
>> -             ret = -ENOMEM;
>> -             goto out;
>> -     }
>> -     if (copy_from_user(param, p->pointer, p->length)) {
>> -             kfree(param);
>> -             ret = -EFAULT;
>> +     param = memdup_user(p->pointer, p->length);
>> +
>> +     if (IS_ERR(param)) {
>> +             ret = PTR_ERR(param);
>>               goto out;
>>       }
>
> It's usually neater to avoid a blank line between the param = and the
> IS_ERR check (similar to the style in the code you removed).
>
> The same in the other fragment.
>
> Cheers,
> Rusty.

Thank you for the feedback, Rusty! I will send a v2 patch asap. I have
a similiar patch for rtl8192e at [0]. Can I also send a v2 for that or
should I wait for a review first?

[0] https://groups.google.com/forum/#!topic/opw-kernel/kRlMzOQ7IEI

Teodora
Rusty Russell Oct. 25, 2013, noon UTC | #3
Teodora B?lu?? <teobaluta@gmail.com> writes:
> On Fri, Oct 25, 2013 at 3:56 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Teodora Baluta <teobaluta@gmail.com> writes:
>>
>>> Use memdup_user rather than duplicating its implementation. Fix the
>>> following coccinelle warnings:
>>>
>>> drivers/staging/rtl8192u/r8192U_core.c:3792:7-14: WARNING opportunity for memdup_user
>>> drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:3153:9-16: WARNING opportunity for memdup_user
>>>
>>> Signed-off-by: Teodora Baluta <teobaluta@gmail.com>
>>
>> Hi Teodora,
>>
>>         This is a nice cleanup, but one minor comment:
>>
>>> ---
>>>  drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c |   12 ++++--------
>>>  drivers/staging/rtl8192u/r8192U_core.c                 |   12 ++++--------
>>>  2 files changed, 8 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
>>> index 5fd6969..00b7143 100644
>>> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
>>> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
>>> @@ -3150,14 +3150,10 @@ int ieee80211_wpa_supplicant_ioctl(struct ieee80211_device *ieee, struct iw_poin
>>>               goto out;
>>>       }
>>>
>>> -     param = kmalloc(p->length, GFP_KERNEL);
>>> -     if (param == NULL){
>>> -             ret = -ENOMEM;
>>> -             goto out;
>>> -     }
>>> -     if (copy_from_user(param, p->pointer, p->length)) {
>>> -             kfree(param);
>>> -             ret = -EFAULT;
>>> +     param = memdup_user(p->pointer, p->length);
>>> +
>>> +     if (IS_ERR(param)) {
>>> +             ret = PTR_ERR(param);
>>>               goto out;
>>>       }
>>
>> It's usually neater to avoid a blank line between the param = and the
>> IS_ERR check (similar to the style in the code you removed).
>>
>> The same in the other fragment.
>>
>> Cheers,
>> Rusty.
>
> Thank you for the feedback, Rusty! I will send a v2 patch asap. I have
> a similiar patch for rtl8192e at [0]. Can I also send a v2 for that or
> should I wait for a review first?
>
> [0] https://groups.google.com/forum/#!topic/opw-kernel/kRlMzOQ7IEI

Saw that one, assumed you'd re-send that too.

Thanks,
Rusty.
Teodora Baluta Oct. 26, 2013, 6:21 a.m. UTC | #4
On Fri, Oct 25, 2013 at 3:00 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Teodora B?lu?? <teobaluta@gmail.com> writes:
>> On Fri, Oct 25, 2013 at 3:56 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> Teodora Baluta <teobaluta@gmail.com> writes:
>>>
>>>> Use memdup_user rather than duplicating its implementation. Fix the
>>>> following coccinelle warnings:
>>>>
>>>> drivers/staging/rtl8192u/r8192U_core.c:3792:7-14: WARNING opportunity for memdup_user
>>>> drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:3153:9-16: WARNING opportunity for memdup_user
>>>>
>>>> Signed-off-by: Teodora Baluta <teobaluta@gmail.com>
>>>
>>> Hi Teodora,
>>>
>>>         This is a nice cleanup, but one minor comment:
>>>
>>>> ---
>>>>  drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c |   12 ++++--------
>>>>  drivers/staging/rtl8192u/r8192U_core.c                 |   12 ++++--------
>>>>  2 files changed, 8 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
>>>> index 5fd6969..00b7143 100644
>>>> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
>>>> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
>>>> @@ -3150,14 +3150,10 @@ int ieee80211_wpa_supplicant_ioctl(struct ieee80211_device *ieee, struct iw_poin
>>>>               goto out;
>>>>       }
>>>>
>>>> -     param = kmalloc(p->length, GFP_KERNEL);
>>>> -     if (param == NULL){
>>>> -             ret = -ENOMEM;
>>>> -             goto out;
>>>> -     }
>>>> -     if (copy_from_user(param, p->pointer, p->length)) {
>>>> -             kfree(param);
>>>> -             ret = -EFAULT;
>>>> +     param = memdup_user(p->pointer, p->length);
>>>> +
>>>> +     if (IS_ERR(param)) {
>>>> +             ret = PTR_ERR(param);
>>>>               goto out;
>>>>       }
>>>
>>> It's usually neater to avoid a blank line between the param = and the
>>> IS_ERR check (similar to the style in the code you removed).
>>>
>>> The same in the other fragment.
>>>
>>> Cheers,
>>> Rusty.
>>
>> Thank you for the feedback, Rusty! I will send a v2 patch asap. I have
>> a similiar patch for rtl8192e at [0]. Can I also send a v2 for that or
>> should I wait for a review first?
>>
>> [0] https://groups.google.com/forum/#!topic/opw-kernel/kRlMzOQ7IEI
>
> Saw that one, assumed you'd re-send that too.
>
> Thanks,
> Rusty.

Ok, done. :)

Thanks,
Teodora
diff mbox

Patch

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
index 5fd6969..00b7143 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
@@ -3150,14 +3150,10 @@  int ieee80211_wpa_supplicant_ioctl(struct ieee80211_device *ieee, struct iw_poin
 		goto out;
 	}
 
-	param = kmalloc(p->length, GFP_KERNEL);
-	if (param == NULL){
-		ret = -ENOMEM;
-		goto out;
-	}
-	if (copy_from_user(param, p->pointer, p->length)) {
-		kfree(param);
-		ret = -EFAULT;
+	param = memdup_user(p->pointer, p->length);
+
+	if (IS_ERR(param)) {
+		ret = PTR_ERR(param);
 		goto out;
 	}
 
diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
index 63a4cdf..eeee256 100644
--- a/drivers/staging/rtl8192u/r8192U_core.c
+++ b/drivers/staging/rtl8192u/r8192U_core.c
@@ -3789,14 +3789,10 @@  int rtl8192_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 		goto out;
 	}
 
-	ipw = kmalloc(p->length, GFP_KERNEL);
-	if (ipw == NULL) {
-		ret = -ENOMEM;
-		goto out;
-	}
-	if (copy_from_user(ipw, p->pointer, p->length)) {
-		kfree(ipw);
-		ret = -EFAULT;
+	ipw = memdup_user(p->pointer, p->length);
+
+	if (IS_ERR(ipw)) {
+		ret = PTR_ERR(ipw);
 		goto out;
 	}