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 AE556C433F5 for ; Thu, 27 Jan 2022 00:42:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 26F116B0071; Wed, 26 Jan 2022 19:42:10 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 21F0F6B0072; Wed, 26 Jan 2022 19:42:10 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0C04A6B0073; Wed, 26 Jan 2022 19:42:10 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0117.hostedemail.com [216.40.44.117]) by kanga.kvack.org (Postfix) with ESMTP id F257C6B0071 for ; Wed, 26 Jan 2022 19:42:09 -0500 (EST) Received: from smtpin16.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id B15DC181CA75A for ; Thu, 27 Jan 2022 00:42:09 +0000 (UTC) X-FDA: 79074215178.16.2858DFB Received: from mail-qt1-f182.google.com (mail-qt1-f182.google.com [209.85.160.182]) by imf30.hostedemail.com (Postfix) with ESMTP id 47B7E80006 for ; Thu, 27 Jan 2022 00:42:08 +0000 (UTC) Received: by mail-qt1-f182.google.com with SMTP id h25so303131qtm.1 for ; Wed, 26 Jan 2022 16:42:08 -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=bnqZcSOUBs5sY/5j7CBOSOw5Sx0eF1Hfwywx6M39Fnk=; b=mwO1LpNsfyjAmMoxGx2V/hsh5Pk5fKxYt6rJWfdiUHvdLJXyE1faV5ABM29QqZXJmn wZOkSDB47mLo9b2uQsv0TqF5wl40KzkCGV8y0pG3jOz872t4MiER/jSEyltK/r39N9mz RmHrCZJvKZCHQ8lEQqchQ8dp2P9ScqiMUsCYvvcbsGfkWjGTqcvFnh9Jg8Wlc2VDIXp5 8GwifcIsd4oKqlFJpJEiD9Rgq/GSa8UNJICOlW89CtACQNLVrHQadmNhd/ZYHD7MGUH3 ox9zQNTz8MnGal7j2HPVp9kT7H91AjYVUk26Tefjt6wcBLQUllWusU70nLQiY67pwi/0 aiDQ== 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=bnqZcSOUBs5sY/5j7CBOSOw5Sx0eF1Hfwywx6M39Fnk=; b=SF9YMGVN1WZvoREVwRThMOlpEcXj64P8eUBYOe3idMOMJcO0TIm9EYKPxpS+t0BOyO OMyI2Z4onl9TFaoyLosYo7ekfKAjAGFoEKDpSRW8siHPNz88xBQRdXS24SCMbDN3TU4a qUzQeowETSdeTKURjS/NXqsqwEUAgiwvVN9mpW0qej19iV5gIq8g5fAHJ1Ldo7jS4JyK k9hhjX5oKbRwx9O0EYCjVQxMCicPeRLbjsZv6HUoRLUSjAzLxu6fqqNwFBrbbNXrpAvF v661pBKfZizEdzDriEHEcA3kMsGX+C33WioD96xRNdL7QlEKMoOV/KcqKchlSO6EfBcy /LJA== X-Gm-Message-State: AOAM532WYM7AZocBzzRfvq+6V2bE0cCKAl4axBhw5Sbb7vIJdJxOwJG0 6mv4j0WTVwD8FKhHMbzbcdYzBA== X-Google-Smtp-Source: ABdhPJx2URkyBLEcA/QFr2E6yt/ZE6AVVxwQTugVCTs7LL4aFX7UBjXrwHKGyBHnIxIBAjH+kbwNfg== X-Received: by 2002:a05:622a:199c:: with SMTP id u28mr982324qtc.221.1643244128025; Wed, 26 Jan 2022 16:42:08 -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 t22sm535796qkp.8.2022.01.26.16.42.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 26 Jan 2022 16:42:07 -0800 (PST) Received: from jgg by mlx with local (Exim 4.94) (envelope-from ) id 1nCsrW-0065us-IN; Wed, 26 Jan 2022 20:42:06 -0400 Date: Wed, 26 Jan 2022 20:42:06 -0400 From: Jason Gunthorpe To: John Hubbard Cc: Peter Xu , 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: <20220127004206.GP8034@ziepe.ca> References: <20220125033700.69705-1-peterx@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Stat-Signature: autkr4khfu49y4wsw7q45pbifwcu4neu X-Rspam-User: nil Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=ziepe.ca header.s=google header.b=mwO1LpNs; spf=pass (imf30.hostedemail.com: domain of jgg@ziepe.ca designates 209.85.160.182 as permitted sender) smtp.mailfrom=jgg@ziepe.ca; dmarc=none X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 47B7E80006 X-HE-Tag: 1643244128-665956 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000363, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Jan 26, 2022 at 04:15:02PM -0800, John Hubbard wrote: > > We had that -EEXIST logic since commit 1027e4436b6a ("mm: make GUP handle pfn > > mapping unless FOLL_GET is requested") which seems very reasonable. It could > > be that when we reworked GUP with FOLL_PIN we could have overlooked that > > special path in commit 3faa52c03f44 ("mm/gup: track FOLL_PIN pages"), even if > > that commit rightfully touched up follow_devmap_pud() on checking FOLL_PIN when > > it needs to return an -EEXIST. It sounds like this commit was all about changing the behavior of follow_page() It feels like that is another ill-fated holdover from the effort to make pageless DAX that doesn't exist anymore. Can we safely drop it now? Regardless.. > > 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? The caller will always see the error code and refrain from trying to pin it and unwind upwards, just the same as -EFAULT. We shouldn't need to test the flags at this point at all. > > if (flags & FOLL_TOUCH) { > > @@ -1181,7 +1181,13 @@ static long __get_user_pages(struct mm_struct *mm, > > /* > > * Proper page table entry exists, but no corresponding > > * struct page. > > + * > > + * Warn if we jumped over even with a valid **pages. > > + * It shouldn't trigger in practise, but when there's > > + * buggy returns on -EEXIST we'll warn before returning > > + * an invalid page pointer in the array. > > */ > > + WARN_ON_ONCE(pages); > > 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); Jason