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 9D1C3C19F2D for ; Tue, 9 Aug 2022 22:26:16 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D961D8E0002; Tue, 9 Aug 2022 18:26:15 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D45528E0001; Tue, 9 Aug 2022 18:26:15 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C0DA08E0002; Tue, 9 Aug 2022 18:26:15 -0400 (EDT) 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 B06088E0001 for ; Tue, 9 Aug 2022 18:26:15 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 7F9D01C67BE for ; Tue, 9 Aug 2022 22:26:15 +0000 (UTC) X-FDA: 79781488710.08.D2A8DA9 Received: from out0.migadu.com (out0.migadu.com [94.23.1.103]) by imf06.hostedemail.com (Postfix) with ESMTP id A6092180156 for ; Tue, 9 Aug 2022 22:26:14 +0000 (UTC) Date: Tue, 9 Aug 2022 15:25:54 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1660083972; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=hOnt2Z7v85TLJ7c4EagfbuOt1hNBAIsJlwJaIbi0B/k=; b=Rxnb7nBzIHrkN8GbjZULOtGCCexNPdqnvEmPDG5K/SP7Ugt2QYQYtVR1qWLBe0mggDxdJo EdcQXS/P8LMsRN+FfCYXKckwQLMJyom1+ULCj6DcSB/xOQgTX/E0GtgJWJrDkZ0XzC0fGD xMRaZwT1xiA+Rt7WT19Ek5pZbPbC1wc= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Roman Gushchin To: Waiman Long Cc: Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Vlastimil Babka , Hyeonggon Yoo <42.hyeyoo@gmail.com>, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm/slab_common: Deleting kobject in kmem_cache_destroy() without holding slab_mutex/cpu_hotplug_lock Message-ID: References: <20220809205901.76595-1-longman@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220809205901.76595-1-longman@redhat.com> X-Migadu-Flow: FLOW_OUT X-Migadu-Auth-User: linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1660083975; a=rsa-sha256; cv=none; b=dZKwG1gMnWksU1N/MfCloLYoHh1kLjfa7WcCPKkVm69MYx1HdtM2CJXsxvUOXVFzZDscID WKMZg8v5CzOPzDEwHcGQwxGmsRAAKUKUoliibOZ3dnNcBmrOSwkEvGuWlyXZFq3bvOSAau 2mUy4XeHPOyMr7kwSCyBh01WPSxivmE= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=Rxnb7nBz; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf06.hostedemail.com: domain of roman.gushchin@linux.dev designates 94.23.1.103 as permitted sender) smtp.mailfrom=roman.gushchin@linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1660083975; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=hOnt2Z7v85TLJ7c4EagfbuOt1hNBAIsJlwJaIbi0B/k=; b=1cGthj6mt6HZwegfIRv02RozvQNeXcI496x+RbyOF773XtGidIV0+rqtQeenYashujI2fB 24eotf0j2auQOFJD8J5tM3xLYTygxkegoKmRQ7b1V3MUa2G20D0q3HHCyvKEr8KOItdhwV co8q99BOrWwdk95AT1O+ByP4/4Fy2mk= X-Rspamd-Server: rspam10 X-Stat-Signature: s4j9fiq16fw5qy6c77umjs7x6m6p81jb Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=Rxnb7nBz; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf06.hostedemail.com: domain of roman.gushchin@linux.dev designates 94.23.1.103 as permitted sender) smtp.mailfrom=roman.gushchin@linux.dev X-Rspam-User: X-Rspamd-Queue-Id: A6092180156 X-HE-Tag: 1660083974-656832 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: On Tue, Aug 09, 2022 at 04:59:01PM -0400, Waiman Long wrote: > A circular locking problem is reported by lockdep due to the following > circular locking dependency. > > +--> cpu_hotplug_lock --> slab_mutex --> kn->active#126 --+ > | | > +---------------------------------------------------------+ > > One way to break this circular locking chain is to avoid holding > cpu_hotplug_lock and slab_mutex while deleting the kobject in > sysfs_slab_unlink() which should be equivalent to doing a write_lock > and write_unlock pair of the kn->active virtual lock. > > Since the kobject structures are not protected by slab_mutex or the > cpu_hotplug_lock, we can certainly release those locks before doing > the delete operation. > > Move sysfs_slab_unlink() and sysfs_slab_release() to the newly > created kmem_cache_release() and call it outside the slab_mutex & > cpu_hotplug_lock critical sections. > > Signed-off-by: Waiman Long > --- > mm/slab_common.c | 48 +++++++++++++++++++++++++++++++----------------- > 1 file changed, 31 insertions(+), 17 deletions(-) > > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 17996649cfe3..9274fb03563e 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -392,6 +392,30 @@ kmem_cache_create(const char *name, unsigned int size, unsigned int align, > } > EXPORT_SYMBOL(kmem_cache_create); > > +#ifdef SLAB_SUPPORTS_SYSFS > +/* > + * For a given kmem_cache, kmem_cache_destroy() should only be called > + * once or there will be a use-after-free problem. The actual deletion > + * and release of the kobject does not need slab_mutex or cpu_hotplug_lock > + * protection. So they are now done without holding those locks. > + */ > +static void kmem_cache_release(struct kmem_cache *s, bool workfn) > +{ > + if (!workfn) > + sysfs_slab_unlink(s); > + > + if (workfn || !(s->flags & SLAB_TYPESAFE_BY_RCU)) > + sysfs_slab_release(s); > + else > + schedule_work(&slab_caches_to_rcu_destroy_work); > +} > +#else > +static inline void kmem_cache_release(struct kmem_cache *s, bool workfn) > +{ > + slab_kmem_cache_release(s); > +} > +#endif > + > static void slab_caches_to_rcu_destroy_workfn(struct work_struct *work) > { > LIST_HEAD(to_destroy); > @@ -418,11 +442,7 @@ static void slab_caches_to_rcu_destroy_workfn(struct work_struct *work) > list_for_each_entry_safe(s, s2, &to_destroy, list) { > debugfs_slab_release(s); > kfence_shutdown_cache(s); > -#ifdef SLAB_SUPPORTS_SYSFS > - sysfs_slab_release(s); > -#else > - slab_kmem_cache_release(s); > -#endif > + kmem_cache_release(s, true); Hi Waiman! As I understand, with SLAB_SUPPORTS_SYSFS kmem_cache_release() can effectively call into itself: first it's called with workfn == false from shutdown_cache() and then optionally it's scheduled to call itself from a work context with workfn == true just to call sysfs_slab_release(). Is it right? If !SLAB_SUPPORTS_SYSFS, shutdown_cache() optionally adds kmem_cache to the slab_caches_to_rcu_destroy list and calls kmem_cache_release(s, false) == slab_kmem_cache_release(). How it's then removed from the list? Overall the patch is a bit hard to follow (not like this code was easy to read before, so can't blame the patch). But I wonder if it will make things simpler to decouple kmem_cache_release(workfn == true) and kmem_cache_release(workfn == false) into 2 different helpers? Or at least add a bold comment on how things are supposed to work. Thanks!