Re: [PATCH v2 4/5] userfaultfd: mcopy_atomic: return -ENOENT when no compatible VMA found

[Mike Rapoport <rppt@linux.vnet.ibm.com>]

Hello Andrea,

On Thu, Feb 02, 2017 at 07:02:47PM +0100, Andrea Arcangeli wrote:
> On Fri, Jan 27, 2017 at 08:44:32PM +0200, Mike Rapoport wrote:
> > -		err = -EINVAL;
> > +		err = -ENOENT;
> >  		dst_vma = find_vma(dst_mm, dst_start);
> >  		if (!dst_vma || !is_vm_hugetlb_page(dst_vma))
> >  			goto out_unlock;
> > +		/*
> > +		 * Only allow __mcopy_atomic_hugetlb on userfaultfd
> > +		 * registered ranges.
> > +		 */
> > +		if (!dst_vma->vm_userfaultfd_ctx.ctx)
> > +			goto out_unlock;
> >  
> > +		err = -EINVAL;
> >  		if (vma_hpagesize != vma_kernel_pagesize(dst_vma))
> >  			goto out_unlock;
> 
> That's correct, if a new vma emerges with a different page size it
> cannot have a not null dst_vma->vm_userfaultfd_ctx.ctx in the non
> cooperative case.
> 
> > @@ -219,12 +226,6 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
> >  		goto out_unlock;
> >  
> >  	/*
> > -	 * Only allow __mcopy_atomic_hugetlb on userfaultfd registered ranges.
> > -	 */
> > -	if (!dst_vma->vm_userfaultfd_ctx.ctx)
> > -		goto out_unlock;
> > -
> > -	/*
> 
> but this is buggy and it shouldn't be removed, we need this check also
> if dst_vma was found not NULL.

The check for not-NULL uffd context is done in __mcopy_atomic, between
find_vma and call to __mcopy_atomic_hugetlb. Sp, at this point we verified
that dst_vma->vm_userfaultfd_ctx.ctx is not NULL either in the caller, or
for the 'retry' case in the hunk above.

> >  	 * Ensure the dst_vma has a anon_vma.
> >  	 */
> >  	err = -ENOMEM;
> > @@ -368,10 +369,23 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,
> >  	 * Make sure the vma is not shared, that the dst range is
> >  	 * both valid and fully within a single existing vma.
> >  	 */
> > -	err = -EINVAL;
> > +	err = -ENOENT;
> >  	dst_vma = find_vma(dst_mm, dst_start);
> >  	if (!dst_vma)
> >  		goto out_unlock;
> > +	/*
> > +	 * Be strict and only allow __mcopy_atomic on userfaultfd
> > +	 * registered ranges to prevent userland errors going
> > +	 * unnoticed. As far as the VM consistency is concerned, it
> > +	 * would be perfectly safe to remove this check, but there's
> > +	 * no useful usage for __mcopy_atomic ouside of userfaultfd
> > +	 * registered ranges. This is after all why these are ioctls
> > +	 * belonging to the userfaultfd and not syscalls.
> > +	 */
> > +	if (!dst_vma->vm_userfaultfd_ctx.ctx)
> > +		goto out_unlock;
> > +
> > +	err = -EINVAL;
> >  	if (!vma_is_shmem(dst_vma) && dst_vma->vm_flags & VM_SHARED)
> >  		goto out_unlock;
> >  	if (dst_start < dst_vma->vm_start ||
> 
> This isn't enough, the -ENOENT should be returned also if the address
> doesn't isn't in the range of the found vma, instead of -EINVAL. "vma"
> may be a completely different vma just it happen to be way above the
> fault address, and the vma previously covering the "addr" (which was
> below the found "vma") was already munmapped, so you'd be returning
> -EINVAL after munmap still unless the -EINVAL is moved down below.

Will fix, thanks.
 
> The check on !vma_is_shmem(dst_vma) && dst_vma->vm_flags & VM_SHARED
> instead can be shifted down below after setting err to -EINVAL as then
> we know the vma is really the one we were looking for but it's of a
> type we can't handle.

--
Sincerely yours,
Mike.