Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <20231009064230.2952396-1-surenb@google.com> <20231009064230.2952396-3-surenb@google.com>
 <721366d0-7909-45c9-ae49-f652c8369b9d@redhat.com>
In-Reply-To: <721366d0-7909-45c9-ae49-f652c8369b9d@redhat.com>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Mon, 23 Oct 2023 11:56:50 -0700
Message-ID: <CAJuCfpErrAqZuiiU5uthVU87Sa=yztRRqnTszezFCMQgBEawCg@mail.gmail.com>
Subject: Re: [PATCH v3 2/3] userfaultfd: UFFDIO_MOVE uABI
To:     David Hildenbrand <david@redhat.com>
Cc:     akpm@linux-foundation.org, viro@zeniv.linux.org.uk,
        brauner@kernel.org, shuah@kernel.org, aarcange@redhat.com,
        lokeshgidra@google.com, peterx@redhat.com, hughd@google.com,
        mhocko@suse.com, axelrasmussen@google.com, rppt@kernel.org,
        willy@infradead.org, Liam.Howlett@oracle.com, jannh@google.com,
        zhangpeng362@huawei.com, bgeffon@google.com,
        kaleshsingh@google.com, ngeoffray@google.com, jdduke@google.com,
        linux-mm@kvack.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        kernel-team@android.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Oct 23, 2023 at 5:29=E2=80=AFAM David Hildenbrand <david@redhat.com=
> wrote:
>
> Focusing on validate_remap_areas():
>
> > +
> > +static int validate_remap_areas(struct vm_area_struct *src_vma,
> > +                             struct vm_area_struct *dst_vma)
> > +{
> > +     /* Only allow remapping if both have the same access and protecti=
on */
> > +     if ((src_vma->vm_flags & VM_ACCESS_FLAGS) !=3D (dst_vma->vm_flags=
 & VM_ACCESS_FLAGS) ||
> > +         pgprot_val(src_vma->vm_page_prot) !=3D pgprot_val(dst_vma->vm=
_page_prot))
> > +             return -EINVAL;
>
> Makes sense. I do wonder about pkey and friends and if we even have to
> so anything special.

I don't see anything special done for mremap. Do you have something in mind=
?

>
> > +
> > +     /* Only allow remapping if both are mlocked or both aren't */
> > +     if ((src_vma->vm_flags & VM_LOCKED) !=3D (dst_vma->vm_flags & VM_=
LOCKED))
> > +             return -EINVAL;
> > +
> > +     if (!(src_vma->vm_flags & VM_WRITE) || !(dst_vma->vm_flags & VM_W=
RITE))
> > +             return -EINVAL;
>
> Why does one of both need VM_WRITE? If one really needs it, then the
> destination (where we're moving stuff to).

As you noticed later, both should have VM_WRITE.

>
> > +
> > +     /*
> > +      * Be strict and only allow remap_pages if either the src or
> > +      * dst range is registered in the userfaultfd 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
> > +      * remap_pages ouside of userfaultfd registered ranges. This
> > +      * is after all why it is an ioctl belonging to the
> > +      * userfaultfd and not a syscall.
>
> I think the last sentence is the important bit and the comment can
> likely be reduced.

Ok, I'll look into shortening it.

>
> > +      *
> > +      * Allow both vmas to be registered in the userfaultfd, just
> > +      * in case somebody finds a way to make such a case useful.
> > +      * Normally only one of the two vmas would be registered in
> > +      * the userfaultfd.
>
> Should we just check the destination? That makes most sense to me,
> because with uffd we are resolving uffd-events. And just like
> copy/zeropage we want to resolve a page fault ("userfault") of a
> non-present page on the destination.

I think that makes sense. Not sure why the original implementation
needed the check for source too. Seems unnecessary.

>
>
> > +      */
> > +     if (!dst_vma->vm_userfaultfd_ctx.ctx &&
> > +         !src_vma->vm_userfaultfd_ctx.ctx)
> > +             return -EINVAL;
>
>
>
> > +
> > +     /*
> > +      * FIXME: only allow remapping across anonymous vmas,
> > +      * tmpfs should be added.
> > +      */
> > +     if (!vma_is_anonymous(src_vma) || !vma_is_anonymous(dst_vma))
> > +             return -EINVAL;
>
> Why a FIXME here? Just drop the comment completely or replace it with
> "We only allow to remap anonymous folios accross anonymous VMAs".

Will do. I guess Andrea had plans to cover tmpfs as well.

>
> > +
> > +     /*
> > +      * Ensure the dst_vma has a anon_vma or this page
> > +      * would get a NULL anon_vma when moved in the
> > +      * dst_vma.
> > +      */
> > +     if (unlikely(anon_vma_prepare(dst_vma)))
> > +             return -ENOMEM;
>
> Makes sense.
>
> > +
> > +     return 0;
> > +}
>
>

Thanks,
Suren.


> --
> Cheers,
>
> David / dhildenb
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an=
 email to kernel-team+unsubscribe@android.com.
>