Message ID | c7bfaf30cb682b92766e35ec85d93a84798b37f4.1696174961.git.lstoakes@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2a8e:b0:403:3b70:6f57 with SMTP id in14csp1060440vqb; Sun, 1 Oct 2023 14:26:36 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFpqobR5h4fecD8Enk1Oughttrr1rYfq+APSbBjhqLf7gdEmtyirwMt+3yvjaKQs+euMAVM X-Received: by 2002:a17:903:181:b0:1c1:f27e:a55a with SMTP id z1-20020a170903018100b001c1f27ea55amr9880725plg.46.1696195596481; Sun, 01 Oct 2023 14:26:36 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1696195596; cv=none; d=google.com; s=arc-20160816; b=ffIHHgyAlnegOoUy03r0DpLRpd3SATuNqeuz8z946OefPqr3BvHwhRImoUytLG+GxF 1tT8SiiiZA3LbPAQT++MrSj5Ef/vNaNV7Aga6kXMefJ3Lb+Vmae5Ki5l3z5VoTYJFm4c XiSYlYCnW4j3LNzTHagjSO7yOhmVvQgNMZv5SZ3TS3agIwiQGtIEXO0ep8d+wlWiiOne VouIMzqZQSKqVBphToZhtlQAq4Umf7I6zTxtY7P6IbjwRksu0BCvTrNf9rCO6NfCyoM+ RN8CXOkJpPKipu9l4jEj4VhYcvqrsmO7bsiI29Ajm1YTyCSlKctaIfoyWlrGUsnrC5A5 km4g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=1xEwOtL01+HmLCODlG5jDnboU50nYLcrpSFuPs0VWD8=; fh=mNvOpR73OaUnRrr53gRvslddDUjaIrpQi7RNpkp0Veo=; b=sjqNA4xxeL87bhTnNUDEnS5YnRFYDTmg/c4s8H2+AUv3NhqpqSVK2NcGWkhvGbQEmQ 4EGgQBCSFgStnK37HJjxw+scNxwNmE9qyyFLy3CgrweDPqnAUvd1EVSHmmZ2KM0UcRBN eMC23aBWoRakig2Yp/QDczJxD02WoFYTt99BMbMfiAL49+01sx4GVHxGjupM8yeLPvhT Yx6Hb2FeC/z7PgffsbsWpymmH1JlXwOmYSWCeYt7EKJxehfK5jzZcMWxKehcLNoxP54u vrG73P6ZeTaCzlzf7aAkP8NXBWn9vOC0PaMqcZQ9ikg5ixerl7A752dgNAD+DkIZTvaK earg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=hFnRHMmA; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from howler.vger.email (howler.vger.email. [23.128.96.34]) by mx.google.com with ESMTPS id j4-20020a170903028400b001b9c992945fsi26473915plr.555.2023.10.01.14.26.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 01 Oct 2023 14:26:36 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=hFnRHMmA; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by howler.vger.email (Postfix) with ESMTP id AC62B8074526; Sun, 1 Oct 2023 09:00:41 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at howler.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235188AbjJAQAZ (ORCPT <rfc822;chrisfriedt@gmail.com> + 18 others); Sun, 1 Oct 2023 12:00:25 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54020 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235192AbjJAQAR (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sun, 1 Oct 2023 12:00:17 -0400 Received: from mail-wm1-x331.google.com (mail-wm1-x331.google.com [IPv6:2a00:1450:4864:20::331]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 12817DA for <linux-kernel@vger.kernel.org>; Sun, 1 Oct 2023 09:00:15 -0700 (PDT) Received: by mail-wm1-x331.google.com with SMTP id 5b1f17b1804b1-4066241289bso17466035e9.0 for <linux-kernel@vger.kernel.org>; Sun, 01 Oct 2023 09:00:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1696176013; x=1696780813; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=1xEwOtL01+HmLCODlG5jDnboU50nYLcrpSFuPs0VWD8=; b=hFnRHMmA4lo/2w3tKnAYh6JXvlNSDY4KQCmh/Tj9yi79eMbL0j0ysTNz9uh2PRMfb2 mg80XnivtwqN5F4Wp+d6RIzGD8eXpq6X6V9o/e9oYYDJ0ES+wsce6/0qHMZ6Rg4G6soe MlJKbxZzzdxActTVwKf9r6Y2k9BacE+6wdzjshE6szpq6VyY+7HXO3VKSZJ6w3IGQoBc QZkXfVphMGrb8J4RMtzY+cIWy6QsN08VVUAUGnFmfHLRrIQK+xIU0X8EX91hOx0uRY0d SxBhubrp5oo8i2Lc1wo8BlxaqXTeT8LyEN2zrytq6Mw8tKiEZH8CHZh1vpFzxPk5hxId BaFw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696176013; x=1696780813; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=1xEwOtL01+HmLCODlG5jDnboU50nYLcrpSFuPs0VWD8=; b=rUCFtHkrGWLfBbXdQOGVBbqpDk0I+IBJalkcRETJzWJ9DhgHRY9Z1h139LlFXpN8Ed /em/spHd/1eQlGwhs1UWR1Yr3Vsg55IzeoyUwhBng8jKdE1AoanpCnFW9/w+LBORGavO kxJbHPgk4FidwRcCtQIXbQ05CNYCA5kbAQvAQjUBzpRLO9iVR2k35rWgVz7BBlIaB/8O G0dckJK1iiYeG3MrOpEYtiRrUOToN0T/3cGuxujEyK5R+p7/PZs86VnhSdi6u6nYadll oGbA/vOY5U8qMgQwAkTJitk/JM791ceNVX9gGH/SfBeZ35hVF4xO9QwcQL3BJT7+zO2p qX/A== X-Gm-Message-State: AOJu0YxDpPk02ehq68fwh/1uBEdWUhlgu3dp3UIc8hv/6XCCX9BS7dcl 06AsEV0RFwMSEwekV4NssbQ= X-Received: by 2002:a1c:7407:0:b0:3fe:d1b9:7ea9 with SMTP id p7-20020a1c7407000000b003fed1b97ea9mr7951514wmc.36.1696176013401; Sun, 01 Oct 2023 09:00:13 -0700 (PDT) Received: from lucifer.home ([2a00:23c5:dc8c:8701:1663:9a35:5a7b:1d76]) by smtp.googlemail.com with ESMTPSA id s16-20020a05600c045000b0040536dcec17sm5487695wmb.27.2023.10.01.09.00.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 01 Oct 2023 09:00:12 -0700 (PDT) From: Lorenzo Stoakes <lstoakes@gmail.com> To: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, Mark Rutland <mark.rutland@arm.com> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>, Ian Rogers <irogers@google.com>, Adrian Hunter <adrian.hunter@intel.com>, Oleg Nesterov <oleg@redhat.com>, Richard Cochran <richardcochran@gmail.com>, Jason Gunthorpe <jgg@nvidia.com>, John Hubbard <jhubbard@nvidia.com>, Arnd Bergmann <arnd@arndb.de>, Lorenzo Stoakes <lstoakes@gmail.com> Subject: [PATCH 3/4] mm/gup: make failure to pin an error if FOLL_NOWAIT not specified Date: Sun, 1 Oct 2023 17:00:04 +0100 Message-ID: <c7bfaf30cb682b92766e35ec85d93a84798b37f4.1696174961.git.lstoakes@gmail.com> X-Mailer: git-send-email 2.42.0 In-Reply-To: <cover.1696174961.git.lstoakes@gmail.com> References: <cover.1696174961.git.lstoakes@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Sun, 01 Oct 2023 09:00:41 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778589993769695489 X-GMAIL-MSGID: 1778589993769695489 |
Series |
various improvements to the GUP interface
|
|
Commit Message
Lorenzo Stoakes
Oct. 1, 2023, 4 p.m. UTC
There really should be no circumstances under which a non-FOLL_NOWAIT GUP
operation fails to return any pages, so make this an error.
To catch the trivial case, simply exit early if nr_pages == 0.
This brings __get_user_pages_locked() in line with the behaviour of its
nommu variant.
Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
---
mm/gup.c | 11 +++++++++++
1 file changed, 11 insertions(+)
Comments
On 01.10.23 18:00, Lorenzo Stoakes wrote: > There really should be no circumstances under which a non-FOLL_NOWAIT GUP > operation fails to return any pages, so make this an error. > > To catch the trivial case, simply exit early if nr_pages == 0. > > This brings __get_user_pages_locked() in line with the behaviour of its > nommu variant. > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> > --- > mm/gup.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/mm/gup.c b/mm/gup.c > index b21b33d1787e..fb2218d74ca5 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1471,6 +1471,9 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, > long ret, pages_done; > bool must_unlock = false; > > + if (!nr_pages) > + return 0; > + Probably unlikely() is reasonable. I even wonder if WARN_ON_ONCE() would be appropriate, but likely there are weird callers that end up calling this with nr_pages==0 ... probably they should be identified and changed. Future work. > /* > * The internal caller expects GUP to manage the lock internally and the > * lock must be released when this returns. > @@ -1595,6 +1598,14 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, > mmap_read_unlock(mm); > *locked = 0; > } > + > + /* > + * Failing to pin anything implies something has gone wrong except when > + * FOLL_NOWAIT is specified, so explicitly make this an error. > + */ > + if (pages_done == 0 && !(flags & FOLL_NOWAIT)) > + return -EFAULT; > + But who would be affected by that and why do we care about adding this check? This smells like a "if (WARN_ON_ONCE())", correct?
On Mon, Oct 02, 2023 at 01:04:51PM +0200, David Hildenbrand wrote: > On 01.10.23 18:00, Lorenzo Stoakes wrote: > > There really should be no circumstances under which a non-FOLL_NOWAIT GUP > > operation fails to return any pages, so make this an error. > > > > To catch the trivial case, simply exit early if nr_pages == 0. > > > > This brings __get_user_pages_locked() in line with the behaviour of its > > nommu variant. > > > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> > > --- > > mm/gup.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/mm/gup.c b/mm/gup.c > > index b21b33d1787e..fb2218d74ca5 100644 > > --- a/mm/gup.c > > +++ b/mm/gup.c > > @@ -1471,6 +1471,9 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, > > long ret, pages_done; > > bool must_unlock = false; > > + if (!nr_pages) > > + return 0; > > + > > Probably unlikely() is reasonable. I even wonder if WARN_ON_ONCE() would be > appropriate, but likely there are weird callers that end up calling this > with nr_pages==0 ... probably they should be identified and changed. Future > work. > > > /* > > * The internal caller expects GUP to manage the lock internally and the > > * lock must be released when this returns. > > @@ -1595,6 +1598,14 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, > > mmap_read_unlock(mm); > > *locked = 0; > > } > > + > > + /* > > + * Failing to pin anything implies something has gone wrong except when > > + * FOLL_NOWAIT is specified, so explicitly make this an error. > > + */ > > + if (pages_done == 0 && !(flags & FOLL_NOWAIT)) > > + return -EFAULT; > > + > > But who would be affected by that and why do we care about adding this > check? > > This smells like a "if (WARN_ON_ONCE())", correct? Sure it does somewhat, however there are 'ordinary' (maybe) scenarios where this could possibly happen - FOLL_UNLOCKABLE and __get_user_pages() returns 0, or lock retained for non-FOLL_NOWAIT scenario and __get_user_pages() 0 also. So I think the safest option might be to leave without-WARN, however you could argue since we're making it an error now maybe we want to draw attention to it by warning. I just want to avoid a warning that _might_ be a product of a particular faulting scenario. Jason or John may have an opinion on this. Actually having written all this, given we're changing this into an error now anyway and this is just not a correct or expected scenario, yeah I think WARN_ON_ONCE() would make sense, will update on v2. > > -- > Cheers, > > David / dhildenb >
On Mon, Oct 02, 2023 at 11:51:04PM +0100, Lorenzo Stoakes wrote: > > > diff --git a/mm/gup.c b/mm/gup.c > > > index b21b33d1787e..fb2218d74ca5 100644 > > > --- a/mm/gup.c > > > +++ b/mm/gup.c > > > @@ -1471,6 +1471,9 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, > > > long ret, pages_done; > > > bool must_unlock = false; > > > + if (!nr_pages) > > > + return 0; > > > + > > > > Probably unlikely() is reasonable. I even wonder if WARN_ON_ONCE() would be > > appropriate, but likely there are weird callers that end up calling this > > with nr_pages==0 ... probably they should be identified and changed. Future > > work. > > > > > /* > > > * The internal caller expects GUP to manage the lock internally and the > > > * lock must be released when this returns. > > > @@ -1595,6 +1598,14 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, > > > mmap_read_unlock(mm); > > > *locked = 0; > > > } > > > + > > > + /* > > > + * Failing to pin anything implies something has gone wrong except when > > > + * FOLL_NOWAIT is specified, so explicitly make this an error. > > > + */ > > > + if (pages_done == 0 && !(flags & FOLL_NOWAIT)) > > > + return -EFAULT; > > > + > > > > But who would be affected by that and why do we care about adding this > > check? > > > > This smells like a "if (WARN_ON_ONCE())", correct? > > Sure it does somewhat, however there are 'ordinary' (maybe) scenarios where > this could possibly happen - FOLL_UNLOCKABLE and __get_user_pages() returns > 0, or lock retained for non-FOLL_NOWAIT scenario and __get_user_pages() 0 > also. > > So I think the safest option might be to leave without-WARN, however you > could argue since we're making it an error now maybe we want to draw > attention to it by warning. > > I just want to avoid a warning that _might_ be a product of a particular > faulting scenario. > > Jason or John may have an opinion on this. Ideally the subfunctions would never return 0 when they are not supposed to return zero and this would be a warn on to try to enforce that. There should be a clear limited set of flags where the caller is expected to handle a 0 return - and those flags should have guidance what the caller should do to handle it.. Jason
diff --git a/mm/gup.c b/mm/gup.c index b21b33d1787e..fb2218d74ca5 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1471,6 +1471,9 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, long ret, pages_done; bool must_unlock = false; + if (!nr_pages) + return 0; + /* * The internal caller expects GUP to manage the lock internally and the * lock must be released when this returns. @@ -1595,6 +1598,14 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, mmap_read_unlock(mm); *locked = 0; } + + /* + * Failing to pin anything implies something has gone wrong except when + * FOLL_NOWAIT is specified, so explicitly make this an error. + */ + if (pages_done == 0 && !(flags & FOLL_NOWAIT)) + return -EFAULT; + return pages_done; }