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

[Axel Rasmussen]

On Tue, Feb 23, 2021 at 7:38 AM Peter Xu <peterx@xxxxxxxxxx> 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.

>
> Otherwise the patch looks good to me.

Shall I add a R-B? :)

Thanks for taking the time to review Peter!

>
> Thanks,
>
> --
> Peter Xu
>