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 B89F2C48297 for ; Fri, 9 Feb 2024 20:59:11 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 473846B0074; Fri, 9 Feb 2024 15:59:11 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 423796B007E; Fri, 9 Feb 2024 15:59:11 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 29DB96B0083; Fri, 9 Feb 2024 15:59:11 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 176586B0074 for ; Fri, 9 Feb 2024 15:59:11 -0500 (EST) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id DE5F41207D1 for ; Fri, 9 Feb 2024 20:59:10 +0000 (UTC) X-FDA: 81773480460.03.7B4CE57 Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) by imf03.hostedemail.com (Postfix) with ESMTP id E8FFE20016 for ; Fri, 9 Feb 2024 20:59:08 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=WegJRNKh; spf=pass (imf03.hostedemail.com: domain of lokeshgidra@google.com designates 209.85.128.41 as permitted sender) smtp.mailfrom=lokeshgidra@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707512349; 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=5zrEc8EqBS4KRRpBKI/h97lI/mfHCFzgoeDGIbbkfEk=; b=6ThpedMxK0VckYuaubusvuJ5M8/YsNp86vfMaEddX78AVoOFrtQHk84Is4s/Mti4dVwfXt jMuvr4h6Gd0XuxAiSI3bPWE8KteTKDsDR850BcqTzGbRo1Kd8zk0rFt7SF7Z2vC47mkXr4 fVQAoiMuTvJZ005YaAK7P/LXlLip43c= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707512349; a=rsa-sha256; cv=none; b=56OtMXtQNvbAqB8IfJjrKFyX3bQAtSatVnKD1WwowAsKHsX4bGBJZVpDE5FHX3yu6DcF4X znir9f/FhCuIWSeoi3LYvg3uWhjYyFgtWIa5eO6rCW2G36Wh3/8bF8bPRQ1dzx9TmHqLg7 nDw+SVv22oZsKBwr0E0zCZ7RvmB6yBY= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=WegJRNKh; spf=pass (imf03.hostedemail.com: domain of lokeshgidra@google.com designates 209.85.128.41 as permitted sender) smtp.mailfrom=lokeshgidra@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-wm1-f41.google.com with SMTP id 5b1f17b1804b1-41089cc9ae4so439995e9.1 for ; Fri, 09 Feb 2024 12:59:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1707512347; x=1708117147; 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=5zrEc8EqBS4KRRpBKI/h97lI/mfHCFzgoeDGIbbkfEk=; b=WegJRNKhJyvN1FBEAHl/n5FqB21tlglyXfV99DiYT7gqvjn0AfvnqOarThCq5wMcQl ERVJcLcB37AVeM3iSUWR1AzQWb1r0mqcLg6yZB3c201J4ISZLO831dqjs1qfOw1l74u7 WE7qfLOymZNGbh9/uAzd5FJK8E4u/s8Pxx6+dqIqWyXNBBbSndRt+v1FLdbgsWwC2kyo trlro/RUNV9qqqIRkyUFM1pBzoNTIrsFGW8VkehcYG53J2iZs6TNQcZdOOXTf7jvFSWu rj3J0fgdmg3TPAaRL32PmjVP0lDpF9tzHIkISvVHzsytIsyl0mez1Popu0zSI50HPEiS sVYw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707512347; x=1708117147; 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=5zrEc8EqBS4KRRpBKI/h97lI/mfHCFzgoeDGIbbkfEk=; b=NCpp3V7w3HBLNXUDRxhPtkX3O1xYBoDKnDrxCjEM1Tmb7TnZAxqygL2G7EhIOCSu9C rW8onq/vgkZ3noOzgfw8kpMBthSR7Ri6s9gf3CK108cGAlE9hgoJJl8bKOrIWI3Krh7I SQzjv4OK22DU0KbdBdIJKcM8ECx8zdhZ0W/lkbHkUM9RsS9jtk6qPZbkS2QNiUZLgLy5 nHhA5v2lGWdbewkpl9MbE2Zl9t3RzlgBfIcVkLheTMlwc7RlFWt2oGzeKaISAzi9tqxQ hnf4V0myVc/xTuv+pmvUGktbbxodyR16BPdFuAI94jgNBqpAqED7hfkI6cNdC3cFOAsy LTsQ== X-Gm-Message-State: AOJu0YwcGW8aTBcHWjQ2dHWv5MwFp6/WljHp+kO0v5Tj2dh0KeNjHpgc xnb2XUIEId0bQGw9+LlAshlp/6PT3Yp4T7D7M+k6EIAG7ZhtewXQ1HCgUsgZcCH5Fd+G1xHjx4S H/Wwrz2b3EECHyHLfov7xO2lRYk26fR6ba3Cg X-Google-Smtp-Source: AGHT+IET79bq/KXualeLq6y64qmiChq2VEWzMI7B+dTzlcxES5Vi+I1Oyhry+wQjYe6iol3G7/KYPZwHB9JOJ/+1n0A= X-Received: by 2002:a05:600c:1910:b0:40f:d3d8:c8d5 with SMTP id j16-20020a05600c191000b0040fd3d8c8d5mr328584wmq.9.1707512347048; Fri, 09 Feb 2024 12:59:07 -0800 (PST) MIME-Version: 1.0 References: <20240208212204.2043140-1-lokeshgidra@google.com> <20240208212204.2043140-4-lokeshgidra@google.com> <20240209030654.lxh4krmxmiuszhab@revolver> <20240209190605.7gokzhg7afy7ibyf@revolver> <20240209193110.ltfdc6nolpoa2ccv@revolver> In-Reply-To: <20240209193110.ltfdc6nolpoa2ccv@revolver> From: Lokesh Gidra Date: Fri, 9 Feb 2024 12:58:54 -0800 Message-ID: Subject: Re: [PATCH v4 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-Stat-Signature: 3jjejo69b3orx3ccrqo3cb9x64nhxrg8 X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: E8FFE20016 X-Rspam-User: X-HE-Tag: 1707512348-256235 X-HE-Meta: U2FsdGVkX193UHTKB8N0zb8XTI0wWWFnG/z/vtN0gHXSvfjgQuoPZsEt2rbBPhLtD8xEf3WsZ8g32cDHNe/nY7bKCkZc+MKgQLn76jm87QXBL0UIK/fIbCsV47babliUD4so47NQsirFoQwEuquKcGeTVTkqShKCo7k9UnqFi0aIt84odvyEtxLnV7mx4oMLnzM+Lkcx4N4/wLoa/cfZAw0JhxOD9vH80NddgNiy2ANeDH/cRreCuWVA3I851FHdeL/3OpNptZyXT6uTK2J2YYyNSfn8fBpT6fVLN4RcPmV0z6C6Jh+UsgMe+F10hDy/cbjwsdJginTpOAbt6SAMFuukKJKA6fPEjdiea3hXuNuwK8i6cf30CKneekHKbGbog3EqvbAN1c22UwqyteD5w9nhdTHOUXogKBVmiS8TxMbCHlSQLGz7oD3ZboYOR0vgskDeaS0pmFb7LvQXbBtrEV+WnbuTyv29viTvCYYAxAqTlQv47/ylb80Xae5MWctVw6WKCqEW+wsb3940Y+LhKVpb7eo+spN6TDJDy2NzYshk/DFwIh2aydVrVO4LX6zMEEYjtM5cwfG205WPlFp3ionONSi0BPBgwoyDOeOw17iz8kZiROTvp2Y6qSdb4i9UfZMn52KWkN6jir7YyRoD0RbSGPG6vlw1ADlTHPYnftAkYkfqfegv8XSKKxGeSMBVHr76XwQXFQoF9xLmWZVbp7/srxaFQjrelWB3/GxeONRUCL76JsiLBL7IdhzdU/+eAVQ7s17s9c56VvoyiqcnQ1duoeZssRuoSmxqfHCp5gwHLyTkCDYm+c3UUNKMJfFhAVIFHLB/OyO7ShVDuZw1z/I/w8aukcmmXYk/ECqO+N7+D17vy6bjHmQ/07zYM3Kcs5Z8YWjzwDpuN9MHEha1yDrOVYpe5XnCzpyX52DWpNFtPqJ6H4SMWMSLKIb9h4vr1RNb33jEV33Y/3jE5vu u1Emapjq kMZzC2SsBPPmuv1BUboNPoPRqeKX5+UVTHjtgQw22iR70hkmBwx/YHjb3kao6jmoDR5HOFbRFoXgL7cWa3CW5UHEod5ipBSFzgH7TZsKbNMtb+8S6aw1yv+mQIwjlvDThZY7gWUAh8joAs6fQ4nvzjJPg8BWPSO/yqvURnejE6bIZN7xImJZISxQ446dsYi9M1jMSbAk2kdco1GMhWyAX408SwsTsKBAY7OIQzz2bFqbojVHoLnmv4HiqRS9RvquVS17vi6Dul35dwdGumcrlt/QFhw== 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 Fri, Feb 9, 2024 at 11:31=E2=80=AFAM Liam R. Howlett wrote: > > * Lokesh Gidra [240209 14:21]: > > On Fri, Feb 9, 2024 at 11:06=E2=80=AFAM Liam R. Howlett wrote: > > > > > > * Lokesh Gidra [240209 13:02]: > > > > On Thu, Feb 8, 2024 at 7:07=E2=80=AFPM Liam R. Howlett wrote: > > > > > > > > > > * Lokesh Gidra [240208 16:22]: > > > > > > All userfaultfd operations, except write-protect, opportunistic= ally use > > > > > > per-vma locks to lock vmas. On failure, attempt again inside mm= ap_lock > > > > > > critical section. > > > > > > > > > > > > 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 | 356 ++++++++++++++++++++++++++= -------- > > > > > > 3 files changed, 275 insertions(+), 99 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 userf= aultfd_ctx *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.d= st, uffdio_move.src, > > > > > > - uffdio_move.len, uffdio_= move.mode); > > > > > > - else > > > > > > - ret =3D -EAGAIN; > > > > > > - up_read(&ctx->map_changing_lock); > > > > > > - mmap_read_unlock(mm); > > > > > > + ret =3D move_pages(ctx, uffdio_move.dst, uffdio_m= ove.src, > > > > > > + uffdio_move.len, uffdio_move.mod= e); > > > > > > mmput(mm); > > > > > > } else { > > > > > > return -ESRCH; > > > > > > diff --git a/include/linux/userfaultfd_k.h b/include/linux/user= faultfd_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_st= ruct *vma, > > > > > > /* 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_stru= ct *mm, > > > > > > - unsigned long dst_start, unsigned long src_sta= rt, > > > > > > - unsigned long len, __u64 flags); > > > > > > +ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long = dst_start, > > > > > > + unsigned long src_start, unsigned long len, __= u64 flags); > > > > > > int move_pages_huge_pmd(struct mm_struct *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, > > > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > > > > > > index 74aad0831e40..1e25768b2136 100644 > > > > > > --- a/mm/userfaultfd.c > > > > > > +++ b/mm/userfaultfd.c > > > > > > @@ -19,20 +19,12 @@ > > > > > > #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) > > > > > > > > > > You could probably leave the __always_inline for this. > > > > > > > > Sure > > > > > > > > > > > +static bool validate_dst_vma(struct vm_area_struct *dst_vma, > > > > > > + unsigned long dst_end) > > > > > > { > > > > > > - /* > > > > > > - * Make sure that the dst range is both valid and fully w= ithin 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; > > > > > > + /* Make sure that the dst range is fully within dst_vma. = */ > > > > > > + if (dst_end > dst_vma->vm_end) > > > > > > + return false; > > > > > > > > > > > > /* > > > > > > * Check the vma is registered in uffd, this is required = to > > > > > > @@ -40,11 +32,125 @@ struct vm_area_struct *find_dst_vma(struct= mm_struct *dst_mm, > > > > > > * time. > > > > > > */ > > > > > > if (!dst_vma->vm_userfaultfd_ctx.ctx) > > > > > > - return NULL; > > > > > > + return false; > > > > > > + > > > > > > + return true; > > > > > > +} > > > > > > + > > > > > > +#ifdef CONFIG_PER_VMA_LOCK > > > > > > +/* > > > > > > + * lock_vma() - Lookup and lock vma corresponding to @address. > > > > > > + * @mm: mm to search vma in. > > > > > > + * @address: address that the vma should contain. > > > > > > + * @prepare_anon: If true, then prepare the vma (if private) w= ith anon_vma. > > > > > > + * > > > > > > + * Should be called without holding mmap_lock. vma should be u= nlocked after use > > > > > > + * with unlock_vma(). > > > > > > + * > > > > > > + * Return: A locked vma containing @address, NULL if no vma is= found, or > > > > > > + * -ENOMEM if anon_vma couldn't be allocated. > > > > > > + */ > > > > > > +static struct vm_area_struct *lock_vma(struct mm_struct *mm, > > > > > > + unsigned long address, > > > > > > + bool prepare_anon) > > > > > > +{ > > > > > > + struct vm_area_struct *vma; > > > > > > + > > > > > > + vma =3D lock_vma_under_rcu(mm, address); > > > > > > + if (vma) { > > > > > > + /* > > > > > > + * lock_vma_under_rcu() only checks anon_vma for = private > > > > > > + * anonymous mappings. But we need to ensure it i= s assigned in > > > > > > + * private file-backed vmas as well. > > > > > > + */ > > > > > > + if (prepare_anon && !(vma->vm_flags & VM_SHARED) = && > > > > > > + !vma->anon_vma) > > > > > > + vma_end_read(vma); > > > > > > + else > > > > > > + return vma; > > > > > > + } > > > > > > + > > > > > > + mmap_read_lock(mm); > > > > > > + vma =3D vma_lookup(mm, address); > > > > > > + if (vma) { > > > > > > + if (prepare_anon && !(vma->vm_flags & VM_SHARED) = && > > > > > > + anon_vma_prepare(vma)) { > > > > > > + vma =3D ERR_PTR(-ENOMEM); > > > > > > + } else { > > > > > > + /* > > > > > > + * We cannot use vma_start_read() as it m= ay fail due to > > > > > > + * false locked (see comment in vma_start= _read()). We > > > > > > + * can avoid that by directly locking vm_= lock under > > > > > > + * mmap_lock, which guarantees that nobod= y can lock the > > > > > > + * vma for write (vma_start_write()) unde= r us. > > > > > > + */ > > > > > > + down_read(&vma->vm_lock->lock); > > > > > > + } > > > > > > + } > > > > > > + > > > > > > + mmap_read_unlock(mm); > > > > > > + return vma; > > > > > > +} > > > > > > + > > > > > > +static void unlock_vma(struct vm_area_struct *vma) > > > > > > +{ > > > > > > + vma_end_read(vma); > > > > > > +} > > > > > > + > > > > > > +static struct vm_area_struct *find_and_lock_dst_vma(struct mm_= struct *dst_mm, > > > > > > + unsigned long= dst_start, > > > > > > + unsigned long= len) > > > > > > +{ > > > > > > + struct vm_area_struct *dst_vma; > > > > > > + > > > > > > + /* Ensure anon_vma is assigned for private vmas */ > > > > > > + dst_vma =3D lock_vma(dst_mm, dst_start, true); > > > > > > + > > > > > > + if (!dst_vma) > > > > > > + return ERR_PTR(-ENOENT); > > > > > > + > > > > > > + if (PTR_ERR(dst_vma) =3D=3D -ENOMEM) > > > > > > + return dst_vma; > > > > > > + > > > > > > + if (!validate_dst_vma(dst_vma, dst_start + len)) > > > > > > + goto out_unlock; > > > > > > > > > > > > return dst_vma; > > > > > > +out_unlock: > > > > > > + unlock_vma(dst_vma); > > > > > > + return ERR_PTR(-ENOENT); > > > > > > } > > > > > > > > > > > > +#else > > > > > > + > > > > > > +static struct vm_area_struct *lock_mm_and_find_dst_vma(struct = mm_struct *dst_mm, > > > > > > + unsigned l= ong dst_start, > > > > > > + unsigned l= ong len) > > > > > > +{ > > > > > > + struct vm_area_struct *dst_vma; > > > > > > + int err =3D -ENOENT; > > > > > > + > > > > > > + mmap_read_lock(dst_mm); > > > > > > + dst_vma =3D vma_lookup(dst_mm, dst_start); > > > > > > + if (!dst_vma) > > > > > > + goto out_unlock; > > > > > > + > > > > > > + /* Ensure anon_vma is assigned for private vmas */ > > > > > > + if (!(dst_vma->vm_flags & VM_SHARED) && anon_vma_prepare(= dst_vma)) { > > > > > > + err =3D -ENOMEM; > > > > > > + goto out_unlock; > > > > > > + } > > > > > > + > > > > > > + if (!validate_dst_vma(dst_vma, dst_start + len)) > > > > > > + goto out_unlock; > > > > > > + > > > > > > + return dst_vma; > > > > > > +out_unlock: > > > > > > + mmap_read_unlock(dst_mm); > > > > > > + return ERR_PTR(err); > > > > > > +} > > > > > > +#endif > > > > > > + > > > > > > /* Check if dst_addr is outside of file's size. Must be called= with ptl held. */ > > > > > > static bool mfill_file_over_size(struct vm_area_struct *dst_vm= a, > > > > > > unsigned long dst_addr) > > > > > > @@ -350,7 +456,8 @@ static pmd_t *mm_alloc_pmd(struct mm_struct= *mm, unsigned long address) > > > > > > #ifdef CONFIG_HUGETLB_PAGE > > > > > > /* > > > > > > * mfill_atomic processing for HUGETLB vmas. Note that this r= outine is > > > > > > - * called with mmap_lock held, it will release mmap_lock befor= e returning. > > > > > > + * called with either vma-lock or mmap_lock held, it will rele= ase the lock > > > > > > + * before returning. > > > > > > */ > > > > > > static __always_inline ssize_t mfill_atomic_hugetlb( > > > > > > struct userfaultfd_= ctx *ctx, > > > > > > @@ -361,7 +468,6 @@ static __always_inline ssize_t mfill_atomic= _hugetlb( > > > > > > uffd_flags_t flags) > > > > > > { > > > > > > struct mm_struct *dst_mm =3D dst_vma->vm_mm; > > > > > > - int vm_shared =3D dst_vma->vm_flags & VM_SHARED; > > > > > > ssize_t err; > > > > > > pte_t *dst_pte; > > > > > > unsigned long src_addr, dst_addr; > > > > > > @@ -380,7 +486,11 @@ static __always_inline ssize_t mfill_atomi= c_hugetlb( > > > > > > */ > > > > > > if (uffd_flags_mode_is(flags, MFILL_ATOMIC_ZEROPAGE)) { > > > > > > up_read(&ctx->map_changing_lock); > > > > > > +#ifdef CONFIG_PER_VMA_LOCK > > > > > > + unlock_vma(dst_vma); > > > > > > +#else > > > > > > mmap_read_unlock(dst_mm); > > > > > > +#endif > > > > > > return -EINVAL; > > > > > > } > > > > > > > > > > > > @@ -403,24 +513,32 @@ static __always_inline ssize_t mfill_atom= ic_hugetlb( > > > > > > * retry, dst_vma will be set to NULL and we must lookup = again. > > > > > > */ > > > > > > if (!dst_vma) { > > > > > > +#ifdef CONFIG_PER_VMA_LOCK > > > > > > + dst_vma =3D find_and_lock_dst_vma(dst_mm, dst_sta= rt, len); > > > > > > +#else > > > > > > + dst_vma =3D lock_mm_and_find_dst_vma(dst_mm, dst_= start, len); > > > > > > +#endif > > > > > > + if (IS_ERR(dst_vma)) { > > > > > > + err =3D PTR_ERR(dst_vma); > > > > > > + goto out; > > > > > > + } > > > > > > + > > > > > > 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; > > > > > > + if (!is_vm_hugetlb_page(dst_vma)) > > > > > > + goto out_unlock_vma; > > > > > > > > > > > > err =3D -EINVAL; > > > > > > if (vma_hpagesize !=3D vma_kernel_pagesize(dst_vm= a)) > > > > > > - goto out_unlock; > > > > > > - > > > > > > - vm_shared =3D dst_vma->vm_flags & VM_SHARED; > > > > > > - } > > > > > > + goto out_unlock_vma; > > > > > > > > > > > > - /* > > > > > > - * If not shared, ensure the dst_vma has a anon_vma. > > > > > > - */ > > > > > > - err =3D -ENOMEM; > > > > > > - if (!vm_shared) { > > > > > > - if (unlikely(anon_vma_prepare(dst_vma))) > > > > > > + /* > > > > > > + * 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 > > > > > > + */ > > > > > > + down_read(&ctx->map_changing_lock); > > > > > > + err =3D -EAGAIN; > > > > > > + if (atomic_read(&ctx->mmap_changing)) > > > > > > goto out_unlock; > > > > > > } > > > > > > > > > > > > @@ -465,7 +583,11 @@ static __always_inline ssize_t mfill_atomi= c_hugetlb( > > > > > > > > > > > > if (unlikely(err =3D=3D -ENOENT)) { > > > > > > up_read(&ctx->map_changing_lock); > > > > > > +#ifdef CONFIG_PER_VMA_LOCK > > > > > > + unlock_vma(dst_vma); > > > > > > +#else > > > > > > mmap_read_unlock(dst_mm); > > > > > > +#endif > > > > > > BUG_ON(!folio); > > > > > > > > > > > > err =3D copy_folio_from_user(folio, > > > > > > @@ -474,17 +596,6 @@ static __always_inline ssize_t mfill_atomi= c_hugetlb( > > > > > > err =3D -EFAULT; > > > > > > goto out; > > > > > > } > > > > > > - mmap_read_lock(dst_mm); > > > > > > - down_read(&ctx->map_changing_lock); > > > > > > - /* > > > > > > - * If memory mappings are changing becaus= e of non-cooperative > > > > > > - * operation (e.g. mremap) running in par= allel, bail out and > > > > > > - * request the user to retry later > > > > > > - */ > > > > > > - if (atomic_read(&ctx->mmap_changing)) { > > > > > > - err =3D -EAGAIN; > > > > > > - break; > > > > > > - } > > > > > > > > > > > > dst_vma =3D NULL; > > > > > > goto retry; > > > > > > @@ -505,7 +616,12 @@ static __always_inline ssize_t mfill_atomi= c_hugetlb( > > > > > > > > > > > > out_unlock: > > > > > > up_read(&ctx->map_changing_lock); > > > > > > +out_unlock_vma: > > > > > > +#ifdef CONFIG_PER_VMA_LOCK > > > > > > + unlock_vma(dst_vma); > > > > > > +#else > > > > > > mmap_read_unlock(dst_mm); > > > > > > +#endif > > > > > > out: > > > > > > if (folio) > > > > > > folio_put(folio); > > > > > > @@ -597,7 +713,19 @@ static __always_inline ssize_t mfill_atomi= c(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. > > > > > > + */ > > > > > > +#ifdef CONFIG_PER_VMA_LOCK > > > > > > + dst_vma =3D find_and_lock_dst_vma(dst_mm, dst_start, len)= ; > > > > > > +#else > > > > > > + dst_vma =3D lock_mm_and_find_dst_vma(dst_mm, dst_start, l= en); > > > > > > +#endif > > > > > > + if (IS_ERR(dst_vma)) { > > > > > > + err =3D PTR_ERR(dst_vma); > > > > > > + goto out; > > > > > > + } > > > > > > > > > > > > /* > > > > > > * If memory mappings are changing because of non-coopera= tive > > > > > > @@ -609,15 +737,6 @@ static __always_inline ssize_t mfill_atomi= c(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_SHARED but > > > > > > @@ -647,16 +766,6 @@ static __always_inline ssize_t mfill_atomi= c(struct userfaultfd_ctx *ctx, > > > > > > uffd_flags_mode_is(flags, MFILL_ATOMIC_CONTINUE)) > > > > > > goto out_unlock; > > > > > > > > > > > > - /* > > > > > > - * Ensure the dst_vma has a anon_vma or this page > > > > > > - * would get a NULL anon_vma when moved in the > > > > > > - * dst_vma. > > > > > > - */ > > > > > > - err =3D -ENOMEM; > > > > > > - if (!(dst_vma->vm_flags & VM_SHARED) && > > > > > > - unlikely(anon_vma_prepare(dst_vma))) > > > > > > - goto out_unlock; > > > > > > - > > > > > > while (src_addr < src_start + len) { > > > > > > pmd_t dst_pmdval; > > > > > > > > > > > > @@ -699,7 +808,11 @@ static __always_inline ssize_t mfill_atomi= c(struct userfaultfd_ctx *ctx, > > > > > > void *kaddr; > > > > > > > > > > > > up_read(&ctx->map_changing_lock); > > > > > > +#ifdef CONFIG_PER_VMA_LOCK > > > > > > + unlock_vma(dst_vma); > > > > > > +#else > > > > > > mmap_read_unlock(dst_mm); > > > > > > +#endif > > > > > > BUG_ON(!folio); > > > > > > > > > > > > kaddr =3D kmap_local_folio(folio, 0); > > > > > > @@ -730,7 +843,11 @@ static __always_inline ssize_t mfill_atomi= c(struct userfaultfd_ctx *ctx, > > > > > > > > > > > > out_unlock: > > > > > > up_read(&ctx->map_changing_lock); > > > > > > +#ifdef CONFIG_PER_VMA_LOCK > > > > > > + unlock_vma(dst_vma); > > > > > > +#else > > > > > > mmap_read_unlock(dst_mm); > > > > > > +#endif > > > > > > out: > > > > > > if (folio) > > > > > > folio_put(folio); > > > > > > @@ -1267,16 +1384,67 @@ static int validate_move_areas(struct u= serfaultfd_ctx *ctx, > > > > > > if (!vma_is_anonymous(src_vma) || !vma_is_anonymous(dst_v= ma)) > > > > > > return -EINVAL; > > > > > > > > > > > > - /* > > > > > > - * 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; > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > +#ifdef CONFIG_PER_VMA_LOCK > > > > > > +static int find_and_lock_vmas(struct mm_struct *mm, > > > > > > + unsigned long dst_start, > > > > > > + unsigned long src_start, > > > > > > + struct vm_area_struct **dst_vmap, > > > > > > + struct vm_area_struct **src_vmap) > > > > > > +{ > > > > > > + int err; > > > > > > + > > > > > > + /* There is no need to prepare anon_vma for src_vma */ > > > > > > + *src_vmap =3D lock_vma(mm, src_start, false); > > > > > > + if (!*src_vmap) > > > > > > + return -ENOENT; > > > > > > + > > > > > > + /* Ensure anon_vma is assigned for anonymous vma */ > > > > > > + *dst_vmap =3D lock_vma(mm, dst_start, true); > > > > > > + err =3D -ENOENT; > > > > > > + if (!*dst_vmap) > > > > > > + goto out_unlock; > > > > > > + > > > > > > + err =3D -ENOMEM; > > > > > > + if (PTR_ERR(*dst_vmap) =3D=3D -ENOMEM) > > > > > > + goto out_unlock; > > > > > > > > > > If you change lock_vma() to return the vma or ERR_PTR(-ENOENT) / > > > > > ERR_PTR(-ENOMEM), then you could change this to check IS_ERR() an= d > > > > > return the PTR_ERR(). > > > > > > > > > > You could also use IS_ERR_OR_NULL here, but the first suggestion = will > > > > > simplify your life for find_and_lock_dst_vma() and the error type= to > > > > > return. > > > > > > > > Good suggestion. I'll make the change. Thanks > > > > > > > > > > What you have here will work though. > > > > > > > > > > > > > > > > > return 0; > > > > > > +out_unlock: > > > > > > + unlock_vma(*src_vmap); > > > > > > + return err; > > > > > > } The current implementation has a deadlock problem: thread 1 thread 2 vma_start_read(dst_vma) mmap_write_lock() vma_start_write(src_vma) vma_start_read(src_vma) fails mmap_read_lock() blocks vma_start_write(dst_vma) blocks I think the solution is to implement it this way: long find_and_lock_vmas(...) { dst_vma =3D lock_vma(mm, dst_start, true); if (IS_ERR(dst_vma)) return PTR_ERR(vma); src_vma =3D lock_vma_under_rcu(mm, src_start); if (!src_vma) { long err; if (mmap_read_trylock(mm)) { src_vma =3D vma_lookup(mm, src_= start); if (src_vma) { down_read(&src_vma->vm_lock->lock); err =3D 0; } else { err =3D -ENOENT; } mmap_read_unlock(mm); } else { vma_end_read(dst_vma); err =3D lock_mm_and_find_vmas(..= .); if (!err) { mmap_read_unlock(m= m); } } return err; } return 0; } Of course this would need defining lock_mm_and_find_vmas() regardless of CONFIG_PER_VMA_LOCK. I can also remove the prepare_anon condition in lock_vma(). > > > > > > +#else > > > > > > +static int lock_mm_and_find_vmas(struct mm_struct *mm, > > > > > > + unsigned long dst_start, > > > > > > + unsigned long src_start, > > > > > > + struct vm_area_struct **dst_vmap= , > > > > > > + struct vm_area_struct **src_vmap= ) > > > > > > +{ > > > > > > + int err =3D -ENOENT; > > > > > > > > > > Nit: new line after declarations. > > > > > > > > > > > + mmap_read_lock(mm); > > > > > > + > > > > > > + *src_vmap =3D vma_lookup(mm, src_start); > > > > > > + if (!*src_vmap) > > > > > > + goto out_unlock; > > > > > > + > > > > > > + *dst_vmap =3D vma_lookup(mm, dst_start); > > > > > > + if (!*dst_vmap) > > > > > > + goto out_unlock; > > > > > > + > > > > > > + /* Ensure anon_vma is assigned */ > > > > > > + err =3D -ENOMEM; > > > > > > + if (vma_is_anonymous(*dst_vmap) && anon_vma_prepare(*dst_= vmap)) > > > > > > + goto out_unlock; > > > > > > + > > > > > > + return 0; > > > > > > +out_unlock: > > > > > > + mmap_read_unlock(mm); > > > > > > + return err; > > > > > > +} > > > > > > +#endif > > > > > > > > > > > > /** > > > > > > * move_pages - move arbitrary anonymous pages of an existing = vma > > > > > > @@ -1287,8 +1455,6 @@ static int validate_move_areas(struct use= rfaultfd_ctx *ctx, > > > > > > * @len: length of the virtual memory range > > > > > > * @mode: flags from uffdio_move.mode > > > > > > * > > > > > > - * Must be called with mmap_lock held for read. > > > > > > - * > > > > > > > > > > Will either use the mmap_lock in read mode or per-vma locking ? > > > > > > > > Makes sense. Will add it. > > > > > > > > > > > * move_pages() remaps arbitrary anonymous pages atomically in= zero > > > > > > * copy. It only works on non shared anonymous pages because t= hose can > > > > > > * be relocated without generating non linear anon_vmas in the= rmap > > > > > > @@ -1355,10 +1521,10 @@ static int validate_move_areas(struct u= serfaultfd_ctx *ctx, > > > > > > * could be obtained. This is the only additional complexity a= dded to > > > > > > * the rmap code to provide this anonymous page remapping func= tionality. > > > > > > */ > > > > > > -ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_stru= ct *mm, > > > > > > - unsigned long dst_start, unsigned long src_sta= rt, > > > > > > - unsigned long len, __u64 mode) > > > > > > +ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long = dst_start, > > > > > > + unsigned long src_start, unsigned long len, __= u64 mode) > > > > > > { > > > > > > + struct mm_struct *mm =3D ctx->mm; > > > > > > > > > > You dropped the argument, but left the comment for the argument. > > > > > > > > Thanks, will fix it. > > > > > > > > > > > struct vm_area_struct *src_vma, *dst_vma; > > > > > > unsigned long src_addr, dst_addr; > > > > > > pmd_t *src_pmd, *dst_pmd; > > > > > > @@ -1376,28 +1542,40 @@ ssize_t move_pages(struct userfaultfd_c= tx *ctx, struct mm_struct *mm, > > > > > > WARN_ON_ONCE(dst_start + len <=3D dst_start)) > > > > > > goto out; > > > > > > > > > > > > +#ifdef CONFIG_PER_VMA_LOCK > > > > > > + err =3D find_and_lock_vmas(mm, dst_start, src_start, > > > > > > + &dst_vma, &src_vma); > > > > > > +#else > > > > > > + err =3D lock_mm_and_find_vmas(mm, dst_start, src_start, > > > > > > + &dst_vma, &src_vma); > > > > > > +#endif > > > > > > > > > > I was hoping you could hide this completely, but it's probably be= tter to > > > > > show what's going on and the function names document it well. > > > > > > > > I wanted to hide unlock as it's called several times, but then I > > > > thought you wanted explicit calls to mmap_read_unlock() so didn't h= ide > > > > it. If you are ok can I define unlock_vma() for !CONFIG_PER_VMA_LOC= K > > > > as well, calling mmap_read_unlock()? > > > > > > My bigger problem was with the name. We can't have unlock_vma() > > > just unlock the mm - it is confusing to read and I think it'll lead t= o > > > misunderstandings of what is really going on here. > > > > > > Whatever you decide to do is fine as long as it's clear what's going = on. > > > I think this is clear while hiding it could also be clear with the ri= ght > > > function name - I'm not sure what that would be; naming is hard. > > > > Maybe unlock_mm_or_vma() ? If not then I'll just keep it as is. > > > > Maybe just leave it as it is unless someone else has issue with it. > Using some form of uffd_unlock name runs into the question of that > atomic and the new lock. Sure. I'll let it as it is. > >