From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753083AbcHUMKC (ORCPT ); Sun, 21 Aug 2016 08:10:02 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:47402 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752307AbcHUMKA (ORCPT ); Sun, 21 Aug 2016 08:10:00 -0400 Subject: Re: [PATCH] staging/lustre/llite: Use memdup_user() rather than duplicating its implementation To: Julia Lawall References: <566ABCD9.1060404@users.sourceforge.net> <73da135c-be81-e915-9b7a-6773e730b4e7@users.sourceforge.net> <66b32046-56ee-430e-e80e-1cdd5a8e68c4@wanadoo.fr> <57B98890.7070900@oracle.com> Cc: Christophe JAILLET , kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org From: Vaishali Thakkar Message-ID: <57B99A0C.3060407@oracle.com> Date: Sun, 21 Aug 2016 17:39:48 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>>> 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 >>>>> --- >>>>> 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