From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id B3937C433EF for ; Fri, 28 Jan 2022 02:31:31 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0825A6B009E; Thu, 27 Jan 2022 21:31:31 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 032786B00A0; Thu, 27 Jan 2022 21:31:30 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E15256B00A2; Thu, 27 Jan 2022 21:31:30 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0231.hostedemail.com [216.40.44.231]) by kanga.kvack.org (Postfix) with ESMTP id CC1636B009E for ; Thu, 27 Jan 2022 21:31:30 -0500 (EST) Received: from smtpin20.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 74A4A95B05 for ; Fri, 28 Jan 2022 02:31:30 +0000 (UTC) X-FDA: 79078119540.20.AF3DCFC Received: from mail-qt1-f174.google.com (mail-qt1-f174.google.com [209.85.160.174]) by imf26.hostedemail.com (Postfix) with ESMTP id AB93F140014 for ; Fri, 28 Jan 2022 02:31:29 +0000 (UTC) Received: by mail-qt1-f174.google.com with SMTP id w6so4131650qtk.4 for ; Thu, 27 Jan 2022 18:31:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ziepe.ca; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=fBSO0JiP6iYifSeVX8bHYUDeIjW8UqgFRfdhyYm6FEY=; b=Ll1BPYztCwKN6N7nCGaA9PZyCjGwY2kDFSQFBIwBU2ZEe+PMlt0fFB0PlPPP01Jnn4 Xs3APSXSq+JPafLMGlhxtB0OyAOZyablpPRvlxqgUf1w9iiGKMOyOjJPtfzdPdZmKklN gQrCFgKs+ryFNT60w3Plxsrzx0YoRqgPr9N43osfxqoo+JNih+GxOqxYnF/ISfiPuKIq vwsrQPTnQvx27gUit3Fv9IMt88DNub1hn/kPm8m6jUdxnPidt097ZeUxjzepOl4R02+A WKE2Z24x9hnyqQa/8ufwmlj/kITQo7rYeLRRCSiQ/HVvHoyOUjVN0UV1hyVATYz1QZ1Y e9MA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=fBSO0JiP6iYifSeVX8bHYUDeIjW8UqgFRfdhyYm6FEY=; b=bjqwD7AtkIfkp5NGzHVeoc0cBNV49HZkQ/7cYwIMT7v8e3ZQLxiA74+KaV/a1pIx0Y EgNTbXiNgi0SGz7lvHYmLw1H3Bqacb1QhcaJQGf8Rmr7k52rfUMdxQsFwyXo88+7OxP4 EnolIRgSMvWdIC7y2YiW9HkiH3eADvlk1afFS4RmCXKWdR7r6pY5hbH2D4hmSIRy8SLO OJrCpJ8esAF0y8NEgF1PAT2ohmk8DfSGr38cpQ/zQApFKD082LmZD6aquECJEj8cYY0j 0VowBCfBNCKQFxC4YeUBGODo3KUIl5sMg8VT8L5Gw7p0dZRugE6G0VR0wM+mb9UT/zXV ZE5Q== X-Gm-Message-State: AOAM533EWbshRkFMt68BociV04ta+zObYpac1Ui5VhNiyfQF16C4O0Ax QQ8SqBkuiIWMFCSKttuSgXXwlg== X-Google-Smtp-Source: ABdhPJzcyx9s/wHO/0iOPgK//H2ExJco2Rul/11ythT7/+GThiDuJLRvCchrLXYN9n8ZnQTzLslwlw== X-Received: by 2002:a05:622a:1a0b:: with SMTP id f11mr4849070qtb.329.1643337088807; Thu, 27 Jan 2022 18:31:28 -0800 (PST) Received: from ziepe.ca (hlfxns017vw-142-162-113-129.dhcp-dynamic.fibreop.ns.bellaliant.net. [142.162.113.129]) by smtp.gmail.com with ESMTPSA id h9sm2465080qkn.54.2022.01.27.18.31.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 Jan 2022 18:31:28 -0800 (PST) Received: from jgg by mlx with local (Exim 4.94) (envelope-from ) id 1nDH2t-007K8d-88; Thu, 27 Jan 2022 22:31:27 -0400 Date: Thu, 27 Jan 2022 22:31:27 -0400 From: Jason Gunthorpe To: Peter Xu Cc: John Hubbard , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Andrea Arcangeli , Jan Kara , =?utf-8?B?SsOpcsO0bWU=?= Glisse , "Kirill A . Shutemov" , Alex Williamson Subject: Re: [PATCH] mm: Fix invalid page pointer returned with FOLL_PIN gups Message-ID: <20220128023127.GR8034@ziepe.ca> References: <20220125033700.69705-1-peterx@redhat.com> <20220127004206.GP8034@ziepe.ca> <20220127152538.GQ8034@ziepe.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Stat-Signature: nhte15dede6ku7ca45uwsxpobsqsis3y X-Rspam-User: nil Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=ziepe.ca header.s=google header.b=Ll1BPYzt; spf=pass (imf26.hostedemail.com: domain of jgg@ziepe.ca designates 209.85.160.174 as permitted sender) smtp.mailfrom=jgg@ziepe.ca; dmarc=none X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: AB93F140014 X-HE-Tag: 1643337089-712908 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Jan 28, 2022 at 09:36:14AM +0800, Peter Xu wrote: > On Thu, Jan 27, 2022 at 11:25:38AM -0400, Jason Gunthorpe wrote: > > On Thu, Jan 27, 2022 at 05:19:56PM +0800, Peter Xu wrote: > > > > > > > > diff --git a/mm/gup.c b/mm/gup.c > > > > > > index f0af462ac1e2..8ebc04058e97 100644 > > > > > > +++ b/mm/gup.c > > > > > > @@ -440,7 +440,7 @@ static int follow_pfn_pte(struct vm_area_struct *vma, unsigned long address, > > > > > > pte_t *pte, unsigned int flags) > > > > > > { > > > > > > /* No page to get reference */ > > > > > > - if (flags & FOLL_GET) > > > > > > + if (flags & (FOLL_GET | FOLL_PIN)) > > > > > > return -EFAULT; > > > > > > > > > > Yes. This clearly fixes the problem that the patch describes, and also > > > > > clearly matches up with the Fixes tag. So that's correct. > > > > > > > > It is a really confusing though, why not just always return -EEXIST > > > > here? > > > > > > Because in current code GUP handles -EEXIST and -EFAULT differently? > > > > That has nothing to do with here. We shouldn't be deciding what the > > top layer does way down here. Return the correct error code for what > > was discovered at this layer the upper loop should make the decision > > what it should do > > > > > We do early bail out on -EFAULT. -EEXIST was first introduced in 2015 from > > > Kirill for not failing some mlock() or mmap(MAP_POPULATE) on dax (1027e4436b6). > > > Then in 2017 it got used again with pud-sized thp (a00cc7d9dd93d) on dax too. > > > They seem to service the same goal and it seems to be designed that -EEXIST > > > shouldn't fail GUP immediately. > > > > It must fail GUP immeidately if there is a pages list. > > Right, but my point is we don't have an user at all for follow_page_mask() > returning -EEXIST with a **page which is non-NULL. Or did I miss > it? We don't, but that isn't the point - it is about understandability not utility. There are only two call chains that involve follow_page_mask(), one always wants the -EEXIST and the other wants -EEXIST sometimes and sometimes wants to return -EFAULT for -EEXIST The solution is not to change follow_page_mask() but to be consistent throughout that -EEXIST means we encountered a populated but non-struct page PTE and the single place that wants -EEXIST to be reported as -EFAULT (__get_user_pages) should make that transformation. For instance, should we return -EEXIST in other cases like devmap and more that have PTEs present but are not returnable? It is already really confusing (and probably wrong) that we sometimes return NULL for populated PTEs. NULL causes faultin_page() to be called on something we already know is populated, seems like nonsense. I certainly don't want to see every place like that doing some if to transform -EEXIST into -EFAULT. > > Callers that want an early failure must pass in NULL for pages, it is > > just that simple. It has nothing to do with the FOLL flags. > > > > A WARN_ON would be appropriate to compare the FOLL flags against the > > pages. eg FOLL_GET without a pages is nonsense and should be > > immediately aborted. On the other hand, we avoid this by construction > > internal to gup.c > > We have something like that already, although it's only a VM_BUG_ON() not a > BUG_ON() or WARN_ON() at the entry of __get_user_pages(): > > VM_BUG_ON(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN))); Right > I believe this works too and I think I get your point, but as stated above it's > just not used yet so the path is not useful to any real code path. It is not about useful, it is about understandable and consistency.. There should be clear rules about when and what errno to return in every case, we should trend in that direction. Jason