Messages in this thread | | | From | Axel Rasmussen <> | Date | Tue, 23 Feb 2021 10:05:49 -0800 | Subject | Re: [PATCH v7 4/6] userfaultfd: add UFFDIO_CONTINUE ioctl |
| |
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.
> > Otherwise the patch looks good to me.
Shall I add a R-B? :)
Thanks for taking the time to review Peter!
> > Thanks, > > -- > Peter Xu >
| |