Re: [PATCH v7 4/6] userfaultfd: add UFFDIO_CONTINUE ioctl

From: Peter Xu <peterx@redhat.com>
To: Axel Rasmussen <axelrasmussen@google.com>
Cc: "Alexander Viro" <viro@zeniv.linux.org.uk>,
	"Alexey Dobriyan" <adobriyan@gmail.com>,
	"Andrea Arcangeli" <aarcange@redhat.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Anshuman Khandual" <anshuman.khandual@arm.com>,
	"Catalin Marinas" <catalin.marinas@arm.com>,
	"Chinwen Chang" <chinwen.chang@mediatek.com>,
	"Huang Ying" <ying.huang@intel.com>,
	"Ingo Molnar" <mingo@redhat.com>, "Jann Horn" <jannh@google.com>,
	"Jerome Glisse" <jglisse@redhat.com>,
	"Lokesh Gidra" <lokeshgidra@google.com>,
	"Matthew Wilcox (Oracle)" <willy@infradead.org>,
	"Michael Ellerman" <mpe@ellerman.id.au>,
	"Michal Koutný" <mkoutny@suse.com>,
	"Michel Lespinasse" <walken@google.com>,
	"Mike Kravetz" <mike.kravetz@oracle.com>,
	"Mike Rapoport" <rppt@linux.vnet.ibm.com>,
	"Nicholas Piggin" <npiggin@gmail.com>, "Shaohua Li" <shli@fb.com>,
	"Shawn Anastasio" <shawn@anastas.io>,
	"Steven Rostedt" <rostedt@goodmis.org>,
	"Steven Price" <steven.price@arm.com>,
	"Vlastimil Babka" <vbabka@suse.cz>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-fsdevel@vger.kernel.org, "Linux MM" <linux-mm@kvack.org>,
	"Adam Ruprecht" <ruprecht@google.com>,
	"Cannon Matthews" <cannonmatthews@google.com>,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
	"David Rientjes" <rientjes@google.com>,
	"Mina Almasry" <almasrymina@google.com>,
	"Oliver Upton" <oupton@google.com>
Subject: Re: [PATCH v7 4/6] userfaultfd: add UFFDIO_CONTINUE ioctl
Date: Tue, 23 Feb 2021 13:28:56 -0500	[thread overview]
Message-ID: <20210223182856.GA176114@xz-x1> (raw)
In-Reply-To: <CAJHvVcg_hV0diLxyB2=JbLbJkXWTW+zsPsdzBTJW_WcG-vbvbA@mail.gmail.com>

On Tue, Feb 23, 2021 at 10:05:49AM -0800, Axel Rasmussen wrote:
> On Tue, Feb 23, 2021 at 7:38 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Thu, Feb 18, 2021 at 04:48:22PM -0800, Axel Rasmussen wrote:
> > > @@ -4645,8 +4646,18 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> > >       spinlock_t *ptl;
> > >       int ret;
> > >       struct page *page;
> > > +     int writable;
> > >
> > > -     if (!*pagep) {
> > > +     mapping = dst_vma->vm_file->f_mapping;
> > > +     idx = vma_hugecache_offset(h, dst_vma, dst_addr);
> > > +
> > > +     if (is_continue) {
> > > +             ret = -EFAULT;
> > > +             page = find_lock_page(mapping, idx);
> > > +             *pagep = NULL;
> >
> > Why set *pagep to NULL?  Shouldn't it be NULL always?.. If that's the case,
> > maybe WARN_ON_ONCE(*pagep) suite more.
> 
> Right, the caller should be passing in NULL in the
> MCOPY_ATOMIC_CONTINUE case. Looking more closely at the caller
> (__mcopy_atomic_hugetlb), it already has a BUG_ON(page), so at best
> this assignment is redundant, and at worst it might actually cover up
> a real bug (say the caller mistakenly *did* pass in some page, we'd
> set it to NULL and the BUG_ON wouldn't trigger).
> 
> So, I'll just remove this - I don't think an additional WARN_ON_ONCE
> is needed given the existing BUG_ON.

It's still okay to have the WARN_ON_ONCE; it gives a direct hint that *pagep
should never be set for uffdio_continue.  No strong opinion.

> 
> >
> > Otherwise the patch looks good to me.
> 
> Shall I add a R-B? :)

Yes, as long as "*pagep = NULL" dropped, please feel free to. :)

Thanks,

-- 
Peter Xu