From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752875AbcHUKzb (ORCPT ); Sun, 21 Aug 2016 06:55:31 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:37655 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752307AbcHUKza (ORCPT ); Sun, 21 Aug 2016 06:55:30 -0400 Subject: Re: [PATCH] staging/lustre/llite: Use memdup_user() rather than duplicating its implementation To: Julia Lawall , Christophe JAILLET References: <566ABCD9.1060404@users.sourceforge.net> <73da135c-be81-e915-9b7a-6773e730b4e7@users.sourceforge.net> <66b32046-56ee-430e-e80e-1cdd5a8e68c4@wanadoo.fr> Cc: kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org From: Vaishali Thakkar Message-ID: <57B98890.7070900@oracle.com> Date: Sun, 21 Aug 2016 16:25:12 +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: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. > 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