From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9FF43C47DB3 for ; Mon, 29 Jan 2024 20:53:02 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2D9406B0078; Mon, 29 Jan 2024 15:53:02 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 2899E6B0085; Mon, 29 Jan 2024 15:53:02 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 12ACD6B0089; Mon, 29 Jan 2024 15:53:02 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 026E86B0078 for ; Mon, 29 Jan 2024 15:53:01 -0500 (EST) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 819DE1A0B67 for ; Mon, 29 Jan 2024 20:53:01 +0000 (UTC) X-FDA: 81733548162.10.F39CBC6 Received: from mail-yw1-f176.google.com (mail-yw1-f176.google.com [209.85.128.176]) by imf25.hostedemail.com (Postfix) with ESMTP id 95305A0007 for ; Mon, 29 Jan 2024 20:52:59 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=UHloFpZI; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf25.hostedemail.com: domain of surenb@google.com designates 209.85.128.176 as permitted sender) smtp.mailfrom=surenb@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1706561579; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=I6c3s6W4387xNYwMOxxsc37wD0YgKnZOAa+81n+g6fc=; b=2CZo0CECxOoItuPhJE0mvScG1jRJZ90XLWhXd3gpyJtHtVnrhnDG7t6f/qaPTShfgggxcR buvvKm52nf1mBgWnVz9QPDMtMqoScYih0wlR4QUxGBIiyzqZK0I3j+JO1HdYkQv26J0GTF ewM2FVgzYOHiK7Fj+ez0+732+uvzqpA= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=UHloFpZI; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf25.hostedemail.com: domain of surenb@google.com designates 209.85.128.176 as permitted sender) smtp.mailfrom=surenb@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1706561579; a=rsa-sha256; cv=none; b=Enfc6d9yw4V+ZkyZj82ffxotzuQISCJ0Bvptwff4WlPVoosSRx8iVSS0+8kUkZkTu0hE7F kKCxr6sYsE+HDuXC9+DCNDkGIhmiN3vtvd4NuaAgN10yX5gQ2Z04HqApvwlS10oVg4H0gj xI7bxlmovYVKck/GV21jHBCFoz67oUw= Received: by mail-yw1-f176.google.com with SMTP id 00721157ae682-602ab446cd8so32146987b3.1 for ; Mon, 29 Jan 2024 12:52:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1706561579; x=1707166379; darn=kvack.org; h=content-transfer-encoding:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=I6c3s6W4387xNYwMOxxsc37wD0YgKnZOAa+81n+g6fc=; b=UHloFpZI3vsQZy18wLMJCcg0LuvG5zj+p/0RjUP13fcVAVqXsrgtF3vT4+zJcE5BYZ b0mGyMB0JtXIJrQjl4q93Cxf50XtksvvRiQgPENDgD/JF3wJYiYm4804NXpgkf54PsSa JYRzf5mELYodFq1tLBK/c+a2e4bGwj9bAexXsIrIlPPEwIhpRxtW/Q+T7yb6+TzgwfCr jzwqyqLtqMb58qKh2S/7beSLOJ24AvIx126aqiFa0lpdjhKR8tCZCAgqllkRFMOy/Gwp g5nqmS/XDyqm/QUDZ8epa3OtICvohOoi2kowzgXAejvYTB7IFMvnTYakW/2H0pepChnX Jyug== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706561579; x=1707166379; h=content-transfer-encoding:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=I6c3s6W4387xNYwMOxxsc37wD0YgKnZOAa+81n+g6fc=; b=F5BsPu0BlPzhGoy2FtLIqRbp4VudUW3mm9JzpUKb6Mg3G2kSnXgVvH1JKYQeZygJAE noSwfnO391K9GIwkokXswwn0KMW8/pvT2oLfMLAxk3OUVEUGkFfcjQ0IUru5krHxKyTK NWcDKRl5+cPXeU8TivehMiNqxemfuAykzAuNtDHayKEORCinAeLkBCU8rCi+rqHvxWTf OWn1uDYPmpIdsTl3tPEpy0jb7S14SpMqyGYmwRMRi5ZOfLGAVYKwQthzJJ+SzifCOEnd uc3FYOCTEfUN42qN7DNZQqL3viL0KgLz/+jQK+pp8laiUTSa7q4cLdJu10rL9kYaIZqT +NwA== X-Gm-Message-State: AOJu0Yyv9JvaED0HYJroM7CTfRQZLSezwtvRDR0C/ZKBuTHO0KZUCAA5 kkHWejuTctNmePLOtQ6pz8q9fAclO1cOlE4/EL7X8E/KihswNursWHm4zxh2dMR/s06YTrPJ57c O08V+x0qcOihcWN53IBbSo2NiLB5x7e9d006+mu4aOvJelp3dC8lw X-Google-Smtp-Source: AGHT+IFsvWEBOjwKNfU98Gkus8OdnxSxEchA0Au3vF4bBczhIurYe1orO9DpBz0Qhfjl4r/TxUKxl7wHiS6jD0NH8zk= X-Received: by 2002:a0d:db11:0:b0:603:cafe:dafc with SMTP id d17-20020a0ddb11000000b00603cafedafcmr2558695ywe.30.1706561578276; Mon, 29 Jan 2024 12:52:58 -0800 (PST) MIME-Version: 1.0 References: <20240129193512.123145-1-lokeshgidra@google.com> <20240129193512.123145-4-lokeshgidra@google.com> <20240129203626.uq5tdic4z5qua5qy@revolver> In-Reply-To: <20240129203626.uq5tdic4z5qua5qy@revolver> From: Suren Baghdasaryan Date: Mon, 29 Jan 2024 12:52:45 -0800 Message-ID: Subject: Re: [PATCH v2 3/3] userfaultfd: use per-vma locks in userfaultfd operations To: "Liam R. Howlett" , Lokesh Gidra , akpm@linux-foundation.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, selinux@vger.kernel.org, surenb@google.com, kernel-team@android.com, aarcange@redhat.com, peterx@redhat.com, david@redhat.com, axelrasmussen@google.com, bgeffon@google.com, willy@infradead.org, jannh@google.com, kaleshsingh@google.com, ngeoffray@google.com, timmurray@google.com, rppt@kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: p198qcxz7d53mt1q3g4tnhmz618epw4z X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 95305A0007 X-HE-Tag: 1706561579-991142 X-HE-Meta: U2FsdGVkX18Gj4Eat45TscqDm+U4kKftcVFu0vtXgVEInLqSZqBa/cwEqRAQ+9VdsBvfuoyCZMY8TtXfueIOxzrs/Al9yyph9yvZPEWdqbXmZLigDPf5QmGRU+KEKJddBWEC98VnrnA49feIfHY1IGEzl5niFxO1xb1o9zynaBDmT79gnN7XPqRstypQ9k9jESrUsxBjwiYZuwRousPXr06BFnx6PCH3wqaol8ZWYdaoCAHWvcOC7w4f5WT9OrYZHhfjNQmWVJbppEe0U4Jn3g/4JJqljHNT9raiQtclD8q5PZZgIwiiZKcglOHoGNKqpTkAuiEc9USJt1FmGcJletv+maa5PeTy1C4sZrvqQi75g73RnfEBSfa212ORsdxVv1jtwzGqDj/6WSW1KlFdhYfWPmQGDKogCRHPt5S51EdZdWuvlosydBaABdRBSB0XMiJo3qMBrk9uSBu5GlWV9k7fAn7BnnU9R/jPcCBZAmXqmZL2/nl37xeKGAiAXTqOTrEUvtrnrzB09JhjHQq8/zx06TTXO8YYJjuEpPu690h3SQKvFwrgrgnBsb5bI8oCdVrpuauW5B6byVXjCd1bLgbq7BQ0AWLevD0Ycqbvo2QEGk5nhMmVjjPzf6adOF8PCJP+8LiBrqCG3CmS68TJ3v5X4T1Ntp0SDz6qyRXdub49Pi7o6wsNBTuOaqroIMHSO/zxJgi9IztdYssQgVlwUNhHqRv3MATnIWoHVGgw5bGIUzxWruyBgwbTy+OiRaQh+sOUBQEoeDv5A4jt+M29DTbOpgOXVaMrq1CPfBYKK3dkxyO8qeN8cbujw8rdjXRcpqpZfGuji14iCHOzIVfKSGuFmgMhubXx5IgVWIXsIryJjoacUBmo9Kge1dQGzWXSNropBQ2GOLoNLxytklxDcpwP4J2h5xx5VD9ioYhtuMD29jnbc3PmWYGDxbtnhYv0cd7ove89mcR83a/HrG/ cVI7raXZ 82vXCRvxIfgh/63XeREVHHFjb1DMv47ZLVfwFrDPzOiYO/1/sEp8PHUBmNAsU3J7zhCNOdvfYA8wkEfQ772JFMgBfD/lDGzjn9BMLJRm3U7mPoHGPsAl4BE9cXQh5c2kJ+Yy5Vgj5QR0tjIrDcDre9JGBRFFS9iMqiptskKiUS5UrWlNP+N/LF4+aJjkgzTYaqg5kILrvZ/cJ5lbZGyMVRCre7mwVSqjkoEuRHZ39p7fXFbNVQwlbvOcU3goWNQhJ6BXutudENPgkK9TUW84AiMQHmQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Mon, Jan 29, 2024 at 12:36=E2=80=AFPM Liam R. Howlett wrote: > > * Lokesh Gidra [240129 14:35]: > > All userfaultfd operations, except write-protect, opportunistically use > > per-vma locks to lock vmas. If we fail then fall back to locking > > mmap-lock in read-mode. > > > > Write-protect operation requires mmap_lock as it iterates over multiple= vmas. > > > > Signed-off-by: Lokesh Gidra > > --- > > fs/userfaultfd.c | 13 +-- > > include/linux/userfaultfd_k.h | 5 +- > > mm/userfaultfd.c | 175 +++++++++++++++++++++++----------- > > 3 files changed, 122 insertions(+), 71 deletions(-) > > > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c > > index c00a021bcce4..60dcfafdc11a 100644 > > --- a/fs/userfaultfd.c > > +++ b/fs/userfaultfd.c > > @@ -2005,17 +2005,8 @@ static int userfaultfd_move(struct userfaultfd_c= tx *ctx, > > return -EINVAL; > > > > if (mmget_not_zero(mm)) { > > - mmap_read_lock(mm); > > - > > - /* Re-check after taking map_changing_lock */ > > - down_read(&ctx->map_changing_lock); > > - if (likely(!atomic_read(&ctx->mmap_changing))) > > - ret =3D move_pages(ctx, mm, uffdio_move.dst, uffd= io_move.src, > > - uffdio_move.len, uffdio_move.mod= e); > > - else > > - ret =3D -EAGAIN; > > - up_read(&ctx->map_changing_lock); > > - mmap_read_unlock(mm); > > + ret =3D move_pages(ctx, uffdio_move.dst, uffdio_move.src, > > + uffdio_move.len, uffdio_move.mode); > > mmput(mm); > > } else { > > return -ESRCH; > > diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_= k.h > > index 3210c3552976..05d59f74fc88 100644 > > --- a/include/linux/userfaultfd_k.h > > +++ b/include/linux/userfaultfd_k.h > > @@ -138,9 +138,8 @@ extern long uffd_wp_range(struct vm_area_struct *vm= a, > > /* move_pages */ > > void double_pt_lock(spinlock_t *ptl1, spinlock_t *ptl2); > > void double_pt_unlock(spinlock_t *ptl1, spinlock_t *ptl2); > > -ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm, > > - unsigned long dst_start, unsigned long src_start, > > - unsigned long len, __u64 flags); > > +ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_star= t, > > + unsigned long src_start, unsigned long len, __u64 flag= s); > > int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *s= rc_pmd, pmd_t dst_pmdval, > > struct vm_area_struct *dst_vma, > > struct vm_area_struct *src_vma, > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > > index 6e2ca04ab04d..d55bf18b80db 100644 > > --- a/mm/userfaultfd.c > > +++ b/mm/userfaultfd.c > > @@ -19,20 +19,39 @@ > > #include > > #include "internal.h" > > > > -static __always_inline > > -struct vm_area_struct *find_dst_vma(struct mm_struct *dst_mm, > > - unsigned long dst_start, > > - unsigned long len) > > +void unpin_vma(struct mm_struct *mm, struct vm_area_struct *vma, bool = *mmap_locked) > > +{ > > + BUG_ON(!vma && !*mmap_locked); > > + > > + if (*mmap_locked) { > > + mmap_read_unlock(mm); > > + *mmap_locked =3D false; > > + } else > > + vma_end_read(vma); > > You are missing braces here. > > This function is small so it could be inline, although I hope the > compiler would get that right for us. > > I don't think this small helper is worth it, considering you are > altering a pointer in here, which makes things harder to follow (not to > mention the locking). The only code that depends on this update is a > single place, which already assigns a custom variable after the function > return. > > > +} > > + > > +/* > > + * Search for VMA and make sure it is stable either by locking it or t= aking > > + * mmap_lock. > > This function returns something that isn't documented and also sets a > boolean which is passed in as a pointer which also is lacking from the > documentation. > > > + */ > > +struct vm_area_struct *find_and_pin_dst_vma(struct mm_struct *dst_mm, > > + unsigned long dst_start, > > + unsigned long len, > > + bool *mmap_locked) > > { > > + struct vm_area_struct *dst_vma =3D lock_vma_under_rcu(dst_mm, dst= _start); > > lock_vma_under_rcu() calls mas_walk(), which goes to dst_start for the > VMA. It is not possible for dst_start to be outside the range. > > > + if (!dst_vma) { > > BUG_ON(mmap_locked) ? > > > + mmap_read_lock(dst_mm); > > + *mmap_locked =3D true; > > + dst_vma =3D find_vma(dst_mm, dst_start); > > find_vma() walks to dst_start and searches upwards from that address. > This is functionally different than what you have asked for above. You > will not see an issue as you have coded it - but it may be suboptimal > since a start address lower than the VMA you are looking for can be > found... however, later you check the range falls between the dst_start > and dst_start + len. > > If you expect the dst_start to always be within the VMA range and not > lower, then you should use vma_lookup(). > > If you want to search upwards from dst_start for a VMA then you should > move the range check below into this brace. > > > + } > > + > > /* > > * Make sure that the dst range is both valid and fully within a > > * single existing vma. > > */ > > - struct vm_area_struct *dst_vma; > > - > > - dst_vma =3D find_vma(dst_mm, dst_start); > > if (!range_in_vma(dst_vma, dst_start, dst_start + len)) > > - return NULL; > > + goto unpin; > > > > /* > > * Check the vma is registered in uffd, this is required to > > @@ -40,9 +59,13 @@ struct vm_area_struct *find_dst_vma(struct mm_struct= *dst_mm, > > * time. > > */ > > if (!dst_vma->vm_userfaultfd_ctx.ctx) > > - return NULL; > > + goto unpin; > > > > return dst_vma; > > + > > +unpin: > > + unpin_vma(dst_mm, dst_vma, mmap_locked); > > + return NULL; > > } > > > > /* Check if dst_addr is outside of file's size. Must be called with pt= l held. */ > > @@ -350,7 +373,8 @@ static pmd_t *mm_alloc_pmd(struct mm_struct *mm, un= signed long address) > > #ifdef CONFIG_HUGETLB_PAGE > > /* > > * mfill_atomic processing for HUGETLB vmas. Note that this routine i= s > > - * called with mmap_lock held, it will release mmap_lock before return= ing. > > + * called with either vma-lock or mmap_lock held, it will release the = lock > > + * before returning. > > */ > > static __always_inline ssize_t mfill_atomic_hugetlb( > > struct userfaultfd_ctx *ctx= , > > @@ -358,7 +382,8 @@ static __always_inline ssize_t mfill_atomic_hugetlb= ( > > unsigned long dst_start, > > unsigned long src_start, > > unsigned long len, > > - uffd_flags_t flags) > > + uffd_flags_t flags, > > + bool *mmap_locked) > > { > > struct mm_struct *dst_mm =3D dst_vma->vm_mm; > > int vm_shared =3D dst_vma->vm_flags & VM_SHARED; > > @@ -380,7 +405,7 @@ static __always_inline ssize_t mfill_atomic_hugetlb= ( > > */ > > if (uffd_flags_mode_is(flags, MFILL_ATOMIC_ZEROPAGE)) { > > up_read(&ctx->map_changing_lock); > > - mmap_read_unlock(dst_mm); > > + unpin_vma(dst_mm, dst_vma, mmap_locked); > > return -EINVAL; > > } > > > > @@ -404,12 +429,25 @@ static __always_inline ssize_t mfill_atomic_huget= lb( > > */ > > if (!dst_vma) { > > err =3D -ENOENT; > > - dst_vma =3D find_dst_vma(dst_mm, dst_start, len); > > - if (!dst_vma || !is_vm_hugetlb_page(dst_vma)) > > - goto out_unlock; > > + dst_vma =3D find_and_pin_dst_vma(dst_mm, dst_start, > > + len, mmap_locked); > > + if (!dst_vma) > > + goto out; > > + if (!is_vm_hugetlb_page(dst_vma)) > > + goto out_unlock_vma; > > > > err =3D -EINVAL; > > if (vma_hpagesize !=3D vma_kernel_pagesize(dst_vma)) > > + goto out_unlock_vma; > > + > > + /* > > + * If memory mappings are changing because of non-coopera= tive > > + * operation (e.g. mremap) running in parallel, bail out = and > > + * request the user to retry later > > + */ > > + down_read(&ctx->map_changing_lock); > > + err =3D -EAGAIN; > > + if (atomic_read(&ctx->mmap_changing)) > > goto out_unlock; > > > > vm_shared =3D dst_vma->vm_flags & VM_SHARED; > > @@ -465,7 +503,7 @@ static __always_inline ssize_t mfill_atomic_hugetlb= ( > > > > if (unlikely(err =3D=3D -ENOENT)) { > > up_read(&ctx->map_changing_lock); > > - mmap_read_unlock(dst_mm); > > + unpin_vma(dst_mm, dst_vma, mmap_locked); > > BUG_ON(!folio); > > > > err =3D copy_folio_from_user(folio, > > @@ -474,17 +512,6 @@ static __always_inline ssize_t mfill_atomic_hugetl= b( > > err =3D -EFAULT; > > goto out; > > } > > - mmap_read_lock(dst_mm); > > - down_read(&ctx->map_changing_lock); > > - /* > > - * If memory mappings are changing because of non= -cooperative > > - * operation (e.g. mremap) running in parallel, b= ail out and > > - * request the user to retry later > > - */ > > - if (atomic_read(ctx->mmap_changing)) { > > - err =3D -EAGAIN; > > - break; > > - } > > ... Okay, this is where things get confusing. > > How about this: Don't do this locking/boolean dance. > > Instead, do something like this: > In mm/memory.c, below lock_vma_under_rcu(), but something like this > > struct vm_area_struct *lock_vma(struct mm_struct *mm, > unsigned long addr)) /* or some better name.. */ > { > struct vm_area_struct *vma; > > vma =3D lock_vma_under_rcu(mm, addr); > > if (vma) > return vma; > > mmap_read_lock(mm); > vma =3D lookup_vma(mm, addr); > if (vma) > vma_start_read(vma); /* Won't fail */ Please don't assume vma_start_read() won't fail even when you have mmap_read_lock(). See the comment in vma_start_read() about the possibility of an overflow producing false negatives. > > mmap_read_unlock(mm); > return vma; > } > > Now, we know we have a vma that's vma locked if there is a vma. The vma > won't go away - you have it locked. The mmap lock is held for even > less time for your worse case, and the code gets easier to follow. > > Once you are done with the vma do a vma_end_read(vma). Don't forget to > do this! > > Now the comment above such a function should state that the vma needs to > be vma_end_read(vma), or that could go undetected.. It might be worth > adding a unlock_vma() counterpart to vma_end_read(vma) even. Locking VMA while holding mmap_read_lock is an interesting usage pattern I haven't seen yet. I think this should work quite well! > > > > > > dst_vma =3D NULL; > > goto retry; > > @@ -505,7 +532,8 @@ static __always_inline ssize_t mfill_atomic_hugetlb= ( > > > > out_unlock: > > up_read(&ctx->map_changing_lock); > > - mmap_read_unlock(dst_mm); > > +out_unlock_vma: > > + unpin_vma(dst_mm, dst_vma, mmap_locked); > > out: > > if (folio) > > folio_put(folio); > > @@ -521,7 +549,8 @@ extern ssize_t mfill_atomic_hugetlb(struct userfaul= tfd_ctx *ctx, > > unsigned long dst_start, > > unsigned long src_start, > > unsigned long len, > > - uffd_flags_t flags); > > + uffd_flags_t flags, > > + bool *mmap_locked); > > Just a thought, tabbing in twice for each argument would make this more > compact. > > > > #endif /* CONFIG_HUGETLB_PAGE */ > > > > static __always_inline ssize_t mfill_atomic_pte(pmd_t *dst_pmd, > > @@ -581,6 +610,7 @@ static __always_inline ssize_t mfill_atomic(struct = userfaultfd_ctx *ctx, > > unsigned long src_addr, dst_addr; > > long copied; > > struct folio *folio; > > + bool mmap_locked =3D false; > > > > /* > > * Sanitize the command parameters: > > @@ -597,7 +627,14 @@ static __always_inline ssize_t mfill_atomic(struct= userfaultfd_ctx *ctx, > > copied =3D 0; > > folio =3D NULL; > > retry: > > - mmap_read_lock(dst_mm); > > + /* > > + * Make sure the vma is not shared, that the dst range is > > + * both valid and fully within a single existing vma. > > + */ > > + err =3D -ENOENT; > > + dst_vma =3D find_and_pin_dst_vma(dst_mm, dst_start, len, &mmap_lo= cked); > > + if (!dst_vma) > > + goto out; > > > > /* > > * If memory mappings are changing because of non-cooperative > > @@ -609,15 +646,6 @@ static __always_inline ssize_t mfill_atomic(struct= userfaultfd_ctx *ctx, > > if (atomic_read(&ctx->mmap_changing)) > > goto out_unlock; > > > > - /* > > - * Make sure the vma is not shared, that the dst range is > > - * both valid and fully within a single existing vma. > > - */ > > - err =3D -ENOENT; > > - dst_vma =3D find_dst_vma(dst_mm, dst_start, len); > > - if (!dst_vma) > > - goto out_unlock; > > - > > err =3D -EINVAL; > > /* > > * shmem_zero_setup is invoked in mmap for MAP_ANONYMOUS|MAP_SHAR= ED but > > @@ -638,8 +666,8 @@ static __always_inline ssize_t mfill_atomic(struct = userfaultfd_ctx *ctx, > > * If this is a HUGETLB vma, pass off to appropriate routine > > */ > > if (is_vm_hugetlb_page(dst_vma)) > > - return mfill_atomic_hugetlb(ctx, dst_vma, dst_start, > > - src_start, len, flags); > > + return mfill_atomic_hugetlb(ctx, dst_vma, dst_start, src= _start > > + len, flags, &mmap_locked); > > > > if (!vma_is_anonymous(dst_vma) && !vma_is_shmem(dst_vma)) > > goto out_unlock; > > @@ -699,7 +727,8 @@ static __always_inline ssize_t mfill_atomic(struct = userfaultfd_ctx *ctx, > > void *kaddr; > > > > up_read(&ctx->map_changing_lock); > > - mmap_read_unlock(dst_mm); > > + unpin_vma(dst_mm, dst_vma, &mmap_locked); > > + > > BUG_ON(!folio); > > > > kaddr =3D kmap_local_folio(folio, 0); > > @@ -730,7 +759,7 @@ static __always_inline ssize_t mfill_atomic(struct = userfaultfd_ctx *ctx, > > > > out_unlock: > > up_read(&ctx->map_changing_lock); > > - mmap_read_unlock(dst_mm); > > + unpin_vma(dst_mm, dst_vma, &mmap_locked); > > out: > > if (folio) > > folio_put(folio); > > @@ -1285,8 +1314,6 @@ static int validate_move_areas(struct userfaultfd= _ctx *ctx, > > * @len: length of the virtual memory range > > * @mode: flags from uffdio_move.mode > > * > > - * Must be called with mmap_lock held for read. > > - * > > * move_pages() remaps arbitrary anonymous pages atomically in zero > > * copy. It only works on non shared anonymous pages because those can > > * be relocated without generating non linear anon_vmas in the rmap > > @@ -1353,15 +1380,16 @@ static int validate_move_areas(struct userfault= fd_ctx *ctx, > > * could be obtained. This is the only additional complexity added to > > * the rmap code to provide this anonymous page remapping functionalit= y. > > */ > > -ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm, > > - unsigned long dst_start, unsigned long src_start, > > - unsigned long len, __u64 mode) > > +ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_star= t, > > + unsigned long src_start, unsigned long len, __u64 mode= ) > > { > > + struct mm_struct *mm =3D ctx->mm; > > struct vm_area_struct *src_vma, *dst_vma; > > unsigned long src_addr, dst_addr; > > pmd_t *src_pmd, *dst_pmd; > > long err =3D -EINVAL; > > ssize_t moved =3D 0; > > + bool mmap_locked =3D false; > > > > /* Sanitize the command parameters. */ > > if (WARN_ON_ONCE(src_start & ~PAGE_MASK) || > > @@ -1374,28 +1402,52 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx,= struct mm_struct *mm, > > WARN_ON_ONCE(dst_start + len <=3D dst_start)) > > goto out; > > Ah, is this safe for rmap? I think you need to leave this read lock. > > > > > + dst_vma =3D NULL; > > + src_vma =3D lock_vma_under_rcu(mm, src_start); > > + if (src_vma) { > > + dst_vma =3D lock_vma_under_rcu(mm, dst_start); > > + if (!dst_vma) > > + vma_end_read(src_vma); > > + } > > + > > + /* If we failed to lock both VMAs, fall back to mmap_lock */ > > + if (!dst_vma) { > > + mmap_read_lock(mm); > > + mmap_locked =3D true; > > + src_vma =3D find_vma(mm, src_start); > > + if (!src_vma) > > + goto out_unlock_mmap; > > + dst_vma =3D find_vma(mm, dst_start); > > Again, there is a difference in how find_vma and lock_vam_under_rcu > works. > > > + if (!dst_vma) > > + goto out_unlock_mmap; > > + } > > + > > + /* Re-check after taking map_changing_lock */ > > + down_read(&ctx->map_changing_lock); > > + if (likely(atomic_read(&ctx->mmap_changing))) { > > + err =3D -EAGAIN; > > + goto out_unlock; > > + } > > /* > > * Make sure the vma is not shared, that the src and dst remap > > * ranges are both valid and fully within a single existing > > * vma. > > */ > > - src_vma =3D find_vma(mm, src_start); > > - if (!src_vma || (src_vma->vm_flags & VM_SHARED)) > > - goto out; > > + if (src_vma->vm_flags & VM_SHARED) > > + goto out_unlock; > > if (src_start < src_vma->vm_start || > > src_start + len > src_vma->vm_end) > > - goto out; > > + goto out_unlock; > > > > - dst_vma =3D find_vma(mm, dst_start); > > - if (!dst_vma || (dst_vma->vm_flags & VM_SHARED)) > > - goto out; > > + if (dst_vma->vm_flags & VM_SHARED) > > + goto out_unlock; > > if (dst_start < dst_vma->vm_start || > > dst_start + len > dst_vma->vm_end) > > - goto out; > > + goto out_unlock; > > > > err =3D validate_move_areas(ctx, src_vma, dst_vma); > > if (err) > > - goto out; > > + goto out_unlock; > > > > for (src_addr =3D src_start, dst_addr =3D dst_start; > > src_addr < src_start + len;) { > > @@ -1512,6 +1564,15 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, = struct mm_struct *mm, > > moved +=3D step_size; > > } > > > > +out_unlock: > > + up_read(&ctx->map_changing_lock); > > +out_unlock_mmap: > > + if (mmap_locked) > > + mmap_read_unlock(mm); > > + else { > > + vma_end_read(dst_vma); > > + vma_end_read(src_vma); > > + } > > out: > > VM_WARN_ON(moved < 0); > > VM_WARN_ON(err > 0); > > -- > > 2.43.0.429.g432eaa2c6b-goog > > > >