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 3D46DE6B277 for ; Fri, 1 Nov 2024 13:15:30 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 710BF6B0093; Fri, 1 Nov 2024 09:15:29 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 699EB6B0095; Fri, 1 Nov 2024 09:15:29 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 513576B0096; Fri, 1 Nov 2024 09:15:29 -0400 (EDT) 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 2D11A6B0093 for ; Fri, 1 Nov 2024 09:15:29 -0400 (EDT) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id C50C21A0381 for ; Fri, 1 Nov 2024 13:15:28 +0000 (UTC) X-FDA: 82737572190.16.AB7F388 Received: from mail-lf1-f48.google.com (mail-lf1-f48.google.com [209.85.167.48]) by imf05.hostedemail.com (Postfix) with ESMTP id 7B56910000B for ; Fri, 1 Nov 2024 13:14:33 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=bdv+egdd; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf05.hostedemail.com: domain of 42.hyeyoo@gmail.com designates 209.85.167.48 as permitted sender) smtp.mailfrom=42.hyeyoo@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1730466870; a=rsa-sha256; cv=none; b=3E2e3J09wVsuMUyfaTG4rT22ghkvmLe1w06EZjkuT8RPYGNlR61TPTqwDHNcccu5bS1dzy XMo2V43x7kPHFCkUA3azX9RrA6yOBgX8N00pNV1PCKnSCXt+3MgEjS9Sjzar756yX4XPW8 qADKbdA92N1YyRWgKZurQAZ9l+JCiE8= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=bdv+egdd; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf05.hostedemail.com: domain of 42.hyeyoo@gmail.com designates 209.85.167.48 as permitted sender) smtp.mailfrom=42.hyeyoo@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1730466870; 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=Ocnxd+1uUKck5J10IiP1luzFvnxdZ9BHNlGx5Qg53+g=; b=IzkqMwMS/cKrv+B7qzcN5WTVpqD1Qhtm6hfTq581QzbdWqLbgspf9OofoUiO4tg/kHXGCB tbGPfw4yU5XzPDoe+xkkS73zD3IroujD+LxvC2Nu6iAl2KMI6AZPWIreF5PoNxd0oa0mQ8 frySbhLASeunpcEJJjXJpw7e1uEx+gw= Received: by mail-lf1-f48.google.com with SMTP id 2adb3069b0e04-53c78ebe580so2051552e87.1 for ; Fri, 01 Nov 2024 06:15:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1730466925; x=1731071725; 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=Ocnxd+1uUKck5J10IiP1luzFvnxdZ9BHNlGx5Qg53+g=; b=bdv+egddHu2NdNCz74By0fhG2GoMtI5ibhCUS7h++bqqnNt6yEE3/0Yxtbtk0rp799 wkeat4joeK4c6hsVDEFuh4FAzVTmP8s1T0jODSN+b5pxSR0KiL1Y2NeAU5oulbDhIPll DHOvLZRM0Si0Y2ySUchpkL1HDALCAp+QZadMxZdjRTtRqCFCN6UeIfGcACOYzpnYb5f9 QuLKenOrCyMUizIDlWr5WLz4DvxY3mCbZ1K9fjIkRnXSGrV8d6Rg3+nZxRi2e/54+YUL V+Cqsc10VuacgcWQOAEjHjqVei0sU7F0WMKqI3jxbpE5NCraFjx6rUP63nW8il0Gj/iM MiSQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730466925; x=1731071725; 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=Ocnxd+1uUKck5J10IiP1luzFvnxdZ9BHNlGx5Qg53+g=; b=IXAuzm4rWXVYuRxV9yYOjQRZTi8QWbu5N96v7vbtmlnNBJcsrwdmN21KHEUKtFr6xS jHbaH7sBF2jFKRWClRV7QzalQMvlpYU75y/jDesLm1elP4DpzujBE+eTpyv4X82q6X66 iy93czOqsqvbYMez6jwE9s+68WSZrCKg8Y+5T7ED7JRx79gt01q9kqWc/uw+cfF8qDUw 119FlJyCuk+fbfva/gqQQjKv/5iXMFOQkyCK4OGsnapOE2ogkx0mUMO1Ubtg03KSiVUQ bsfquhXVr3wbBYxoYtzZ3kcPX07hNHN2JAyMSZOYSqLHJdLTfrezMiV4uYRjqgbiZyFk rvQQ== X-Gm-Message-State: AOJu0YwconaeCplcTclhTXqh83RVKFPdT+Cmy9910woKNbM8TG7HSMnV qY+RDCzEqWEHkhno/P8PVU2ktvBE7NeSB650V/rOErcNKKqiPGMYlCUA0C3obvLfwIbRthibzT2 6/Jd5HKC/zG22J65NHeVSS/0mOFo= X-Google-Smtp-Source: AGHT+IGFQwrYQle6T0sL/uYDqVsWF2AB3EkJUvjPRsB9jTBFIYZrPubF//JSwFuXF4GuKVfoN+k9u5dbvjTnQZsScfs= X-Received: by 2002:a05:6512:2350:b0:539:ede3:827f with SMTP id 2adb3069b0e04-53d65dc3a59mr1180127e87.24.1730466924636; Fri, 01 Nov 2024 06:15:24 -0700 (PDT) MIME-Version: 1.0 References: <20241101130845.19100-1-42.hyeyoo@gmail.com> In-Reply-To: <20241101130845.19100-1-42.hyeyoo@gmail.com> From: Hyeonggon Yoo <42.hyeyoo@gmail.com> Date: Fri, 1 Nov 2024 22:15:12 +0900 Message-ID: Subject: Re: [PATCH v1] mm/slab: Allow cache creation to proceed even if sysfs registration fails To: Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Vlastimil Babka , Roman Gushchin Cc: linux-mm@kvack.org, Jinjie Ruan , Liu Shixin Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Queue-Id: 7B56910000B X-Rspamd-Server: rspam01 X-Stat-Signature: czs3djxh78ax1p87rnoa65udysfnnuw9 X-HE-Tag: 1730466873-934281 X-HE-Meta: U2FsdGVkX1/nqnaHsB8zvDpf8uWn7GOC3ziOTaYeJHNpOh8+QZ80mZNq1kCl9COubcsRU/QVPUXBZWUzS0AYhAx13MDIhN9Rsx1qw9xpaACuLJIkYCUqHLV+1DvDPp4agn/t9OGoPntkyW2/bG4e95MwpSfCp+jSpFdlL1bBAa5HQyIHcLSd0w6qJBF3AbGikYNGAntJ6DNXhmH5DAiJ1LrdsksSwTmTDbIwN9jygzK/XA6hQ00YS9iMTv/Bagv8/LgpQK2fItB6RDEf5OqYmSxXleXTE8IpBhM/8ZV2x8sEzdi8LrTot1dwDwM0mttyGMA++yuF+cA2E9f+DG2rCDwoog7+O4LWMzN2+WsQYb4uSetz8MbV+qQ2GpWekKmkQtkBgpsfmaeSl7nT7Z9f3NdLSFWjrZUpENUQVRXrkInuucT/yURE/EVFoW8U44/8qna0IZhboxOvo2Bf9x4wqduTg0M8De7xZpIvdA7po2Sd7MmQMCJuvOmtOWj5rUI4pD43nKqNLrR8D5qNHPFnYa+c0HEmmR7dFd6/d9W4FO5FjPYRIhOcCl8JIO5q8143FVjyVWzgvrpzjm7iEJutJM2pG8miXaBf7KoPR2rqV8Y6QCLs5/kLwgW1BqIa3IL019mLG2LPE4Z0a1b0Yz7BeLTnxWsvF3duFg7rFMST/YyZfKyoixqU3rsIVj9psG0M7W91YRIwh3VGXFmnaU1gwL+Qp9LlGyFR04ZL5YMV2w3h2VTS8nt3INx3CBX/KIFfhF3llg7NymQav++DXa6EDCN/XpBhx2BjoUFU3qKoWXlLM3oPVFoFFUkxR53oPX4/gy8nBdLrI1GJCtTtgRfLmwi11mUXPLCrzHs6kWZKzmeIMh/JstNUDtDIgx+VQvdstBqstlc6cOYg5Y4Pmo9aNsQwaKBO2vN4rijfotSG0+HoOph7sCCIPLBaItYHTLASrTeul7ersfAmgVPmQxG qPOGynwr Br4CJalFDAy/yTJPR47d6Nef5jv6Ta7OJH/eWg1MSPjmAg7ECmziPiB//paFlnDVgOuHLXyE0wkYkCSg6pI+ehVwXMs1J3tbKLaX+MMmznu8NpYZygd9K5YtYrtriaQLCWmO0i/apaQhE0PORWjz3ajMFp8QAPJV3f0opLmWhJRrWPAHA4+kpLHKvwGYr4f72/BRg+NnwNKzo8tVvR1NxlN3QBUYKrSHDx9dp67T6ybUB3v64DScYgQFC4fElxwYKr90wkAEA+NTWhsouFoiqrV7KPUEfpGV4Y0dXlE8cR8GSiqpfF8V7Pt6JKmUhHIHgMMJ/yIDu0Kdc29GiUxgNNkgYFdZ2R66rA+6WxHgAWEiZGEOd7Eky31vnVMESJcvAtokRtLxXp018knhTFvHpa4fojw== 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, Nov 1, 2024 at 10:08=E2=80=AFPM Hyeonggon Yoo <42.hyeyoo@gmail.com>= wrote: > > When kobject_init_and_add() fails during cache creation, > kobj->name can be leaked because SLUB does not call kobject_put(), > which should be invoked per the kobject API documentation. > This has a bit of historical context, though; SLUB does not call > kobject_put() to avoid double-free for struct kmem_cache because > 1) simply calling it would free all resources related to the cache, and > 2) struct kmem_cache descriptor is always freed by cache_cache()'s > error handling path, causing struct kmem_cache to be freed twice. > > This issue can be reproduced by creating new slab caches while applying > failslab for kernfs_node_cache. This makes kobject_add_varg() succeed, > but causes kobject_add_internal() to fail in kobject_init_and_add() > during cache creation. > > Historically, this issue has attracted developers' attention several time= s. > Each time a fix addressed either the leak or the double-free, > it caused the other issue. Let's summarize a bit of history here: > > The leak has existed since the early days of SLUB. > > Commit 54b6a731025f ("slub: fix leak of 'name' in sysfs_slab_add") > introduced a double-free bug while fixing the leak. > > Commit 80da026a8e5d ("mm/slub: fix slab double-free in case of duplicat= e > sysfs filename") re-introduced the leak while fixing the double-free > error. > > Commit dde3c6b72a16 ("mm/slub: fix a memory leak in sysfs_slab_add()") > fixed the memory leak, but it was later reverted by commit 757fed1d0898 > ("Revert "mm/slub: fix a memory leak in sysfs_slab_add()"") to avoid > the double-free error. > > This is where we are now: we've chosen a memory leak over a double-free= . > > To resolve this memory leak, skip creating sysfs files if it fails > and continue with cache creation regardless (as suggested by Christoph). > This resolves the memory leak because both the cache and the kobject > remain alive on kobject_init_and_add() failure. > > If SLUB tries to create an alias for a cache without sysfs files, > its symbolic link will not be generated. > > Since a slab cache might not have associated sysfs files, call kobject_de= l() > only if such files exist. > > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> [+Cc Jinjie, Liu] > --- > > RFC -> v1: Make sysfs optional instead of destroying the cache on sysfs > errors. (Suggested by Christoph) RFC version for reference: https://lore.kernel.org/linux-mm/20241021091413.154775-1-42.hyeyoo@gmail.co= m/ > mm/slub.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 151a987dc3a0..b4b211468f77 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -6116,7 +6116,8 @@ __kmem_cache_alias(const char *name, unsigned int s= ize, unsigned int align, > s =3D find_mergeable(size, align, flags, name, ctor); > if (s) { > if (sysfs_slab_alias(s, name)) > - return NULL; > + pr_err("SLUB: Unable to add slab alias %s to sysf= s\n", > + name); > > s->refcount++; > > @@ -6204,9 +6205,15 @@ int do_kmem_cache_create(struct kmem_cache *s, con= st char *name, > goto out; > } > > + /* > + * Failing to create sysfs files is not critical to SLUB function= ality. > + * If it fails, proceed with cache creation without these files. > + */ > err =3D sysfs_slab_add(s); > - if (err) > - goto out; > + if (err) { > + err =3D 0; > + pr_err("SLUB: Unable to add slab %s to sysfs\n", s->name)= ; > + } > > if (s->flags & SLAB_STORE_USER) > debugfs_slab_add(s); > @@ -7276,7 +7283,8 @@ static int sysfs_slab_add(struct kmem_cache *s) > > void sysfs_slab_unlink(struct kmem_cache *s) > { > - kobject_del(&s->kobj); > + if (s->kobj.state_in_sysfs) > + kobject_del(&s->kobj); > } > > void sysfs_slab_release(struct kmem_cache *s) > @@ -7305,6 +7313,11 @@ static int sysfs_slab_alias(struct kmem_cache *s, = const char *name) > * If we have a leftover link then remove it. > */ > sysfs_remove_link(&slab_kset->kobj, name); > + /* > + * The original cache may have failed to generate sysfs f= ile. > + * In that case, sysfs_create_link() returns -ENOENT and > + * symbolic link creation is skipped. > + */ > return sysfs_create_link(&slab_kset->kobj, &s->kobj, name= ); > } > > -- > 2.45.0 >