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 B7734C433EF for ; Fri, 28 Jan 2022 01:36:30 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2E64B6B00A1; Thu, 27 Jan 2022 20:36:30 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 295BC6B00A3; Thu, 27 Jan 2022 20:36:30 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1376B6B00A4; Thu, 27 Jan 2022 20:36:30 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0174.hostedemail.com [216.40.44.174]) by kanga.kvack.org (Postfix) with ESMTP id 00DF56B00A1 for ; Thu, 27 Jan 2022 20:36:29 -0500 (EST) Received: from smtpin08.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id A96158249980 for ; Fri, 28 Jan 2022 01:36:29 +0000 (UTC) X-FDA: 79077980898.08.5FFC1EC Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf07.hostedemail.com (Postfix) with ESMTP id C6B8640002 for ; Fri, 28 Jan 2022 01:36:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1643333787; 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=JcYKtS421ws0gC+d9NqwTyqIBwVVHbKtxBormtVmgOQ=; b=g/fIrFTifIvsgE1kpvuC1GfIFrbU3qdx0iEoF5NjVKPIziP2i6A025LNtFYlTG+nNhbJzs X7mz02lCMr544ajwd/sAIW/92PRG8JtmDy2K1U6GGnuIsnSBXD7UitqUQ1nJTdd5gcoxUL 4dGypxItpBOA93IpYhonwiUYg8yA+Wo= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-447-5faxgSYtOMOx05dyWlFmpA-1; Thu, 27 Jan 2022 20:36:24 -0500 X-MC-Unique: 5faxgSYtOMOx05dyWlFmpA-1 Received: by mail-wr1-f69.google.com with SMTP id m17-20020adfa3d1000000b001dd66c10c0cso1619164wrb.19 for ; Thu, 27 Jan 2022 17:36:23 -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=JcYKtS421ws0gC+d9NqwTyqIBwVVHbKtxBormtVmgOQ=; b=JM+1obIFFVMhc5pHGAhOsBqO9AxzWxqIo1GMiaBdWyOlkRD6VOeYWIw1AMyPnx/6IC Wfq8KE+wCbH46r3k6scPgF5npH6sPesstek14JHRmkcJNVBW0OeNxVODLy7JUa1FlMgM ehqdeWt+RUUIbAiZmaMJUPbc1rZ6XUGOsI91pRtVSIkcKEniARtNgbbQlu8z6npsfak8 2LlIWbOUp7yWNn1V5WqULVA1JrFvV1S+tRqy5XPTqFvcmJxUVBxtG8SQsQIiVJloh2IB Sd6EvdWMJUc2pTrNJudKKc+5U2qgQCdlLzLFxx38toinxnl5Gk/7q/yPNim4eJi9WI9k 9CiA== X-Gm-Message-State: AOAM532dO7UdMvf4eOLELaKftSeOTscpw/F3Nl67TIscUyDLR48WyqVt OL7IGzR1okFe66Do8PvoALMCukqvKLiZPtBWj/pouYhNhAaYHzcxLiHM823DDShHJgxCSNeeL5F cJF9J62D/THY= X-Received: by 2002:a5d:64e2:: with SMTP id g2mr5230153wri.700.1643333782836; Thu, 27 Jan 2022 17:36:22 -0800 (PST) X-Google-Smtp-Source: ABdhPJw+hI5kCTICGWtlKGl7cRSb69fbDbnUyNCyIAHUqHS4kKgcVWKvvMjpxzsrRpePTXKPGiDEcA== X-Received: by 2002:a5d:64e2:: with SMTP id g2mr5230140wri.700.1643333782535; Thu, 27 Jan 2022 17:36:22 -0800 (PST) Received: from xz-m1.local ([64.64.123.9]) by smtp.gmail.com with ESMTPSA id m6sm3903137wrw.54.2022.01.27.17.36.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 Jan 2022 17:36:22 -0800 (PST) Date: Fri, 28 Jan 2022 09:36:14 +0800 From: Peter Xu To: Jason Gunthorpe 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> <20220127152538.GQ8034@ziepe.ca> MIME-Version: 1.0 In-Reply-To: <20220127152538.GQ8034@ziepe.ca> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: C6B8640002 X-Stat-Signature: kectqtyufcoby9cs686dwrssotopxycy X-Rspam-User: nil Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b="g/fIrFTi"; spf=none (imf07.hostedemail.com: domain of peterx@redhat.com has no SPF policy when checking 170.10.129.124) smtp.mailfrom=peterx@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-HE-Tag: 1643333787-294979 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 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? > > 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))); > > > > > 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. 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. Especially with above VM_BUG_ON() it means if we'll go into the "if (pages)" we should have already triggered the VM_BUG_ON() condition when entering the function. Thanks, -- Peter Xu