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 3558FC4828F for ; Wed, 7 Feb 2024 18:50:25 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 959976B0078; Wed, 7 Feb 2024 13:50:24 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 8E1AB6B007D; Wed, 7 Feb 2024 13:50:24 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 73FC36B007E; Wed, 7 Feb 2024 13:50:24 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 5DB736B0078 for ; Wed, 7 Feb 2024 13:50:24 -0500 (EST) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 352F2803BC for ; Wed, 7 Feb 2024 18:50:24 +0000 (UTC) X-FDA: 81765898368.07.583EA98 Received: from mail-wm1-f46.google.com (mail-wm1-f46.google.com [209.85.128.46]) by imf30.hostedemail.com (Postfix) with ESMTP id 694DF8001D for ; Wed, 7 Feb 2024 18:50:22 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=pekD3s87; spf=pass (imf30.hostedemail.com: domain of lokeshgidra@google.com designates 209.85.128.46 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=1707331822; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc: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=slYbxMwYxeMYBVKVdbMzEGm/TzHLZWmuYeE2/l4I/eA=; b=Lpw5bGY0/EeAcEJskWrJFgNvRPQY8mR5ZUKjgPPowywyc4Myw97s2NfqLCscGNbwJJ2wZW ERZ6x5LSVWGBb6gl7RIpPsf6A9Mzms2d51uIaIEQeir1POKY7JMPSyGyZdWFxchBsVBfo0 7TBaF8AyjiWgWp4Ae0uDO50zLN7Y1Eg= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707331822; a=rsa-sha256; cv=none; b=v0Vv+czvwWAMbhzBt8D6EKT2soXqc4r0Owdt2H6BDjkOjYxG4SO4J0OY0S33v2EKrcxGBO Rjn1yfYXKz40UNpMbgXo6zS+WOmWqkXrwa+DDdcpQQy0GuExnWt/rGveYr/sxholklm9xn daIDdYKAh6BmWFv6Pw/vcyMcfK0DY8c= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=pekD3s87; spf=pass (imf30.hostedemail.com: domain of lokeshgidra@google.com designates 209.85.128.46 as permitted sender) smtp.mailfrom=lokeshgidra@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-wm1-f46.google.com with SMTP id 5b1f17b1804b1-4101fc00832so4896785e9.3 for ; Wed, 07 Feb 2024 10:50:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1707331821; x=1707936621; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=slYbxMwYxeMYBVKVdbMzEGm/TzHLZWmuYeE2/l4I/eA=; b=pekD3s87Jc6ueZcMq9yOQPgz4OZ2d9YFlF2Z1eturDd2CPc8FS5P7odyFaw5gQyGgB H2QFt4tj4NMvKuO9zEdsnyCmweNH5Cl7n6LahiRsJSMfku648VAgSy+yT5DNwSd/FJNr pcazECD6mr0mRF84qsdUHN1K2FOuI+BW35KzvnjPe2ySRmyPKH/fK+pHwArPF5LlwhgP Ga0LThXFo50FNKT7v32HUERBEYTXaeID7PuXy7q24dbbo81BfEW9FazA+jpbkvigrXy9 EmvchC9M1uEo2M8CNw6gRDQSc9iXSnoLr6mlm3PG30daD9/+fNpaF8dNQtHy+KMwHm5V hGLQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707331821; x=1707936621; h=content-transfer-encoding:cc: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=slYbxMwYxeMYBVKVdbMzEGm/TzHLZWmuYeE2/l4I/eA=; b=YdED8udC0jX/SfpTQaSpp/aR2wQbZA6O/gxvOpJKqtgzpT3y+NVCmjBR+EzHaUbPM2 5ivsoj3lJF4/aGE5lDYKe9ylKoUEjmdKcfF3J/7/SOxO9r2OXJX9evQ0YNmXSGQ6CPW5 EJmPHXbbdW7hyfa7RPQ34EmxC3Eqr2FUm+bqOgz+Y1W73ZQqaaMiVWxbCv7ilZ/o6zVG yiDX8eyXNU6bO4lOCtnFOjlpXdL1kjQdT4jI4qDPVkxCzI/kwDmyQxdat7ZkMiyG3mUq GHUORCG2muTx3k7X8D+20E8SGmE06niUtlGfOTV2rv66Hcu8hMUAoV/zj0tSu4/Ir44B K2mQ== X-Gm-Message-State: AOJu0YyGFnWiJeIrsX4ZvLO9G1AAewo+wz0JHJ2MAR3BUmN0tRdYjoLi vNSMjwTinR9hSHNGL69hlE69xvMvIBTlKpv3eanDbGbgbbCcjEDbYyJuKyP+Rc2jXz9Y+6QkfP1 EMOE/K2FRybYMgym2gUpWmH4KGZ4A7OA+XgT3 X-Google-Smtp-Source: AGHT+IG0JTw4oZowLgnN8QxNdN+nmk0HIO8joSXtWDsL509mK3h2BqePpHbK4YwZJLB/ZFQYXXR2AjhOoekibUbDJDA= X-Received: by 2002:a05:6000:118e:b0:33b:252d:ec26 with SMTP id g14-20020a056000118e00b0033b252dec26mr3577965wrx.65.1707331820699; Wed, 07 Feb 2024 10:50:20 -0800 (PST) MIME-Version: 1.0 References: <20240206010919.1109005-1-lokeshgidra@google.com> <20240206010919.1109005-4-lokeshgidra@google.com> In-Reply-To: From: Lokesh Gidra Date: Wed, 7 Feb 2024 10:50:08 -0800 Message-ID: Subject: Re: [PATCH v3 3/3] userfaultfd: use per-vma locks in userfaultfd operations To: Jann Horn Cc: 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, kaleshsingh@google.com, ngeoffray@google.com, timmurray@google.com, rppt@kernel.org, Liam.Howlett@oracle.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 694DF8001D X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: 6y8gsee3eokkxe7oztfrfc5yfmjk7jht X-HE-Tag: 1707331822-214121 X-HE-Meta: U2FsdGVkX1/7AVy/l2htpxny0sZAuAC9EHTZwHD21WzJb0tvdOZFx6cTwKuIgw1nBWV3ERGyo8C1CshRcWqwjPjGIoTgUbW5C09vz0cHcpXKJQ1ZHvMlnpfSfTSRL4uADdSahtXmUxiKTs0wchzB5/aBlEchrA4QxF0EGXJbfmIPunOifhlyLBBJA08rvN5CAi98HmKkQSgGhrOVZjOV7HSsYCGoTcdqfZudT6Y5nc1+eFfrKhVRn5wxFuONqg9uEtwoVuiiuugIUrX4rDvB2egEaLzcniPkS4ozblrz+wJMP+p2b/IYl2er9eG3PQeaixRBraBi37DJgnnK9O3IvJ5q3OhudvFMAeXN9RdznhSIApH5cMeoPV6GqRssgvq71d4ZTrKAmsVbjlrlGzrVbLRdc0OtjYr2R4qQYynF+MGEjFKw7a9XYtLdH+npnlZ8yjhm9B/0N5Cjx5qbZFnS73EsbB8jJqvSSM6QaHQmTrsu3QYwX/Nvs9e6WcUqPA0u6P1FWcrIYiZ6Jk/yyvYlX5h81YKbDMpHGYeggrD90JdIIlgUmH+ug0CSy66vFmxnJL3lL3u1HTJKbFq2ZoFSIKGh/PUItbzpaH+VExrcqyqCHzu+r2TKyjMi0OC6w4l++MySEZCtqjHLaDO1zLEWSnIShG+5alNiX5j18POMl0gI/tXFIVxG9EOFB9s+Olq6YdqDzT8wuLUruiIpDPdiRUp5/TJnnowlzJ9Ud018iMQvtaJzMaFQEYYTWxkJn5AmJbLLfBM5HBC43rmO46EYZ3NrGhlZpyr0HnyjWhCN6t9hT5ST2x70pn+2M+USHrdoVkAN2bWjuLHy9XuPKgQbHqyoAQ3V3TAwUO/JUJJ0BF6qFlLetMLrJiTxrD8l6pj51uZotEodKYVter3lWVWITQhGiivycBckdFR5060seaLUolX67Z3SuMqdb8nietsqsI2epdWdistyXn6+MR3 oPbLgP5t WzWJ0bP9hBtTFBZHffheB6PdIHdmCjH66rG13ew3Mo47nS32GFJ8EtSWiPl70gxzKKKabO30DwAbGTdmvFtQRcDFol6FjHkxGvZIxRKiua7Y0WxHe5aJVipaY+ejvm3PpyJqV/bOEGVQK5n7vca+bqf5GsRBfPAhof4QYntXjJ2dwYeMr1mZvFYhnc5vTMwG2TeUF0nS671FP9JY9x4Z3GSQ0PySf/NFVPW9PpP54BFo00R9rg88bmBZtKA== 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 Tue, Feb 6, 2024 at 10:28=E2=80=AFAM Jann Horn wrote: > > On Tue, Feb 6, 2024 at 2:09=E2=80=AFAM Lokesh Gidra wrote: > > All userfaultfd operations, except write-protect, opportunistically use > > per-vma locks to lock vmas. On failure, attempt again inside mmap_lock > > critical section. > > > > Write-protect operation requires mmap_lock as it iterates over multiple > > vmas. > > > > Signed-off-by: Lokesh Gidra > [...] > > diff --git a/mm/memory.c b/mm/memory.c > > index b05fd28dbce1..393ab3b0d6f3 100644 > > --- a/mm/memory.c > > +++ b/mm/memory.c > [...] > > +/* > > + * lock_vma() - Lookup and lock VMA corresponding to @address. > > + * @prepare_anon: If true, then prepare the VMA (if anonymous) with an= on_vma. > > + * > > + * Should be called without holding mmap_lock. VMA should be unlocked = after use > > + * with unlock_vma(). > > + * > > + * Return: A locked VMA containing @address, NULL of no VMA is found, = or > > + * -ENOMEM if anon_vma couldn't be allocated. > > + */ > > +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) > > + return vma; > > + > > + mmap_read_lock(mm); > > + vma =3D vma_lookup(mm, address); > > + if (vma) { > > + if (prepare_anon && vma_is_anonymous(vma) && > > + anon_vma_prepare(vma)) > > + vma =3D ERR_PTR(-ENOMEM); > > + else > > + vma_acquire_read_lock(vma); > > This new code only calls anon_vma_prepare() for VMAs where > vma_is_anonymous() is true (meaning they are private anonymous). > > [...] > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > > index 74aad0831e40..64e22e467e4f 100644 > > --- a/mm/userfaultfd.c > > +++ b/mm/userfaultfd.c > > @@ -19,20 +19,25 @@ > > #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) > > +/* Search for VMA and make sure it is valid. */ > > +static struct vm_area_struct *find_and_lock_dst_vma(struct mm_struct *= dst_mm, > > + unsigned long dst_s= tart, > > + unsigned long len) > > { > > - /* > > - * 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; > > + /* Ensure anon_vma is assigned for anonymous vma */ > > + dst_vma =3D lock_vma(dst_mm, dst_start, true); > > lock_vma() is now used by find_and_lock_dst_vma(), which is used by > mfill_atomic(). > > > + if (!dst_vma) > > + return ERR_PTR(-ENOENT); > > + > > + if (PTR_ERR(dst_vma) =3D=3D -ENOMEM) > > + return dst_vma; > > + > > + /* Make sure that the dst range is fully within dst_vma. */ > > + if (dst_start + len > dst_vma->vm_end) > > + goto out_unlock; > > > > /* > > * Check the vma is registered in uffd, this is required to > [...] > > @@ -597,7 +599,15 @@ 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. > > + */ > > + dst_vma =3D find_and_lock_dst_vma(dst_mm, dst_start, len); > > + if (IS_ERR(dst_vma)) { > > + err =3D PTR_ERR(dst_vma); > > + goto out; > > + } > > > > /* > > * If memory mappings are changing because of non-cooperative > > @@ -609,15 +619,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_SH= ARED but > > @@ -647,16 +648,6 @@ static __always_inline ssize_t mfill_atomic(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; > > But the check mfill_atomic() used to do was different, it checked for VM_= SHARED. Thanks so much for catching this. > > Each VMA has one of these three types: > > 1. shared (marked by VM_SHARED; does not have an anon_vma) > 2. private file-backed (needs to have anon_vma when storing PTEs) > 3. private anonymous (what vma_is_anonymous() detects; needs to have > anon_vma when storing PTEs) As in the case of mfill_atomic(), it seems to me that checking for VM_SHARED flag will cover both (2) and (3) right? > > This old code would call anon_vma_prepare() for both private VMA types > (which is correct). The new code only calls anon_vma_prepare() for > private anonymous VMAs, not for private file-backed ones. I think this > code will probably crash with a BUG_ON() in __folio_set_anon() if you > try to use userfaultfd to insert a PTE into a private file-backed VMA > of a shmem file. (Which you should be able to get by creating a file > in /dev/shm/ and then mapping that file with mmap(NULL, , > PROT_READ|PROT_WRITE, MAP_PRIVATE, , 0).)