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 31BC6E77180 for ; Wed, 11 Dec 2024 16:14:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id BBD426B0089; Wed, 11 Dec 2024 11:14:20 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B6CA96B008A; Wed, 11 Dec 2024 11:14:20 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A0CEA6B0095; Wed, 11 Dec 2024 11:14:20 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 837BC6B0089 for ; Wed, 11 Dec 2024 11:14:20 -0500 (EST) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 2BC051A128E for ; Wed, 11 Dec 2024 16:14:20 +0000 (UTC) X-FDA: 82883173800.20.74AA73F Received: from mail-qt1-f180.google.com (mail-qt1-f180.google.com [209.85.160.180]) by imf13.hostedemail.com (Postfix) with ESMTP id 2921A20013 for ; Wed, 11 Dec 2024 16:13:54 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=aAKuBxqa; spf=pass (imf13.hostedemail.com: domain of surenb@google.com designates 209.85.160.180 as permitted sender) smtp.mailfrom=surenb@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=1733933633; 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=phElHLYxkNBYJyvyG4lECOcpRVx5oOVRU61JUuf+v3s=; b=qpYrWdvKUaJaB5isz4HXPKWSwesGOSs7fLFhGbarrR4a28tlghKvy7+59oq6uVLHLuW8zI eDZlDjJNtNvkUmskHitzJWJPsOJ0G6osgNsGQ2ZbEUuscuNABwFTPjxXaIKiegw7EQ1fAY 5N59OsasRHXnGCqlHLUrsZY+ERByGWY= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=aAKuBxqa; spf=pass (imf13.hostedemail.com: domain of surenb@google.com designates 209.85.160.180 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1733933633; a=rsa-sha256; cv=none; b=Lw7lE7oJ6IaSqA0L5dSgFl8kGuZ/d674cTynSVo8I9PgSG2otSEnEou4TpAH5aDifen2rl SWQ1TBuy4Rg+Txeb3U7M/F+4y8ks8cJdLxMtky8wGxa/Ohk86HIZc7HoorD9uoW/zG4IER 06LPTO3HQKdE7dr9XBhLNfC3HRF5KOs= Received: by mail-qt1-f180.google.com with SMTP id d75a77b69052e-4678c9310afso208221cf.1 for ; Wed, 11 Dec 2024 08:14:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1733933657; x=1734538457; 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=phElHLYxkNBYJyvyG4lECOcpRVx5oOVRU61JUuf+v3s=; b=aAKuBxqagYGUEGTar6gGLfF8BdBBqFeqola63Qw9mOuXdUZ7didMPWvSUXSytDNI2Q 8JWbV6EaKMvHA3CAMAGW3VArQknZdgUCmnwgVrjTk2CetyMcozPKSQRjC6HLnowSzFj4 nApWebiSk8dJzXO/oAz5TW22LstX+F1wSZS3wiaWYThqXRV93NVHMSXTnJFDZdGFV5Ta yE1SRo8tsK82BrWKJq8iVWxDMyL07gCskJggFhm3xlpspGAsIclt1HDR0OU0jgPhB9yE kKTtcQ9he+OOi8YI0CaLojjT999XInVIuLBAKVv03wRhAxs4P07fIr04Encd2/yoI/ss 0gUA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1733933657; x=1734538457; 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=phElHLYxkNBYJyvyG4lECOcpRVx5oOVRU61JUuf+v3s=; b=bW+LL7H542WJyTSWGKytyyUPIBFiMz0f/HDB7ObRQ9MkoXEHrsP2D+I0uVntRpLgDC qpVUtNIvFkdW5qqiam5doMdhTargegMiRskid9N7NErjtZw5oK/tRtWoyoxHwy54LmLL 5deUFudjRJrcald1axx7y9uaPel3ajydiT/LrzKY/hBz9GfltzAZZQtZRqX4DiI31nGd tY5LAnubXEDYY+crEMcHCJd7IYOw5ndmmmRbgg5Vtb366usvTxooMF60uispmrImlNmT tuhP3GysjJ7Pv+ado35DKJRWRBu2olZbJlV+XbPUO/GktmMaKpVXwAqfC4p2t6Bfb/61 NA4Q== X-Forwarded-Encrypted: i=1; AJvYcCXRLDEe0FOhbPVc/iUHTJVR5xLp90LdfN3TgkSBNUwxujbKcLIJLdO4cFRiNWhcNqHRJ5Ymkj2D5A==@kvack.org X-Gm-Message-State: AOJu0Yzfn3rQtoaCK5PkDrlCfLPoZTqYegbGaWvKq2xPl/ZLB9lNAQDR 5PmjT6Zu1owjOa5QZdheEM+o4xLqeWqDsz3naMkHdIJxTg7b+27ZQR8DY+8gYIVfCxkuBt9Zw4j LGc4xwEJDFx2jeo1Uqv84Nz4nQy6HQBrJFWi3 X-Gm-Gg: ASbGnctQv5Ahhc3X4HpnQBbVh38ENCMdVUjenqrTbtCfvIrWBRfu3QJnhjDkFGGuB4P xuol7ZbhEoCJPsqTRapcVR661xiNAKWppBML9of3riT2SpMkdWlv71ooJWw1GbAEG0w== X-Google-Smtp-Source: AGHT+IGFzjfV8u0+PhQ9um56OnOqrfs5vhnKjf9zf+yXxmNnp4EslbB/FiQXC1FySPFT5Ic17QmsFwH+OtK/qb2/rpo= X-Received: by 2002:a05:622a:256:b0:466:a3bf:41ab with SMTP id d75a77b69052e-46795e6ec86mr12811cf.21.1733933657220; Wed, 11 Dec 2024 08:14:17 -0800 (PST) MIME-Version: 1.0 References: <20241206225204.4008261-1-surenb@google.com> <20241206225204.4008261-5-surenb@google.com> <6b29b2a5-c244-4930-a5a0-1a24a04e7e35@suse.cz> <643beb6c-4226-46ca-b7e9-292467479aea@suse.cz> <5036d089-0774-4863-88c5-eaaea1265ac7@suse.cz> <253a0af3-6751-413c-ad37-a5d13dd9b8f0@suse.cz> In-Reply-To: <253a0af3-6751-413c-ad37-a5d13dd9b8f0@suse.cz> From: Suren Baghdasaryan Date: Wed, 11 Dec 2024 08:14:05 -0800 Message-ID: Subject: Re: [PATCH v5 4/6] mm: make vma cache SLAB_TYPESAFE_BY_RCU To: Vlastimil Babka Cc: akpm@linux-foundation.org, willy@infradead.org, liam.howlett@oracle.com, lorenzo.stoakes@oracle.com, mhocko@suse.com, 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, minchan@google.com, jannh@google.com, shakeel.butt@linux.dev, souravpanda@google.com, pasha.tatashin@soleen.com, corbet@lwn.net, linux-doc@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-team@android.com, Peter Zijlstra Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 2921A20013 X-Rspam-User: X-Stat-Signature: 96e3n4g3nim1haf1b67d7qyzknm37198 X-HE-Tag: 1733933634-977806 X-HE-Meta: U2FsdGVkX1/1YzBRWNp+N4s2YSsX/SBpINgg8O4b9iEbzAL/GO958L/tmn1zQ6ZSQBZTXE6dkBbLF1YTYR1M+WYevLsLMUHznS0qhvUv/DDmTeFwkYh3fZVS/SXKGu5ZJ49EZ02g4uWnM7PyT5E6gJxkDg1jDW5cVaTGXUvF+qfvD9sJLaNLrErCsrSlkv7+lb+9LGN1Ukubse80HjxoL5LKOcfP85GroOFulqDDQH69vf7XBMKqDc9yINJZPybAWpi0YzF4wkVGOyiHxkgEIHxfNWfcP3MlxdpIXZkOb6yUtz7/FSeWQIS1e6u91YhdzlYJRrP3enAogWDxhT3Q12ph7lCiN3qbOr48j/USaqpxVQwGHVRnXDeKAI7F1QZDwxMRxFaLi2htlOAC4OaclO7iprRstIh2AzPCALxSN0VV2v5DawCwaPcdPq+XhP/UPWan6ta960pt6N2QnnJoV+fvv7pEZRjN9+iYitJ7zWBijEpVZUhxalqMYg5J2m9uWJXTsuw8DpexOcC2j3ZSkkf8D2hcvcySTCcT4enXgFV2046V8EQw/Oc+dyR0I7gR06VfKPqSObz5YMGgHZWEy48euh12lrGAA+PuNEWe0YGXUqmyzfKIdjPu3yfdwYyE0u42dSa1LRx7gR74a9T6PFP9BniIB+EqFV4hGqTQQ+LIGzqk8cEbf4d/0aFKb6hu6WA49+BkDVHepZbjfjbShYluFVmKM4rdUwCzAtF/E4JtDaK3rBpmqp+NS4LUhOMHEt8wpretB2BDzSwtP1Qf0Z6opKgLNa/BMeEa8OGIED8nqOv3ThNOE0XrDNuON1QMtxahDJOJwEGkcqsX5UOASAC+URT+sAToeVBNOXUCHnkxcjTsEjV/gcPkEfPHbFPC5LBiHZx9qJknXxsOyiL0H4W4dEKLqrvcwaAjChlzPNp3Dy3TpvWQ34NxJWPsUtXl3JU/wK0uuNd0461lUl1 hiUKSenE 0jz7PbmlihA215q3apTmjOahxScPtDO+d43kUDMF+icn8bdxUMnP1eR0Uu0FrV3GCSshZB+gHwwuo+rjcMC8XTK3gYx1+IfqDmRUYqiGPnbI43jGK9jXtoCRW8mpbFt1oEMDrsfAKWE0rly6qN7eE0RsN6l6Np+bXLrhQrxUXpjQqBwPa4kP4O1vsphSWGR182L0p5p/h+Sm7dPNh6AhjnCFmUZrExy8UtUZ36G07LYuEvJveg7MCPuZGkpOO98ZVtmfQlrmrUzKy+xk= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000154, 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 Wed, Dec 11, 2024 at 8:05=E2=80=AFAM Vlastimil Babka wr= ote: > > On 12/11/24 16:30, Suren Baghdasaryan wrote: > > On Tue, Dec 10, 2024 at 3:01=E2=80=AFPM Suren Baghdasaryan wrote: > >> > >> On Tue, Dec 10, 2024 at 9:25=E2=80=AFAM Vlastimil Babka wrote: > >> > > >> > On 12/10/24 18:16, Suren Baghdasaryan wrote: > >> > > On Tue, Dec 10, 2024 at 8:32=E2=80=AFAM Vlastimil Babka wrote: > >> > >> > >> > >> On 12/10/24 17:20, Suren Baghdasaryan wrote: > >> > >> > On Tue, Dec 10, 2024 at 6:21=E2=80=AFAM Vlastimil Babka wrote: > >> > >> >> > >> > >> >> On 12/6/24 23:52, Suren Baghdasaryan wrote: > >> > >> >> > To enable SLAB_TYPESAFE_BY_RCU for vma cache we need to ensu= re that > >> > >> >> > object reuse before RCU grace period is over will be detecte= d inside > >> > >> >> > lock_vma_under_rcu(). > >> > >> >> > lock_vma_under_rcu() enters RCU read section, finds the vma = at the > >> > >> >> > given address, locks the vma and checks if it got detached o= r remapped > >> > >> >> > to cover a different address range. These last checks are th= ere > >> > >> >> > to ensure that the vma was not modified after we found it bu= t before > >> > >> >> > locking it. > >> > >> >> > vma reuse introduces several new possibilities: > >> > >> >> > 1. vma can be reused after it was found but before it is loc= ked; > >> > >> >> > 2. vma can be reused and reinitialized (including changing i= ts vm_mm) > >> > >> >> > while being locked in vma_start_read(); > >> > >> >> > 3. vma can be reused and reinitialized after it was found bu= t before > >> > >> >> > it is locked, then attached at a new address or to a new mm = while > >> > >> >> > read-locked; > >> > >> >> > For case #1 current checks will help detecting cases when: > >> > >> >> > - vma was reused but not yet added into the tree (detached c= heck) > >> > >> >> > - vma was reused at a different address range (address check= ); > >> > >> >> > We are missing the check for vm_mm to ensure the reused vma = was not > >> > >> >> > attached to a different mm. This patch adds the missing chec= k. > >> > >> >> > For case #2, we pass mm to vma_start_read() to prevent acces= s to > >> > >> >> > unstable vma->vm_mm. This might lead to vma_start_read() ret= urning > >> > >> >> > a false locked result but that's not critical if it's rare b= ecause > >> > >> >> > it will only lead to a retry under mmap_lock. > >> > >> >> > For case #3, we ensure the order in which vma->detached flag= and > >> > >> >> > vm_start/vm_end/vm_mm are set and checked. vma gets attached= after > >> > >> >> > vm_start/vm_end/vm_mm were set and lock_vma_under_rcu() shou= ld check > >> > >> >> > vma->detached before checking vm_start/vm_end/vm_mm. This is= required > >> > >> >> > because attaching vma happens without vma write-lock, as opp= osed to > >> > >> >> > vma detaching, which requires vma write-lock. This patch add= s memory > >> > >> >> > barriers inside is_vma_detached() and vma_mark_attached() ne= eded to > >> > >> >> > order reads and writes to vma->detached vs vm_start/vm_end/v= m_mm. > >> > >> >> > After these provisions, SLAB_TYPESAFE_BY_RCU is added to vm_= area_cachep. > >> > >> >> > This will facilitate vm_area_struct reuse and will minimize = the number > >> > >> >> > of call_rcu() calls. > >> > >> >> > > >> > >> >> > Signed-off-by: Suren Baghdasaryan > >> > >> >> > >> > >> >> I'm wondering about the vma freeing path. Consider vma_complet= e(): > >> > >> >> > >> > >> >> vma_mark_detached(vp->remove); > >> > >> >> vma->detached =3D true; - plain write > >> > >> >> vm_area_free(vp->remove); > >> > >> >> vma->vm_lock_seq =3D UINT_MAX; - plain write > >> > >> >> kmem_cache_free(vm_area_cachep) > >> > >> >> ... > >> > >> >> potential reallocation > >> > >> >> > >> > >> >> against: > >> > >> >> > >> > >> >> lock_vma_under_rcu() > >> > >> >> - mas_walk finds a stale vma due to race > >> > >> >> vma_start_read() > >> > >> >> if (READ_ONCE(vma->vm_lock_seq) =3D=3D READ_ONCE(mm->mm_lock= _seq.sequence)) > >> > >> >> - can be false, the vma was not being locked on the freeing = side? > >> > >> >> down_read_trylock(&vma->vm_lock.lock) - suceeds, wasn't lock= ed > >> > >> >> this is acquire, but was there any release? > >> > >> > > >> > >> > Yes, there was a release. I think what you missed is that > >> > >> > vma_mark_detached() that is called from vma_complete() requires= VMA to > >> > >> > be write-locked (see vma_assert_write_locked() in > >> > >> > vma_mark_detached()). The rule is that a VMA can be attached wi= thout > >> > >> > write-locking but only a write-locked VMA can be detached. So, = after > >> > >> > >> > >> OK but write unlocking means the mm's seqcount is bumped and beco= mes > >> > >> non-equal with vma's vma->vm_lock_seq, right? > >> > >> > >> > >> Yet in the example above we happily set it to UINT_MAX and thus e= ffectively > >> > >> false unlock it for vma_start_read()? > >> > >> > >> > >> And this is all done before the vma_complete() side would actuall= y reach > >> > >> mmap_write_unlock(), AFAICS. > >> > > > >> > > Ah, you are right. With the possibility of reuse, even a freed VMA > >> > > should be kept write-locked until it is unlocked by > >> > > mmap_write_unlock(). I think the fix for this is simply to not res= et > >> > > vma->vm_lock_seq inside vm_area_free(). I'll also need to add a > >> > > >> > But even if we don't reset vm_lock_seq to UINT_MAX, then whover real= located > >> > it can proceed and end up doing a vma_start_write() and rewrite it t= here > >> > anyway, no? > >> > >> Actually, I think with a small change we can simplify these locking ru= les: > >> > >> static inline void vma_start_write(struct vm_area_struct *vma) > >> { > >> int mm_lock_seq; > >> > >> - if (__is_vma_write_locked(vma, &mm_lock_seq)) > >> - return; > >> + mmap_assert_write_locked(vma->vm_mm); > >> + mm_lock_seq =3D vma->vm_mm->mm_lock_seq; > >> > >> down_write(&vma->vm_lock->lock); > >> /* > >> * We should use WRITE_ONCE() here because we can have concurre= nt reads > >> * from the early lockless pessimistic check in vma_start_read(= ). > >> * We don't really care about the correctness of that early che= ck, but > >> * we should use WRITE_ONCE() for cleanliness and to keep KCSAN= happy. > >> */ > >> WRITE_ONCE(vma->vm_lock_seq, mm_lock_seq); > >> up_write(&vma->vm_lock->lock); > >> } > >> > >> This will force vma_start_write() to always write-lock vma->vm_lock > >> before changing vma->vm_lock_seq. Since vma->vm_lock survives reuse, > >> the other readers/writers will synchronize on it even if vma got > >> reused. > > > > After thinking of all the alternatives, I think the cleanest way to > > handle vma detaching would be to follow the same pattern as for vma > > attaching. To attach a vma we do: > > > > vma->vm_mm =3D xxx; > > ... > > vma_mark_attached() > > smp_wmb(); > > WRITE_ONCE(vma->detached, false); > > > > > > lock_vma_under_rcu() ensures that a vma is attached and still > > unchanged like this: > > > > lock_vma_under_rcu() > > vma_start_read(); > > is_vma_detached() > > detached =3D READ_ONCE(vma->detached); > > smp_rmb(); > > if (vma->vm_mm !=3D mm) > > > > So, vm_area_free() can follow the same pattern to ensure vma reuse > > gets detected even if lock_vma_under_rcu() succeeds in locking the > > vma: > > > > vm_area_free() > > vma->vm_mm =3D NULL; > > smp_wmb(); > > WRITE_ONCE(vma->detached, true); > > > > Vlastimil, I think that should address the race you described. WDYT? > > I'm not sure. AFAIU the barriers would ensure that if lock_vma_under_rcu(= ) > sees detached, it also sees vm_mm is NULL. But as it doesn't ensure that = it > will see it detached, so it also doesn't ensure we will see vm_mm as NULL= . > > I think the main problem is that we unlock the vma by writing to a mm, no= t > the vma, which makes it hard to apply the necessary SLAB_TYPESAFE_BY_RCU > validation patterns to it. I thought the direction you were discussing wi= th > PeterZ in the other thread would solve this (in addition of getting rid o= f > the rwsem, which we were considering it anyway, but enabling > SLAB_TYPESAFE_BY_RCU by that would be a stronger argument). I was hoping to implement SLAB_TYPESAFE_BY_RCU independently from vm_lock change but you are probably right. Incorporating vma->detached flag into the lock itself (which survives reuse) would make things way easier. Let me pivot towards making that change first and see if SLAB_TYPESAFE_BY_RCU becomes simpler. > > Perhaps a solution to this that would work with the current rwsem would b= e > that setting detached and vm_mm to NULL would be set under the down_write= () > of the rwsem. That would make sure that if lock_vma_under_rcu() succeeds = the > down_read_trylock(), it would be guaranteed to see those assignments? Yeah, that would definitely work. I was trying to avoid extra locking but it looks like it's unavoidable. Anyway, let me try replacing vm_lock first and will see where we end up. Thanks for the input! > > >> > >> > > >> > > comment for vm_lock_seq explaining these requirements. > >> > > Do you agree that such a change would resolve the issue? > >> > > > >> > >> > >> > >> > vma_mark_detached() and before down_read_trylock(&vma->vm_lock.= lock) > >> > >> > in vma_start_read() the VMA write-lock should have been release= d by > >> > >> > mmap_write_unlock() and therefore vma->detached=3Dfalse should = be > >> > >> > visible to the reader when it executed lock_vma_under_rcu(). > >> > >> > > >> > >> >> is_vma_detached() - false negative as the write above didn't= propagate > >> > >> >> here yet; a read barrier but where is the write barrier? > >> > >> >> checks for vma->vm_mm, vm_start, vm_end - nobody reset them = yet so false > >> > >> >> positive, or they got reset on reallocation but writes did= n't propagate > >> > >> >> > >> > >> >> Am I missing something that would prevent lock_vma_under_rcu()= falsely > >> > >> >> succeeding here? > >> > >> >> > >> > >> > >> > >