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 B95D0C433EF for ; Thu, 27 Jan 2022 15:25:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E70076B0083; Thu, 27 Jan 2022 10:25:41 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E1C426B0087; Thu, 27 Jan 2022 10:25:41 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CE3076B0088; Thu, 27 Jan 2022 10:25:41 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0130.hostedemail.com [216.40.44.130]) by kanga.kvack.org (Postfix) with ESMTP id BF4596B0083 for ; Thu, 27 Jan 2022 10:25:41 -0500 (EST) Received: from smtpin17.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 80EC68CE34 for ; Thu, 27 Jan 2022 15:25:41 +0000 (UTC) X-FDA: 79076441682.17.0509A03 Received: from mail-qv1-f52.google.com (mail-qv1-f52.google.com [209.85.219.52]) by imf14.hostedemail.com (Postfix) with ESMTP id 09334100023 for ; Thu, 27 Jan 2022 15:25:40 +0000 (UTC) Received: by mail-qv1-f52.google.com with SMTP id k9so3084315qvv.9 for ; Thu, 27 Jan 2022 07:25:40 -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=01nTXFi9SgWDzYJcR7cfPlKiH9k3njJK1816W1y8hvM=; b=jwPzjxzIwGCJOWitpalJJ4hk1VnpHBFvsKc8TDDXTfrJNqEx5ZyTbwP74nc+pyfEbS Q8TL8MM8KXOImBnER2cGbVufisfizVFaDxomUOquLe9bRi6mqR00d/bny1vmyYma1ayi 08QTu+T4WBBWXkUd/BjmxTlrZ1LIbAMD3Rbhpw+l3rl3YDNO83UuTaUABlXbNQ2jcyTb JLsOgrhsebW40nQycd07NJbTy0OToeBWI89qyXMmUCgwBs6jqwSnX3U4EOBzEh04bKtc ksWA7DdHDAdmkuGbiQBsRhsO3U7mw+w0vVYlg2GHnfLjq5YauyvXlfucL1cRhfSulXVf Ee9w== 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=01nTXFi9SgWDzYJcR7cfPlKiH9k3njJK1816W1y8hvM=; b=oEvDxhXOlClJZgCP2EpmqToFIkMS0C2nOhhIoCW6jmAWVts4nQaxHtroN4nxL+WqxP 4RMgbwfm5rsbDAQKbMJH41FPNrZtQa6y6qKh/2dq4A7gVtxGdy7acZSNCJeALy/7TgaU esI6xbNsmNCc9UBACroLLjAu4uAD32p501L6CMqryf8A9rP4Q6hoscK/PxUixAYwezz3 iGmXdIrQE/KW5yiFINzwcJxggN9x5a61IBj4n9KbYPe3hqrRQrAg4PfecwxgHCNMbC9O YYlq5Q3rB7gIDymxcLXtxBNKXU/TJwR+MZ4+qcB2C2nCyW7APfAzxt8weqVsODR+DBMH defg== X-Gm-Message-State: AOAM5315QAXp9ciwfPcemcSmTCRfs1vPRX1BBvFFgYgSarnPllQa2uZp xs3iWkfN1F7UrU+Tb5XcT1x7RQ== X-Google-Smtp-Source: ABdhPJz4IMI6Co+Rr3F5Ew/nw2aicMV8fDTyHCn6Q5S8fZS74BiFgjGj6u/P0xhV31zH++3E+ARD5w== X-Received: by 2002:ad4:5942:: with SMTP id eo2mr3648579qvb.29.1643297140104; Thu, 27 Jan 2022 07:25:40 -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 k20sm1448791qtx.64.2022.01.27.07.25.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 Jan 2022 07:25:39 -0800 (PST) Received: from jgg by mlx with local (Exim 4.94) (envelope-from ) id 1nD6eY-006Y5a-Lt; Thu, 27 Jan 2022 11:25:38 -0400 Date: Thu, 27 Jan 2022 11:25:38 -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: <20220127152538.GQ8034@ziepe.ca> References: <20220125033700.69705-1-peterx@redhat.com> <20220127004206.GP8034@ziepe.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam11 X-Rspamd-Queue-Id: 09334100023 X-Stat-Signature: tqd46hdjsgy81ric98g4qorkteoih4no Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=ziepe.ca header.s=google header.b=jwPzjxzI; dmarc=none; spf=pass (imf14.hostedemail.com: domain of jgg@ziepe.ca designates 209.85.219.52 as permitted sender) smtp.mailfrom=jgg@ziepe.ca X-Rspam-User: nil X-HE-Tag: 1643297140-440375 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 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. 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 > > > Here, however, I think we need to consider this a little more carefully, > > > and attempt to actually fix up this case. It is never going to be OK > > > here, to return a **pages array that has these little landmines of > > > potentially uninitialized pointers. And so continuing on *at all* seems > > > very wrong. > > > > Indeed, it should just be like this: > > > > @@ -1182,6 +1182,10 @@ static long __get_user_pages(struct mm_struct *mm, > > * Proper page table entry exists, but no corresponding > > * struct page. > > */ > > + if (pages) { > > + page = ERR_PTR(-EFAULT); > > + goto out; > > + } > > goto next_page; > > } else if (IS_ERR(page)) { > > ret = PTR_ERR(page); > > IIUC not failing -EEXIST immediately seems to be what we want. Which is what this does, for the only case it is acceptable - a null page list. > From that POV, WARN_ON_ONCE() helps better on exposing an illegal return of > -EEXIST (as mentioned in the commit message) than the -EFAULT convertion, IMHO. Again, that is upside down, -EEXIST should not be a illegal return. It should be valid, have a defined meaning 'the vaddr exists but has no struct page' and the top loop, and only the top loop, makes the decision what to do about it. Jason