Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
MIME-Version: 1.0
References: <20230923013148.1390521-1-surenb@google.com> <20230923013148.1390521-3-surenb@google.com>
 <CAG48ez1N2kryy08eo0dcJ5a9O-3xMT8aOrgrcD+CqBN=cBfdDw@mail.gmail.com>
 <CAJuCfpGb5Amo9Sk0yyruJt9NKaYe9-y+5jmU442NSf3+VT5-dA@mail.gmail.com>
 <CAG48ez2WNOMwPo4OMVUHbS4mirwbqHUY5qUaaZ9DTkXdkzrjiQ@mail.gmail.com>
 <CAJuCfpGcsBE2XqPJSVo1gdE_O96gzS5=ET=u0uSBSX3Lj56CtA@mail.gmail.com> <CAJuCfpHY5zhkS0OPxOK-twb6pDJg6OpXZnPquw_9wBmbjFiF9Q@mail.gmail.com>
In-Reply-To: <CAJuCfpHY5zhkS0OPxOK-twb6pDJg6OpXZnPquw_9wBmbjFiF9Q@mail.gmail.com>
From:   Jann Horn <jannh@google.com>
Date:   Thu, 28 Sep 2023 00:48:37 +0200
Message-ID: <CAG48ez3w7kbetfhEd7trLhOKJYPw4jSVBeOC+psZZR84d-hJaw@mail.gmail.com>
Subject: Re: [PATCH v2 2/3] userfaultfd: UFFDIO_REMAP uABI
To:     Suren Baghdasaryan <surenb@google.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, david@redhat.com,
        hughd@google.com, mhocko@suse.com, axelrasmussen@google.com,
        rppt@kernel.org, willy@infradead.org, Liam.Howlett@oracle.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 Wed, Sep 27, 2023 at 11:08=E2=80=AFPM Suren Baghdasaryan <surenb@google.=
com> wrote:
> On Wed, Sep 27, 2023 at 1:42=E2=80=AFPM Suren Baghdasaryan <surenb@google=
.com> wrote:
> >
> > On Wed, Sep 27, 2023 at 1:04=E2=80=AFPM Jann Horn <jannh@google.com> wr=
ote:
> > >
> > > On Wed, Sep 27, 2023 at 8:08=E2=80=AFPM Suren Baghdasaryan <surenb@go=
ogle.com> wrote:
> > > > On Wed, Sep 27, 2023 at 5:47=E2=80=AFAM Jann Horn <jannh@google.com=
> wrote:
> > > > > On Sat, Sep 23, 2023 at 3:31=E2=80=AFAM Suren Baghdasaryan <suren=
b@google.com> wrote:
> > > > > > +               dst_pmdval =3D pmdp_get_lockless(dst_pmd);
> > > > > > +               /*
> > > > > > +                * If the dst_pmd is mapped as THP don't overri=
de it and just
> > > > > > +                * be strict. If dst_pmd changes into TPH after=
 this check, the
> > > > > > +                * remap_pages_huge_pmd() will detect the chang=
e and retry
> > > > > > +                * while remap_pages_pte() will detect the chan=
ge and fail.
> > > > > > +                */
> > > > > > +               if (unlikely(pmd_trans_huge(dst_pmdval))) {
> > > > > > +                       err =3D -EEXIST;
> > > > > > +                       break;
> > > > > > +               }
> > > > > > +
> > > > > > +               ptl =3D pmd_trans_huge_lock(src_pmd, src_vma);
> > > > > > +               if (ptl && !pmd_trans_huge(*src_pmd)) {
> > > > > > +                       spin_unlock(ptl);
> > > > > > +                       ptl =3D NULL;
> > > > > > +               }
> > > > >
> > > > > This still looks wrong - we do still have to split_huge_pmd()
> > > > > somewhere so that remap_pages_pte() works.
> > > >
> > > > Hmm, I guess this extra check is not even needed...
> > >
> > > Hm, and instead we'd bail at the pte_offset_map_nolock() in
> > > remap_pages_pte()? I guess that's unusual but works...
> >
> > Yes, that's what I was thinking but I agree, that seems fragile. Maybe
> > just bail out early if (ptl && !pmd_trans_huge())?
>
> No, actually we can still handle is_swap_pmd() case by splitting it
> and remapping the individual ptes. So, I can bail out only in case of
> pmd_devmap().

FWIW I only learned today that "real" swap PMDs don't actually exist -
only migration entries, which are encoded as swap PMDs, exist. You can
see that when you look through the cases that something like
__split_huge_pmd() or zap_pmd_range() actually handles.

So I think if you wanted to handle all the PMD types properly here
without splitting, you could do that without _too_ much extra code.
But idk if it's worth it.