Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1466953 > unrolled thread

[PATCH] staging/lustre/llite: Use memdup_user() rather than duplicating its implementation

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] staging/lustre/llite: Use memdup_user() rather than  duplicating its implementation SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-21 11:50 +0200
    Re: [PATCH] staging/lustre/llite: Use memdup_user() rather than  duplicating its implementation Christophe JAILLET <christophe.jaillet@wanadoo.fr> - 2016-08-21 12:10 +0200
      Re: [PATCH] staging/lustre/llite: Use memdup_user() rather than  duplicating its implementation Julia Lawall <julia.lawall@lip6.fr> - 2016-08-21 12:40 +0200
        Re: [PATCH] staging/lustre/llite: Use memdup_user() rather than  duplicating its implementation Vaishali Thakkar <vaishali.thakkar@oracle.com> - 2016-08-21 13:00 +0200
          Re: [PATCH] staging/lustre/llite: Use memdup_user() rather than  duplicating its implementation Julia Lawall <julia.lawall@lip6.fr> - 2016-08-21 13:10 +0200
            Re: [PATCH] staging/lustre/llite: Use memdup_user() rather than  duplicating its implementation Vaishali Thakkar <vaishali.thakkar@oracle.com> - 2016-08-21 14:20 +0200

#1466953 — [PATCH] staging/lustre/llite: Use memdup_user() rather than duplicating its implementation

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 21 Aug 2016 11:30:57 +0200

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/llite/dir.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c
index 031c9e4..8b70e42 100644
--- a/drivers/staging/lustre/lustre/llite/dir.c
+++ b/drivers/staging/lustre/lustre/llite/dir.c
@@ -1676,14 +1676,9 @@ out_poll:
 	case LL_IOC_QUOTACTL: {
 		struct if_quotactl *qctl;
 
-		qctl = kzalloc(sizeof(*qctl), GFP_NOFS);
-		if (!qctl)
-			return -ENOMEM;
-
-		if (copy_from_user(qctl, (void __user *)arg, sizeof(*qctl))) {
-			rc = -EFAULT;
-			goto out_quotactl;
-		}
+		qctl = memdup_user((void __user *)arg, sizeof(*qctl));
+		if (IS_ERR(qctl))
+			return PTR_ERR(qctl);
 
 		rc = quotactl_ioctl(sbi, qctl);
 
@@ -1691,7 +1686,6 @@ out_poll:
 					    sizeof(*qctl)))
 			rc = -EFAULT;
 
-out_quotactl:
 		kfree(qctl);
 		return rc;
 	}
-- 
2.9.3

[toc] | [next] | [standalone]

#1466956

Le 21/08/2016 à 11:45, SF Markus Elfring a écrit :
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 21 Aug 2016 11:30:57 +0200
>
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/staging/lustre/lustre/llite/dir.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c
> index 031c9e4..8b70e42 100644
> --- a/drivers/staging/lustre/lustre/llite/dir.c
> +++ b/drivers/staging/lustre/lustre/llite/dir.c
> @@ -1676,14 +1676,9 @@ out_poll:
>   	case LL_IOC_QUOTACTL: {
>   		struct if_quotactl *qctl;
>   
> -		qctl = kzalloc(sizeof(*qctl), GFP_NOFS);
Same as previously reported in another patch, GFP_NOFS has not the same 
meaning than GPF_KERNEL.
So your proposed clean-up is not 100% equivalent.

Are your sure that GPF_KERNEL instead of GFP_NOFS is right in this code?

Maybe, the coccinelle check should be tweak to only spot "kzalloc(..., 
GFP_KERNEL)" allocation?

> -		if (!qctl)
> -			return -ENOMEM;
> -
> -		if (copy_from_user(qctl, (void __user *)arg, sizeof(*qctl))) {
> -			rc = -EFAULT;
> -			goto out_quotactl;
> -		}
> +		qctl = memdup_user((void __user *)arg, sizeof(*qctl));
> +		if (IS_ERR(qctl))
> +			return PTR_ERR(qctl);
>   
>   		rc = quotactl_ioctl(sbi, qctl);
>   
> @@ -1691,7 +1686,6 @@ out_poll:
>   					    sizeof(*qctl)))
>   			rc = -EFAULT;
>   
> -out_quotactl:
>   		kfree(qctl);
>   		return rc;
>   	}



---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus

[toc] | [prev] | [next] | [standalone]

#1466958

[Multipart message — attachments visible in raw view] — view raw

On Sun, 21 Aug 2016, Christophe JAILLET wrote:

> Le 21/08/2016 à 11:45, SF Markus Elfring a écrit :
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sun, 21 Aug 2016 11:30:57 +0200
> >
> > Reuse existing functionality from memdup_user() instead of keeping
> > duplicate source code.
> >
> > This issue was detected by using the Coccinelle software.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >   drivers/staging/lustre/lustre/llite/dir.c | 12 +++---------
> >   1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/staging/lustre/lustre/llite/dir.c
> > b/drivers/staging/lustre/lustre/llite/dir.c
> > index 031c9e4..8b70e42 100644
> > --- a/drivers/staging/lustre/lustre/llite/dir.c
> > +++ b/drivers/staging/lustre/lustre/llite/dir.c
> > @@ -1676,14 +1676,9 @@ out_poll:
> >   	case LL_IOC_QUOTACTL: {
> >   		struct if_quotactl *qctl;
> >   -		qctl = kzalloc(sizeof(*qctl), GFP_NOFS);
> Same as previously reported in another patch, GFP_NOFS has not the same
> meaning than GPF_KERNEL.
> So your proposed clean-up is not 100% equivalent.
>
> Are your sure that GPF_KERNEL instead of GFP_NOFS is right in this code?
>
> Maybe, the coccinelle check should be tweak to only spot "kzalloc(...,
> GFP_KERNEL)" allocation?

To my dim recollection, GFP_NOFS is not actually allowed in a place where
copy_from_user is being used.  copy_from_user can block due to page
faults, and GFP_NOFS is used when a certain kind of blocking is not
allowed.  So if the code really needs GFP_NOFS, then something else is
wrong.

The semantic patch intentionally does not specify GFP_KERNEL for this
reason, ie so that these issues will come up and be discussed.  On the
ther hand I agree about the GFP_DMA case, since that doesn't relate to
blocking, as far as I know.  The semantic patch should be updated to not
make/propose the change in that case.

julia

>
> > -		if (!qctl)
> > -			return -ENOMEM;
> > -
> > -		if (copy_from_user(qctl, (void __user *)arg, sizeof(*qctl))) {
> > -			rc = -EFAULT;
> > -			goto out_quotactl;
> > -		}
> > +		qctl = memdup_user((void __user *)arg, sizeof(*qctl));
> > +		if (IS_ERR(qctl))
> > +			return PTR_ERR(qctl);
> >     		rc = quotactl_ioctl(sbi, qctl);
> >   @@ -1691,7 +1686,6 @@ out_poll:
> >   					    sizeof(*qctl)))
> >   			rc = -EFAULT;
> >   -out_quotactl:
> >   		kfree(qctl);
> >   		return rc;
> >   	}
>
>
>
> ---
> L'absence de virus dans ce courrier électronique a été vérifiée par le
> logiciel antivirus Avast.
> https://www.avast.com/antivirus
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [next] | [standalone]

#1466960

On Sunday 21 August 2016 04:01 PM, Julia Lawall wrote:
> 
> 
> On Sun, 21 Aug 2016, Christophe JAILLET wrote:
> 
>> Le 21/08/2016 à 11:45, SF Markus Elfring a écrit :
>>> From: Markus Elfring <elfring@users.sourceforge.net>
>>> Date: Sun, 21 Aug 2016 11:30:57 +0200
>>>
>>> Reuse existing functionality from memdup_user() instead of keeping
>>> duplicate source code.
>>>
>>> This issue was detected by using the Coccinelle software.
>>>
>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>>> ---
>>>   drivers/staging/lustre/lustre/llite/dir.c | 12 +++---------
>>>   1 file changed, 3 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/staging/lustre/lustre/llite/dir.c
>>> b/drivers/staging/lustre/lustre/llite/dir.c
>>> index 031c9e4..8b70e42 100644
>>> --- a/drivers/staging/lustre/lustre/llite/dir.c
>>> +++ b/drivers/staging/lustre/lustre/llite/dir.c
>>> @@ -1676,14 +1676,9 @@ out_poll:
>>>   	case LL_IOC_QUOTACTL: {
>>>   		struct if_quotactl *qctl;
>>>   -		qctl = kzalloc(sizeof(*qctl), GFP_NOFS);
>> Same as previously reported in another patch, GFP_NOFS has not the same
>> meaning than GPF_KERNEL.
>> So your proposed clean-up is not 100% equivalent.
>>
>> Are your sure that GPF_KERNEL instead of GFP_NOFS is right in this code?
>>
>> Maybe, the coccinelle check should be tweak to only spot "kzalloc(...,
>> GFP_KERNEL)" allocation?
> 
> To my dim recollection, GFP_NOFS is not actually allowed in a place where
> copy_from_user is being used.  copy_from_user can block due to page
> faults, and GFP_NOFS is used when a certain kind of blocking is not
> allowed.  So if the code really needs GFP_NOFS, then something else is
> wrong.
> 
> The semantic patch intentionally does not specify GFP_KERNEL for this
> reason, ie so that these issues will come up and be discussed.  On the
> ther hand I agree about the GFP_DMA case, since that doesn't relate to
> blocking, as far as I know.  The semantic patch should be updated to not
> make/propose the change in that case.

I think semantic patch should be updated for all possible flags except
GFP_NOFS and GFP_ATOMIC. Because only using these 2 flags with
copy_from_user can cause blocking.

> julia
> 
>>
>>> -		if (!qctl)
>>> -			return -ENOMEM;
>>> -
>>> -		if (copy_from_user(qctl, (void __user *)arg, sizeof(*qctl))) {
>>> -			rc = -EFAULT;
>>> -			goto out_quotactl;
>>> -		}
>>> +		qctl = memdup_user((void __user *)arg, sizeof(*qctl));
>>> +		if (IS_ERR(qctl))
>>> +			return PTR_ERR(qctl);
>>>     		rc = quotactl_ioctl(sbi, qctl);
>>>   @@ -1691,7 +1686,6 @@ out_poll:
>>>   					    sizeof(*qctl)))
>>>   			rc = -EFAULT;
>>>   -out_quotactl:
>>>   		kfree(qctl);
>>>   		return rc;
>>>   	}
>>
>>
>>
>> ---
>> L'absence de virus dans ce courrier électronique a été vérifiée par le
>> logiciel antivirus Avast.
>> https://www.avast.com/antivirus
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

-- 
Vaishali

[toc] | [prev] | [next] | [standalone]

#1466962

[Multipart message — attachments visible in raw view] — view raw

On Sun, 21 Aug 2016, Vaishali Thakkar wrote:

>
>
> On Sunday 21 August 2016 04:01 PM, Julia Lawall wrote:
> >
> >
> > On Sun, 21 Aug 2016, Christophe JAILLET wrote:
> >
> >> Le 21/08/2016 à 11:45, SF Markus Elfring a écrit :
> >>> From: Markus Elfring <elfring@users.sourceforge.net>
> >>> Date: Sun, 21 Aug 2016 11:30:57 +0200
> >>>
> >>> Reuse existing functionality from memdup_user() instead of keeping
> >>> duplicate source code.
> >>>
> >>> This issue was detected by using the Coccinelle software.
> >>>
> >>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> >>> ---
> >>>   drivers/staging/lustre/lustre/llite/dir.c | 12 +++---------
> >>>   1 file changed, 3 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/lustre/lustre/llite/dir.c
> >>> b/drivers/staging/lustre/lustre/llite/dir.c
> >>> index 031c9e4..8b70e42 100644
> >>> --- a/drivers/staging/lustre/lustre/llite/dir.c
> >>> +++ b/drivers/staging/lustre/lustre/llite/dir.c
> >>> @@ -1676,14 +1676,9 @@ out_poll:
> >>>   	case LL_IOC_QUOTACTL: {
> >>>   		struct if_quotactl *qctl;
> >>>   -		qctl = kzalloc(sizeof(*qctl), GFP_NOFS);
> >> Same as previously reported in another patch, GFP_NOFS has not the same
> >> meaning than GPF_KERNEL.
> >> So your proposed clean-up is not 100% equivalent.
> >>
> >> Are your sure that GPF_KERNEL instead of GFP_NOFS is right in this code?
> >>
> >> Maybe, the coccinelle check should be tweak to only spot "kzalloc(...,
> >> GFP_KERNEL)" allocation?
> >
> > To my dim recollection, GFP_NOFS is not actually allowed in a place where
> > copy_from_user is being used.  copy_from_user can block due to page
> > faults, and GFP_NOFS is used when a certain kind of blocking is not
> > allowed.  So if the code really needs GFP_NOFS, then something else is
> > wrong.
> >
> > The semantic patch intentionally does not specify GFP_KERNEL for this
> > reason, ie so that these issues will come up and be discussed.  On the
> > ther hand I agree about the GFP_DMA case, since that doesn't relate to
> > blocking, as far as I know.  The semantic patch should be updated to not
> > make/propose the change in that case.
>
> I think semantic patch should be updated for all possible flags except
> GFP_NOFS and GFP_ATOMIC. Because only using these 2 flags with
> copy_from_user can cause blocking.

They don't cause blocking, but rather prevent it.

But people could use variables as well.  Since other things are rare, it
seems like it could be better to only block reports on what is known to
be safe, and not to block reports on unknown things.  A warning could be
given in some cases.

julia

>
> > julia
> >
> >>
> >>> -		if (!qctl)
> >>> -			return -ENOMEM;
> >>> -
> >>> -		if (copy_from_user(qctl, (void __user *)arg, sizeof(*qctl))) {
> >>> -			rc = -EFAULT;
> >>> -			goto out_quotactl;
> >>> -		}
> >>> +		qctl = memdup_user((void __user *)arg, sizeof(*qctl));
> >>> +		if (IS_ERR(qctl))
> >>> +			return PTR_ERR(qctl);
> >>>     		rc = quotactl_ioctl(sbi, qctl);
> >>>   @@ -1691,7 +1686,6 @@ out_poll:
> >>>   					    sizeof(*qctl)))
> >>>   			rc = -EFAULT;
> >>>   -out_quotactl:
> >>>   		kfree(qctl);
> >>>   		return rc;
> >>>   	}
> >>
> >>
> >>
> >> ---
> >> L'absence de virus dans ce courrier électronique a été vérifiée par le
> >> logiciel antivirus Avast.
> >> https://www.avast.com/antivirus
> >>
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >
>
> --
> Vaishali
>

[toc] | [prev] | [next] | [standalone]

#1466971

On Sunday 21 August 2016 04:31 PM, Julia Lawall wrote:
> 
> 
> On Sun, 21 Aug 2016, Vaishali Thakkar wrote:
> 
>>
>>
>> On Sunday 21 August 2016 04:01 PM, Julia Lawall wrote:
>>>
>>>
>>> On Sun, 21 Aug 2016, Christophe JAILLET wrote:
>>>
>>>> Le 21/08/2016 à 11:45, SF Markus Elfring a écrit :
>>>>> From: Markus Elfring <elfring@users.sourceforge.net>
>>>>> Date: Sun, 21 Aug 2016 11:30:57 +0200
>>>>>
>>>>> Reuse existing functionality from memdup_user() instead of keeping
>>>>> duplicate source code.
>>>>>
>>>>> This issue was detected by using the Coccinelle software.
>>>>>
>>>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>>>>> ---
>>>>>   drivers/staging/lustre/lustre/llite/dir.c | 12 +++---------
>>>>>   1 file changed, 3 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/lustre/lustre/llite/dir.c
>>>>> b/drivers/staging/lustre/lustre/llite/dir.c
>>>>> index 031c9e4..8b70e42 100644
>>>>> --- a/drivers/staging/lustre/lustre/llite/dir.c
>>>>> +++ b/drivers/staging/lustre/lustre/llite/dir.c
>>>>> @@ -1676,14 +1676,9 @@ out_poll:
>>>>>   	case LL_IOC_QUOTACTL: {
>>>>>   		struct if_quotactl *qctl;
>>>>>   -		qctl = kzalloc(sizeof(*qctl), GFP_NOFS);
>>>> Same as previously reported in another patch, GFP_NOFS has not the same
>>>> meaning than GPF_KERNEL.
>>>> So your proposed clean-up is not 100% equivalent.
>>>>
>>>> Are your sure that GPF_KERNEL instead of GFP_NOFS is right in this code?
>>>>
>>>> Maybe, the coccinelle check should be tweak to only spot "kzalloc(...,
>>>> GFP_KERNEL)" allocation?
>>>
>>> To my dim recollection, GFP_NOFS is not actually allowed in a place where
>>> copy_from_user is being used.  copy_from_user can block due to page
>>> faults, and GFP_NOFS is used when a certain kind of blocking is not
>>> allowed.  So if the code really needs GFP_NOFS, then something else is
>>> wrong.
>>>
>>> The semantic patch intentionally does not specify GFP_KERNEL for this
>>> reason, ie so that these issues will come up and be discussed.  On the
>>> ther hand I agree about the GFP_DMA case, since that doesn't relate to
>>> blocking, as far as I know.  The semantic patch should be updated to not
>>> make/propose the change in that case.
>>
>> I think semantic patch should be updated for all possible flags except
>> GFP_NOFS and GFP_ATOMIC. Because only using these 2 flags with
>> copy_from_user can cause blocking.
> 
> They don't cause blocking, but rather prevent it.

Yes, sorry. I meant blocking functions which may sleep in between.

> But people could use variables as well.  Since other things are rare, it
> seems like it could be better to only block reports on what is known to
> be safe, and not to block reports on unknown things.  A warning could be
> given in some cases.

Sounds reasonable. Warning will work I guess.

> julia
> 
>>
>>> julia
>>>
>>>>
>>>>> -		if (!qctl)
>>>>> -			return -ENOMEM;
>>>>> -
>>>>> -		if (copy_from_user(qctl, (void __user *)arg, sizeof(*qctl))) {
>>>>> -			rc = -EFAULT;
>>>>> -			goto out_quotactl;
>>>>> -		}
>>>>> +		qctl = memdup_user((void __user *)arg, sizeof(*qctl));
>>>>> +		if (IS_ERR(qctl))
>>>>> +			return PTR_ERR(qctl);
>>>>>     		rc = quotactl_ioctl(sbi, qctl);
>>>>>   @@ -1691,7 +1686,6 @@ out_poll:
>>>>>   					    sizeof(*qctl)))
>>>>>   			rc = -EFAULT;
>>>>>   -out_quotactl:
>>>>>   		kfree(qctl);
>>>>>   		return rc;
>>>>>   	}
>>>>
>>>>
>>>>
>>>> ---
>>>> L'absence de virus dans ce courrier électronique a été vérifiée par le
>>>> logiciel antivirus Avast.
>>>> https://www.avast.com/antivirus
>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>
>> --
>> Vaishali
>>
> 

-- 
Vaishali

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web