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 6D550E77188 for ; Fri, 10 Jan 2025 19:08:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DA8808D000D; Fri, 10 Jan 2025 14:07:59 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D563E8D0005; Fri, 10 Jan 2025 14:07:59 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BCF218D000D; Fri, 10 Jan 2025 14:07:59 -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 9ABC18D0005 for ; Fri, 10 Jan 2025 14:07:59 -0500 (EST) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 52D4F120E40 for ; Fri, 10 Jan 2025 19:07:59 +0000 (UTC) X-FDA: 82992477078.03.A63D896 Received: from mail-qt1-f180.google.com (mail-qt1-f180.google.com [209.85.160.180]) by imf27.hostedemail.com (Postfix) with ESMTP id 8332A4000F for ; Fri, 10 Jan 2025 19:07:57 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=l2Hg8LYQ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf27.hostedemail.com: domain of surenb@google.com designates 209.85.160.180 as permitted sender) smtp.mailfrom=surenb@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736536077; a=rsa-sha256; cv=none; b=rqAGw5kZnsbC6SyRKWczPkOwru3L4HlMhKC0PgcychyRyklgCCz+tCJ7zVYllFNnVexN1Q V2Aj4Xue/Kf1k4AKA//6Y+ky48YZuQeKfrckxGB+H9SdiR4oVKVHzS99TgPGccQeh6QUUg wnV5GLVIgAn7b7G51GHp0OL1p2DfY0I= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=l2Hg8LYQ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf27.hostedemail.com: domain of surenb@google.com designates 209.85.160.180 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=1736536077; 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=qTi2nvSv0ABbvAbGtBJHojiB4NNIn2oSG/eUDgHwzqw=; b=hZwkDTxZcHLTuXlhFN/KPX4b0AT60oECQPBkK66hLllbCUbROddl3kWq2SEMCQdP6tl6G1 G2FgUcl8IyjqtmVyEmDvcemZMBRs6U/MF36Eh9SvQ80Msju9MaaOBoEDO+7t+0/9pnm2JC FjsW92Hc1AK8kkfTi7P5u219NsFcz9Q= Received: by mail-qt1-f180.google.com with SMTP id d75a77b69052e-4678c9310afso22861cf.1 for ; Fri, 10 Jan 2025 11:07:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1736536076; x=1737140876; 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=qTi2nvSv0ABbvAbGtBJHojiB4NNIn2oSG/eUDgHwzqw=; b=l2Hg8LYQWXpSitxHNjQICKuq9WUEERgap8q86dVR62FP+X3aD8jaLkBzQGDlWn+Ed8 adPEIfnJfku952fts8K8aFIoxoHe8N/WT49NnqsnXq0SjU4aPlBnnm3WK/nkdyICLWfZ wamgeeMaPq8+BEK47ki9qkqEJ6y9nF/GBRBkrAfOncVZtocUZ6TcxIUpkavd6/pmpPFy 47SqOzLL5MO+GKwEA2Qg+yUaVWjpRtRYSbQkErKrR52VA/VO74U6O7T2rNWDOKMIZj6a 8fk2ovWI0utkWtJX7Ne6Y7JdVH0WvpCabEVPhXIfiEWUIVRc4ooxs/xG2IPlQqQ+SbPk AwrA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736536076; x=1737140876; 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=qTi2nvSv0ABbvAbGtBJHojiB4NNIn2oSG/eUDgHwzqw=; b=c8Seve820Nu5recWLCNijk9OUDSLl7ulNUOwhQj7O7d9LUEa25L1Wc44OEHgEU1WA1 dSAMGwxEhFJd5gqTIftL6PYvvZTokT2GzeK+eErZj/Chp0WOWPukL0fqtiWEyuiiL5ru AqS95e4NRKpJa3Y4gY5wEWLT7VCKDbLPCoNL6WWDj69m/ONUPyMjE+5JTsbgkVcidYlS JACXYAkhym4A6l6ZzJMpGNtOLaQ3ohGxWTsVnWqCtViod5Oy+RQWk1YPBiEuU1zV5W8Z oSHOIS+EKVz5pUh8dmVwFdJfCV97+ELcmKXTEr+W7pRkQuOc6eoD7z4bRfGOmQMOCljz p2KQ== X-Forwarded-Encrypted: i=1; AJvYcCWpLkodok82Ys1h2RkBbB3Zd98Dd84SOAhU7KYSMvp/168ENgudayyKaFIEYsv2Zkrm9X92ioErbw==@kvack.org X-Gm-Message-State: AOJu0Yyt4rjiomhy+edXs6ECx+RJvwo4J+1rC/0r/mAaqU8pazAba12a oBOM0kqrpFUtKmb3X3Djmg/z5R9VFEJ3BK09ePjLqqe60TRV1IFsBpMeZJaXkfic4CmDfEfswi1 elPcgovIbZM0wc8U6jO5qvBC2pV6GX2I2L7bd X-Gm-Gg: ASbGncsH0uX+P2UO7unfhMYhzPWyW4P9+qmJg9P7UedjxRiWMARTeUi+DcPCUQqYCbw WWr3Dl4iqzMFmhdqB1N3i6WrUt7VcmHV+P9+WBg== X-Google-Smtp-Source: AGHT+IEoXAcMVmHCXCyc8rjGNXE1ti4IyklXI+uMV2SwUHXGaGWMs4W9yqmVxDUBR+OVWVJ548l1xVR/OQhXEz92QFM= X-Received: by 2002:a05:622a:2c1:b0:460:491e:d2a2 with SMTP id d75a77b69052e-46c89dad5a8mr3557021cf.17.1736536076139; Fri, 10 Jan 2025 11:07:56 -0800 (PST) MIME-Version: 1.0 References: <20250109023025.2242447-1-surenb@google.com> <20250109023025.2242447-16-surenb@google.com> In-Reply-To: From: Suren Baghdasaryan Date: Fri, 10 Jan 2025 11:07:44 -0800 X-Gm-Features: AbW1kvYAAyQ3QGj7QebBVjDM6a0dnRe16Ld1SPpTzou8WRN4-K0wMA_sYKPRcJg Message-ID: Subject: Re: [PATCH v8 15/16] mm: make vma cache SLAB_TYPESAFE_BY_RCU To: "Liam R. Howlett" , Suren Baghdasaryan , akpm@linux-foundation.org, peterz@infradead.org, willy@infradead.org, lorenzo.stoakes@oracle.com, mhocko@suse.com, vbabka@suse.cz, hannes@cmpxchg.org, mjguzik@gmail.com, oliver.sang@intel.com, mgorman@techsingularity.net, david@redhat.com, peterx@redhat.com, oleg@redhat.com, dave@stgolabs.net, paulmck@kernel.org, brauner@kernel.org, dhowells@redhat.com, hdanton@sina.com, hughd@google.com, lokeshgidra@google.com, minchan@google.com, jannh@google.com, shakeel.butt@linux.dev, souravpanda@google.com, pasha.tatashin@soleen.com, klarasmodin@gmail.com, richard.weiyang@gmail.com, corbet@lwn.net, linux-doc@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-team@android.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 8332A4000F X-Stat-Signature: 7qdynyqyc5cpqbj9kr9dotn4ugr4zzrp X-HE-Tag: 1736536077-994281 X-HE-Meta: U2FsdGVkX1+zQl8rCQxErX7tcoCsIUksLPr8sOPfo0Mxpy3QJpyxEPNsgVtXyAN7k4k56gEIaqt7KLJfkyn18oaak56IZHk+OnMcIxcG4g37ztPPiVN/BWRoX3CCn4aRz/kiADGBnqg0GOHFF14VpwtVJ241ZWo5KE7Blq0R3D5ZZNMh0MxF1/YbrXkRY67I/PeVs9vOUv08pY9CaVnSq8C7BTF7SIRf+JEiFpMxt049F6hJax4YO9MrTVjurEZXfGyAgGvMp3wV44ltcgjFCNTqNN89Fp2gg8d2xZfyRotdoDMvjCTQeMOxmY22IwOh3EnQdCEkl6HhT7wN5CA8Y3AuoVTIBbfKIVHq2hMxCfGmm8ziNyibYehz72N8LAF6o4SqQ2lkGDLbpXrMNeyS0q9hmJ1xTce5OuL9RMSD2LEcVWxJS3wHOqkYurdXcUCS/tHLNcC+e9IOr1B6ABH/aRwkJ+ZuJHv+wOff82nh9UIj1q9d2lz4ORVF1zvnLsm8Hu82AVREiG9t67YP+8Z5IoRmjwoRQmMYDKSUF8tdmA2jVPuKusTMbeOYkQpP5j7UAOhaNutuMmB5r+0kilUPEUcFAuHX1AP/a0MiLuSgrCGlbUABmPGWH6LulfVr2sfHrGUqlrYRsqA+vGdvcVg/QhXPJla31yGI0iQVNJj8n468lxsKB7yW0MBpf6NF4wNVEc7Lx9w+Cb/LxHZYRtc6OgLXu2JYEWy9r1xtg+u7Pl7Di2KSy4G5QNGUB9Pxl35l6qvn/rzvXohHZNZMPJ5otxGiPOSvCEGorDlNcwKQBSd6ois8bnfbwjvk8ANvi9cZWFytw86ftNPS4qBp2tcWbElI9WdfgTHfTSRLTz/PtZUXZUIFi5g/kohRLPwF0Z4HmzoCY7sLbPj7mmb0FNAvn5I6SIR0XcV22K66f8W2zIOb4krs29S64N+2D42QRI4WiL2pJPZ873rYP4L61mf g0gXZ31m /XYapYqWzVkfcGLmVbAT3NltVoTR487K3/lJif8ilJrW7CjS2DlWEqre1QV4UETj9ERI4ag/bNAAG9RyN3clw5vxxYHrztmaHYwp3zN85KUCA709ww38c9ZuVGloqWK4Ra/9XfGHDNSWhx7KeBNodkfjvuxJE39Q8MoFodRDgI1z2ulf7R7ajQXxE/BOsgDAahguIOf90pYkAl/A5oBF/8ijp6Omlt3y5zNYnJEgjDCCLqClXPMclJyeIckZ1dj1jtT47AAW1qxI5hL3TDM04AakvoxrUXTAjoncyUDC5fpW9TrIjbqPKsruxHigbWteu/fJF8AedvXXZynm8gru+/a5/O/CSHPmz0Yf2MMIRgzSiHQk= 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, Jan 10, 2025 at 9:48=E2=80=AFAM Liam R. Howlett wrote: > > * Suren Baghdasaryan [250108 21:31]: > > To enable SLAB_TYPESAFE_BY_RCU for vma cache we need to ensure that > > object reuse before RCU grace period is over will be detected by > > lock_vma_under_rcu(). > > Current checks are sufficient as long as vma is detached before it is > > freed. The only place this is not currently happening is in exit_mmap()= . > > Add the missing vma_mark_detached() in exit_mmap(). > > Another issue which might trick lock_vma_under_rcu() during vma reuse > > is vm_area_dup(), which copies the entire content of the vma into a new > > one, overriding new vma's vm_refcnt and temporarily making it appear as > > attached. This might trick a racing lock_vma_under_rcu() to operate on > > a reused vma if it found the vma before it got reused. To prevent this > > situation, we should ensure that vm_refcnt stays at detached state (0) > > when it is copied and advances to attached state only after it is added > > into the vma tree. Introduce vma_copy() which preserves new vma's > > vm_refcnt and use it in vm_area_dup(). Since all vmas are in detached > > state with no current readers when they are freed, lock_vma_under_rcu() > > will not be able to take vm_refcnt after vma got detached even if vma > > is reused. > > Finally, make vm_area_cachep SLAB_TYPESAFE_BY_RCU. This will facilitate > > vm_area_struct reuse and will minimize the number of call_rcu() calls. > > > > Signed-off-by: Suren Baghdasaryan > > --- > > include/linux/mm.h | 2 - > > include/linux/mm_types.h | 10 +++-- > > include/linux/slab.h | 6 --- > > kernel/fork.c | 72 ++++++++++++++++++++------------ > > mm/mmap.c | 3 +- > > mm/vma.c | 11 ++--- > > mm/vma.h | 2 +- > > tools/testing/vma/vma_internal.h | 7 +--- > > 8 files changed, 59 insertions(+), 54 deletions(-) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index 1d6b1563b956..a674558e4c05 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -258,8 +258,6 @@ void setup_initial_init_mm(void *start_code, void *= end_code, > > struct vm_area_struct *vm_area_alloc(struct mm_struct *); > > struct vm_area_struct *vm_area_dup(struct vm_area_struct *); > > void vm_area_free(struct vm_area_struct *); > > -/* Use only if VMA has no other users */ > > -void __vm_area_free(struct vm_area_struct *vma); > > > > #ifndef CONFIG_MMU > > extern struct rb_root nommu_region_tree; > > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > > index 2d83d79d1899..93bfcd0c1fde 100644 > > --- a/include/linux/mm_types.h > > +++ b/include/linux/mm_types.h > > @@ -582,6 +582,12 @@ static inline void *folio_get_private(struct folio= *folio) > > > > typedef unsigned long vm_flags_t; > > > > +/* > > + * freeptr_t represents a SLUB freelist pointer, which might be encode= d > > + * and not dereferenceable if CONFIG_SLAB_FREELIST_HARDENED is enabled= . > > + */ > > +typedef struct { unsigned long v; } freeptr_t; > > + > > /* > > * A region containing a mapping of a non-memory backed file under NOM= MU > > * conditions. These are held in a global tree and are pinned by the = VMAs that > > @@ -695,9 +701,7 @@ struct vm_area_struct { > > unsigned long vm_start; > > unsigned long vm_end; > > }; > > -#ifdef CONFIG_PER_VMA_LOCK > > - struct rcu_head vm_rcu; /* Used for deferred freeing. */ > > -#endif > > + freeptr_t vm_freeptr; /* Pointer used by SLAB_TYPESAFE_BY= _RCU */ > > }; > > > > /* > > diff --git a/include/linux/slab.h b/include/linux/slab.h > > index 10a971c2bde3..681b685b6c4e 100644 > > --- a/include/linux/slab.h > > +++ b/include/linux/slab.h > > @@ -234,12 +234,6 @@ enum _slab_flag_bits { > > #define SLAB_NO_OBJ_EXT __SLAB_FLAG_UNUSED > > #endif > > > > -/* > > - * freeptr_t represents a SLUB freelist pointer, which might be encode= d > > - * and not dereferenceable if CONFIG_SLAB_FREELIST_HARDENED is enabled= . > > - */ > > -typedef struct { unsigned long v; } freeptr_t; > > - > > /* > > * ZERO_SIZE_PTR will be returned for zero sized kmalloc requests. > > * > > diff --git a/kernel/fork.c b/kernel/fork.c > > index 9d9275783cf8..770b973a099c 100644 > > --- a/kernel/fork.c > > +++ b/kernel/fork.c > > @@ -449,6 +449,41 @@ struct vm_area_struct *vm_area_alloc(struct mm_str= uct *mm) > > return vma; > > } > > > > There exists a copy_vma() which copies the vma to a new area in the mm > in rmap. Naming this vma_copy() is confusing :) > > It might be better to just put this code in the vm_area_dup() or call it > __vm_area_dup(), or __vma_dup() ? Hmm. It's not really duplicating a vma but copying its content (no allocation). How about __vm_area_copy() to indicate it is copying vm_area_struct content? > > > +static void vma_copy(const struct vm_area_struct *src, struct vm_area_= struct *dest) > > +{ > > + dest->vm_mm =3D src->vm_mm; > > + dest->vm_ops =3D src->vm_ops; > > + dest->vm_start =3D src->vm_start; > > + dest->vm_end =3D src->vm_end; > > + dest->anon_vma =3D src->anon_vma; > > + dest->vm_pgoff =3D src->vm_pgoff; > > + dest->vm_file =3D src->vm_file; > > + dest->vm_private_data =3D src->vm_private_data; > > + vm_flags_init(dest, src->vm_flags); > > + memcpy(&dest->vm_page_prot, &src->vm_page_prot, > > + sizeof(dest->vm_page_prot)); > > + /* > > + * src->shared.rb may be modified concurrently, but the clone > > + * will be reinitialized. > > + */ > > + data_race(memcpy(&dest->shared, &src->shared, sizeof(dest->shared= ))); > > + memcpy(&dest->vm_userfaultfd_ctx, &src->vm_userfaultfd_ctx, > > + sizeof(dest->vm_userfaultfd_ctx)); > > +#ifdef CONFIG_ANON_VMA_NAME > > + dest->anon_name =3D src->anon_name; > > +#endif > > +#ifdef CONFIG_SWAP > > + memcpy(&dest->swap_readahead_info, &src->swap_readahead_info, > > + sizeof(dest->swap_readahead_info)); > > +#endif > > +#ifndef CONFIG_MMU > > + dest->vm_region =3D src->vm_region; > > +#endif > > +#ifdef CONFIG_NUMA > > + dest->vm_policy =3D src->vm_policy; > > +#endif > > +} > > + > > struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig) > > { > > struct vm_area_struct *new =3D kmem_cache_alloc(vm_area_cachep, G= FP_KERNEL); > > @@ -458,11 +493,7 @@ struct vm_area_struct *vm_area_dup(struct vm_area_= struct *orig) > > > > ASSERT_EXCLUSIVE_WRITER(orig->vm_flags); > > ASSERT_EXCLUSIVE_WRITER(orig->vm_file); > > - /* > > - * orig->shared.rb may be modified concurrently, but the clone > > - * will be reinitialized. > > - */ > > - data_race(memcpy(new, orig, sizeof(*new))); > > + vma_copy(orig, new); > > vma_lock_init(new, true); > > I think this suffers from a race still? > > That is, we can still race between vm_lock_seq =3D=3D mm_lock_seq and the > lock acquire, where a free and reuse happens. In the even that the > reader is caught between the sequence and lock taking, the > vma->vmlock_dep_map may not be replaced and it could see the old lock > (or zero?) and things go bad: > > It could try to take vmlock_dep_map =3D=3D 0 in read mode. > > It can take the old lock, detect the refcnt is wrong and release the new > lock. I don't think this race can happen. Notice a call to vma_assert_detached() inside vm_area_free(), so before vma is freed and possibly reused, it has to be detached. vma_mark_detached() ensures that there are no current or future readers by executing the __vma_enter_locked() + __vma_exit_locked() sequence if vm_refcnt is not already at 0. Once __vma_exit_locked() is done, vm_refcnt is at 0 and any new reader will be rejected on __refcount_inc_not_zero_limited(), before even checking vm_lock_seq =3D=3D mm_lock_seq. Even if a reader tries to sneak in between __vma_enter_locked() and __vma_exit_locked() calls, __refcount_inc_not_zero_limited() will reject it because VMA_LOCK_OFFSET is set and VMA_REF_LIMIT will be violated. IOW, when VMA is freed, it's guaranteed to be detached with no current or future readers, therefore "race between vm_lock_seq =3D=3D mm_lock_seq and the lock acquire, where a free and reuse happens" should not be possible. Did I understand your concern correctly and does my explanation make sense to you? > > Thanks, > Liam