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 7D0D7C433F5 for ; Thu, 27 Jan 2022 09:20:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 077AA6B0071; Thu, 27 Jan 2022 04:20:10 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 0279E6B0072; Thu, 27 Jan 2022 04:20:09 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E30E06B0073; Thu, 27 Jan 2022 04:20:09 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0207.hostedemail.com [216.40.44.207]) by kanga.kvack.org (Postfix) with ESMTP id D035F6B0071 for ; Thu, 27 Jan 2022 04:20:09 -0500 (EST) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 71EE5181CAC42 for ; Thu, 27 Jan 2022 09:20:09 +0000 (UTC) X-FDA: 79075520538.29.7683026 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf26.hostedemail.com (Postfix) with ESMTP id D21C4140006 for ; Thu, 27 Jan 2022 09:20:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1643275208; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=D90RcPe2iP0PJhb56Gwnt9wR3ymnukzeMLhlgOBOsAE=; b=TA0O6g9OyKEpA0f5MjuR+YQ40JUE4W72x+dOoFPlEQoxfaD4Wyu5Ko1Vb7TyyrYxe1uDjZ iM5ymF5oQAcwfd3IVF6lrPm7c2iqR0HOp+gVyksRrFCpzE04MSpLM9R7TbqvkC8XQb6nm5 b1JRXy4wdXrMND/jP1g9qUoJngAOpKQ= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-378-axTybL7NPLemBwh-3zKl1w-1; Thu, 27 Jan 2022 04:20:04 -0500 X-MC-Unique: axTybL7NPLemBwh-3zKl1w-1 Received: by mail-wm1-f70.google.com with SMTP id l20-20020a05600c1d1400b0035153bf34c3so1152398wms.2 for ; Thu, 27 Jan 2022 01:20:04 -0800 (PST) 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=D90RcPe2iP0PJhb56Gwnt9wR3ymnukzeMLhlgOBOsAE=; b=o+asw9obcDK3imNMlZHZ1j1f/QwkTq+fCOoc0tDqpYgZePjlLVn0SpdxTNSUSYRfqR zuWgB3cWGKlSlS/yL7oIVHT6szDbnWy+JbMiFX8lQ0YaaPldmC8lCfHAfa/BHTmebg78 UKbXHBdZuvQl7adwfeFdvwLjlwsb1OC8Yl3OaHXlhvH4Hq5eq18ESZdAEGNKvX2Oqt3w m13is5JrW8tkzml5XrOXXWPKRosFOvH4BXlLcfy1YN+VROpHErk4461xAYyYl952q1f1 aWGSRA8Mo03eoZUbX8aqM+NXkqrc4oCsmChw6jLdWsXNpNXK7yJPoBY6sW6Jdip5eGs5 oqxw== X-Gm-Message-State: AOAM5327Qzl1lSIWWYsMSIynpjZvR9GxrC1024dMmzdordud+eybLCvK syS5KN4+PYTlhHcgXanKWzRqRjZx8BpD4iSXcWGQ6HYDdevQ3Jr0ixPWwm9SKKiKp+J9w6OdSds 0lx8u5iY3wMk= X-Received: by 2002:adf:ed42:: with SMTP id u2mr2173937wro.519.1643275203622; Thu, 27 Jan 2022 01:20:03 -0800 (PST) X-Google-Smtp-Source: ABdhPJzImXo3+kGFtUKSYJ9XLCMH17wHTiavXdqjp17eDf6LfthOyYuCcsF6VJeC/6/KbiQqQPURDA== X-Received: by 2002:adf:ed42:: with SMTP id u2mr2173911wro.519.1643275203266; Thu, 27 Jan 2022 01:20:03 -0800 (PST) Received: from xz-m1.local ([85.203.46.170]) by smtp.gmail.com with ESMTPSA id t1sm1920673wre.45.2022.01.27.01.19.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 Jan 2022 01:20:02 -0800 (PST) Date: Thu, 27 Jan 2022 17:19:56 +0800 From: Peter Xu To: Jason Gunthorpe , John Hubbard 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: References: <20220125033700.69705-1-peterx@redhat.com> <20220127004206.GP8034@ziepe.ca> MIME-Version: 1.0 In-Reply-To: <20220127004206.GP8034@ziepe.ca> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-Rspam-User: nil X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: D21C4140006 X-Stat-Signature: upeo5zns3m5uy1qfrdbwyfm5s59py6jz Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=TA0O6g9O; spf=none (imf26.hostedemail.com: domain of peterx@redhat.com has no SPF policy when checking 170.10.133.124) smtp.mailfrom=peterx@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-HE-Tag: 1643275208-548934 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: Hi, Jason, John, On Wed, Jan 26, 2022 at 08:42:06PM -0400, Jason Gunthorpe wrote: > 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? Not quite familiar with the dax effort, but just to note again that this patch fixes an immediate breakage, hence IMHO we should still merge it first (not mention it's a one-liner..). > > 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? Because in current code GUP handles -EEXIST and -EFAULT differently? 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. > > 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); IIUC not failing -EEXIST immediately seems to be what we want. We've discussed the only (unless I overlooked something...) two sites that can return -EEXIST in follow_page_mask() context; if it is triggered somewhere else unexpectedly it is a programming error and needs fixing, imho. Hence the gup code shouldn't rely on the -EEXIST -> -EFAULT transition to work at all in any existing case. >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. Thanks, -- Peter Xu