| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aYhe55zEqvuyng8z@kernel.org>
Date: Sun, 8 Feb 2026 12:01:11 +0200
From: Mike Rapoport <rppt@...nel.org>
To: Peter Xu <peterx@...hat.com>
Cc: linux-mm@...ck.org, Andrea Arcangeli <aarcange@...hat.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Axel Rasmussen <axelrasmussen@...gle.com>,
Baolin Wang <baolin.wang@...ux.alibaba.com>,
David Hildenbrand <david@...hat.com>,
Hugh Dickins <hughd@...gle.com>,
James Houghton <jthoughton@...gle.com>,
"Liam R. Howlett" <Liam.Howlett@...cle.com>,
Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
Michal Hocko <mhocko@...e.com>, Muchun Song <muchun.song@...ux.dev>,
Nikita Kalyazin <kalyazin@...zon.com>,
Oscar Salvador <osalvador@...e.de>,
Paolo Bonzini <pbonzini@...hat.com>,
Sean Christopherson <seanjc@...gle.com>,
Shuah Khan <shuah@...nel.org>,
Suren Baghdasaryan <surenb@...gle.com>,
Vlastimil Babka <vbabka@...e.cz>, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH RFC 05/17] userfaultfd: retry copying with locks dropped
in mfill_atomic_pte_copy()
On Mon, Feb 02, 2026 at 04:23:16PM -0500, Peter Xu wrote:
> Hi, Mike,
>
> On Tue, Jan 27, 2026 at 09:29:24PM +0200, Mike Rapoport wrote:
> > From: "Mike Rapoport (Microsoft)" <rppt@...nel.org>
> >
> > Implementation of UFFDIO_COPY for anonymous memory might fail to copy
> > data data from userspace buffer when the destination VMA is locked
> > (either with mm_lock or with per-VMA lock).
> >
> > In that case, mfill_atomic() releases the locks, retries copying the
> > data with locks dropped and then re-locks the destination VMA and
> > re-establishes PMD.
> >
> > Since this retry-reget dance is only relevant for UFFDIO_COPY and it
> > never happens for other UFFDIO_ operations, make it a part of
> > mfill_atomic_pte_copy() that actually implements UFFDIO_COPY for
> > anonymous memory.
> >
> > shmem implementation will be updated later and the loop in
> > mfill_atomic() will be adjusted afterwards.
>
> Thanks for the refactoring. Looks good to me in general, only some
> nitpicks inline.
>
> >
> > Signed-off-by: Mike Rapoport (Microsoft) <rppt@...nel.org>
> > ---
> > mm/userfaultfd.c | 70 +++++++++++++++++++++++++++++++-----------------
> > 1 file changed, 46 insertions(+), 24 deletions(-)
> >
> > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > index 45d8f04aaf4f..01a2b898fa40 100644
> > --- a/mm/userfaultfd.c
> > +++ b/mm/userfaultfd.c
> > @@ -404,35 +404,57 @@ static int mfill_copy_folio_locked(struct folio *folio, unsigned long src_addr)
> > return ret;
> > }
> >
> > +static int mfill_copy_folio_retry(struct mfill_state *state, struct folio *folio)
> > +{
> > + unsigned long src_addr = state->src_addr;
> > + void *kaddr;
> > + int err;
> > +
> > + /* retry copying with mm_lock dropped */
> > + mfill_put_vma(state);
> > +
> > + kaddr = kmap_local_folio(folio, 0);
> > + err = copy_from_user(kaddr, (const void __user *) src_addr, PAGE_SIZE);
> > + kunmap_local(kaddr);
> > + if (unlikely(err))
> > + return -EFAULT;
> > +
> > + flush_dcache_folio(folio);
> > +
> > + /* reget VMA and PMD, they could change underneath us */
> > + err = mfill_get_vma(state);
> > + if (err)
> > + return err;
> > +
> > + err = mfill_get_pmd(state);
> > + if (err)
> > + return err;
> > +
> > + return 0;
> > +}
> > +
> > static int mfill_atomic_pte_copy(struct mfill_state *state)
> > {
> > - struct vm_area_struct *dst_vma = state->vma;
> > unsigned long dst_addr = state->dst_addr;
> > unsigned long src_addr = state->src_addr;
> > uffd_flags_t flags = state->flags;
> > - pmd_t *dst_pmd = state->pmd;
> > struct folio *folio;
> > int ret;
> >
> > - if (!state->folio) {
> > - ret = -ENOMEM;
> > - folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, dst_vma,
> > - dst_addr);
> > - if (!folio)
> > - goto out;
> > + folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, state->vma, dst_addr);
> > + if (!folio)
> > + return -ENOMEM;
> >
> > - ret = mfill_copy_folio_locked(folio, src_addr);
> > + ret = -ENOMEM;
> > + if (mem_cgroup_charge(folio, state->vma->vm_mm, GFP_KERNEL))
> > + goto out_release;
> >
> > + ret = mfill_copy_folio_locked(folio, src_addr);
> > + if (unlikely(ret)) {
> > /* fallback to copy_from_user outside mmap_lock */
> > - if (unlikely(ret)) {
> > - ret = -ENOENT;
> > - state->folio = folio;
> > - /* don't free the page */
> > - goto out;
> > - }
> > - } else {
> > - folio = state->folio;
> > - state->folio = NULL;
> > + ret = mfill_copy_folio_retry(state, folio);
>
> Yes, I agree this should work and should avoid the previous ENOENT
> processing that might be hard to follow. It'll move the complexity into
> mfill_state though (e.g., now it's unknown on the vma lock state after this
> function returns..), but I guess it's fine.
When this function returns success VMA is locked. If the function fails it
does not matter if the VMA is locked.
I'll add some comments.
> > + if (ret)
> > + goto out_release;
> > }
> >
> > /*
> > @@ -442,17 +464,16 @@ static int mfill_atomic_pte_copy(struct mfill_state *state)
> > */
> > __folio_mark_uptodate(folio);
>
> Since success path should make sure vma lock held when reaching here, but
> now with mfill_copy_folio_retry()'s presence it's not as clear as before,
> maybe we add an assertion for that here before installing ptes? No strong
> feelings.
I'll add comments.
> >
> > - ret = -ENOMEM;
> > - if (mem_cgroup_charge(folio, dst_vma->vm_mm, GFP_KERNEL))
> > - goto out_release;
> > -
> > - ret = mfill_atomic_install_pte(dst_pmd, dst_vma, dst_addr,
> > + ret = mfill_atomic_install_pte(state->pmd, state->vma, dst_addr,
> > &folio->page, true, flags);
> > if (ret)
> > goto out_release;
> > out:
> > return ret;
> > out_release:
> > + /* Don't return -ENOENT so that our caller won't retry */
> > + if (ret == -ENOENT)
> > + ret = -EFAULT;
>
> I recall the code removed is the only path that can return ENOENT? Then
> maybe this line isn't needed?
I didn't want to audit all potential errors and this is a temporal safety
measure to avoid breaking biscection. This is anyway removed in the
following patches.
> > folio_put(folio);
> > goto out;
> > }
> > @@ -907,7 +928,8 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> > break;
> > }
> >
> > - mfill_put_vma(&state);
> > + if (state.vma)
>
> I wonder if we should move this check into mfill_put_vma() directly, it
> might be overlooked if we'll put_vma in other paths otherwise.
Yeah, I'll check this.
--
Sincerely yours,
Mike.
Powered by blists - more mailing lists