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

[Julia Lawall]

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@xxxxxxxxxxxxxxxxxxxxx>
> >>> 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@xxxxxxxxxxxxxxxxxxxxx>
> >>> ---
> >>>   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@xxxxxxxxxxxxxxx
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >
>
> --
> Vaishali
>