Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
MIME-Version: 1.0
References: <20230914152620.2743033-1-surenb@google.com> <20230914152620.2743033-3-surenb@google.com>
 <CAG48ez0gN_nC8NrMOeq44QmUDT27EpT0bFuNu1ReVKDBt3zy7Q@mail.gmail.com>
 <CAJuCfpGdbc70aZPu=cNgemK1EFUyvLfZU8ELSjseZtfpSF+EEg@mail.gmail.com>
 <CAG48ez212+UjQMB94vKvyV4YAEgg=jBhdzg_1b4BRe6=SO09fA@mail.gmail.com> <CAJuCfpGEPPmEW6GqCjQXB5g04PA=BwhNgLVooM+DcroQj8RjyA@mail.gmail.com>
In-Reply-To: <CAJuCfpGEPPmEW6GqCjQXB5g04PA=BwhNgLVooM+DcroQj8RjyA@mail.gmail.com>
From:   Jann Horn <jannh@google.com>
Date:   Wed, 20 Sep 2023 18:11:19 +0200
Message-ID: <CAG48ez0WQiZvBzYBHXVP8ZVFXJEfRsHxNuXvMTFL+ietgyB9yQ@mail.gmail.com>
Subject: Re: [PATCH 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 20, 2023 at 3:49=E2=80=AFAM Suren Baghdasaryan <surenb@google.c=
om> wrote:
> On Tue, Sep 19, 2023 at 4:51=E2=80=AFPM Jann Horn <jannh@google.com> wrot=
e:
> > On Wed, Sep 20, 2023 at 1:08=E2=80=AFAM Suren Baghdasaryan <surenb@goog=
le.com> wrote:
> > > On Thu, Sep 14, 2023 at 7:28=E2=80=AFPM Jann Horn <jannh@google.com> =
wrote:
> > > > On Thu, Sep 14, 2023 at 5:26=E2=80=AFPM Suren Baghdasaryan <surenb@=
google.com> wrote:
> > > > > From: Andrea Arcangeli <aarcange@redhat.com>
> > > > >
> > > > > This implements the uABI of UFFDIO_REMAP.
> > > > >
> > > > > Notably one mode bitflag is also forwarded (and in turn known) by=
 the
> > > > > lowlevel remap_pages method.
> > [...]
> > > > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > [...]
> > > > > +int remap_pages_huge_pmd(struct mm_struct *dst_mm,
> > > > > +                        struct mm_struct *src_mm,
> > > > > +                        pmd_t *dst_pmd, pmd_t *src_pmd,
> > > > > +                        pmd_t dst_pmdval,
> > > > > +                        struct vm_area_struct *dst_vma,
> > > > > +                        struct vm_area_struct *src_vma,
> > > > > +                        unsigned long dst_addr,
> > > > > +                        unsigned long src_addr)
> > > > > +{
> > > > > +       pmd_t _dst_pmd, src_pmdval;
> > > > > +       struct page *src_page;
> > > > > +       struct anon_vma *src_anon_vma, *dst_anon_vma;
> > > > > +       spinlock_t *src_ptl, *dst_ptl;
> > > > > +       pgtable_t pgtable;
> > > > > +       struct mmu_notifier_range range;
> > > > > +
> > > > > +       src_pmdval =3D *src_pmd;
> > > > > +       src_ptl =3D pmd_lockptr(src_mm, src_pmd);
> > > > > +
> > > > > +       BUG_ON(!pmd_trans_huge(src_pmdval));
> > > > > +       BUG_ON(!pmd_none(dst_pmdval));
> > > >
> > > > Why can we assert that pmd_none(dst_pmdval) is true here? Can we no=
t
> > > > have concurrent faults (or userfaultfd operations) populating that
> > > > PMD?
> > >
> > > IIUC dst_pmdval is a copy of the value from dst_pmd, so that local
> > > copy should not change even if some concurrent operation changes
> > > dst_pmd. We can assert that it's pmd_none because we checked for that
> > > before calling remap_pages_huge_pmd. Later on we check if dst_pmd
> > > changed from under us (see pmd_same(*dst_pmd, dst_pmdval) check) and
> > > retry if that happened.
> >
> > Oh, right, I don't know what I was thinking when I typed that.
> >
> > But now I wonder about the check directly above that: What does this
> > code do for swap PMDs? It looks like that might splat on the
> > BUG_ON(!pmd_trans_huge(src_pmdval)). All we've checked on the path to
> > here is that the virtual memory area is aligned, that the destination
> > PMD is empty, and that pmd_trans_huge_lock() succeeded; but
> > pmd_trans_huge_lock() explicitly permits swap PMDs (which is the
> > swapped-out version of transhuge PMDs):
> >
> > static inline spinlock_t *pmd_trans_huge_lock(pmd_t *pmd,
> >                 struct vm_area_struct *vma)
> > {
> >         if (is_swap_pmd(*pmd) || pmd_trans_huge(*pmd) || pmd_devmap(*pm=
d))
> >                 return __pmd_trans_huge_lock(pmd, vma);
> >         else
> >                 return NULL;
> > }
>
> Yeah... Ok, I think I'm missing a check for  pmd_trans_huge(*src_pmd)
> after we lock it with pmd_trans_huge_lock(src_pmd, src_vma). And we
> can remove the above BUG_ON(). Would that address your concern?

Sounds good. It'll end up splitting huge swap entries but I guess the
extra code for moving huge swap entries might not be worth it.