From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D86DE30B50A for ; Tue, 14 Jul 2026 05:17:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784006245; cv=none; b=UrKDxUqDBcJmmy1MGY3TkIv/vN0fY3ml3SzNnOOkL6vGvZ7UzJSgKsk5GmuAfGCSppZLEHEKffjFljNPNHB5HQGuQ0LFxJxGIkzl8gLRZ0nms5TDLc26WCpo2MLXVic6Iekm04MplvbomJ2Eay//27ZT6R2bcLAs1ahk/czfdeg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784006245; c=relaxed/simple; bh=WIHHAoMgKqeoKHRR26XAE8j9oxGFBmD5OaWXep8TGNE=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Wt5v7D71eTie/u7QnzQ11BMVOPcTSGAM5eKCcI6JxNZpOjzzgH0rFkGoRosWY+13ecc67kEYlAKHpYmRYlrYH4Swy4rOEenlaphpxYLg/w4rGf+p+hQzkOB0WlSJErl7CnIQs5dbme22KjdiC4ZnQMH836SqEu5jRx1fyrMWBwI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=g5msADjj; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="g5msADjj" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 093F91F000E9; Tue, 14 Jul 2026 05:17:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1784006243; bh=/OjuiN05GbYsLwmaFUSOWemRKGb9XaF2IwNnXtFq2YA=; h=Date:Subject:To:Cc:References:From:In-Reply-To; b=g5msADjjydhlG2el855oH79NnH+/3tMuni1u9RR13v2aIZi6wnMeMHHWc2FhpkLBi /kmp5xaAiczsTmhFeS2OaknKNyKYuGPVeCKJ5JfoqpXuKPawx6QLYDsybPtt4oyQ/x PRx0qc5goakUGEGPWJ9NG4LDSb39rBL7Vjz1oBS2SU4WVr2orbey03ww5qiil2u7k9 qY7n/rOU0rC06hthkqjKf6v3V7wEKssUQxkRgcYJR0KR962FGB2QVrPQq/ApFSqnI2 2eT1zzGlHwU92BXucG4H9DDj5VUoy+KIPMy1zfo+/RFMKhqagZ8G4vBmh6O+qwYdZQ +MGD7hiqaNrqA== Message-ID: <121dda1a-f050-4035-93c0-6a36278e50b7@kernel.org> Date: Tue, 14 Jul 2026 14:17:16 +0900 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH slab/for-next-fixes v3 4/4] mm/slab: prevent unbounded recursion in free path with new kmalloc type To: Suren Baghdasaryan Cc: Vlastimil Babka , Andrew Morton , Hao Li , Christoph Lameter , David Rientjes , Roman Gushchin , "Liam R. Howlett" , Hao Ge , Kees Cook , Pedro Falcato , Shakeel Butt , Danielle Constantino , linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20260713-kmalloc-no-objext-v3-0-47c7bd138de7@kernel.org> <20260713-kmalloc-no-objext-v3-4-47c7bd138de7@kernel.org> Content-Language: en-US From: Harry Yoo In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------UVBlYSA9X76uRpIL4AKv05mR" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --------------UVBlYSA9X76uRpIL4AKv05mR Content-Type: multipart/mixed; boundary="------------2Cw0wARP2YgVbWWTJx8Rq53F"; protected-headers="v1" From: Harry Yoo To: Suren Baghdasaryan Cc: Vlastimil Babka , Andrew Morton , Hao Li , Christoph Lameter , David Rientjes , Roman Gushchin , "Liam R. Howlett" , Hao Ge , Kees Cook , Pedro Falcato , Shakeel Butt , Danielle Constantino , linux-mm@kvack.org, linux-kernel@vger.kernel.org Message-ID: <121dda1a-f050-4035-93c0-6a36278e50b7@kernel.org> Subject: Re: [PATCH slab/for-next-fixes v3 4/4] mm/slab: prevent unbounded recursion in free path with new kmalloc type References: <20260713-kmalloc-no-objext-v3-0-47c7bd138de7@kernel.org> <20260713-kmalloc-no-objext-v3-4-47c7bd138de7@kernel.org> In-Reply-To: --------------2Cw0wARP2YgVbWWTJx8Rq53F Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 7/14/26 2:08 AM, Suren Baghdasaryan wrote: > On Mon, Jul 13, 2026 at 7:29=E2=80=AFAM Harry Yoo (Oracle) wrote: >> @@ -386,12 +387,17 @@ static inline unsigned int size_index_elem(unsig= ned int bytes) >> * KMALLOC_MAX_CACHE_SIZE and the caller must check that. >> */ >> static inline struct kmem_cache * >> -kmalloc_slab(size_t size, kmem_buckets *b, gfp_t flags, kmalloc_token= _t token) >> +kmalloc_slab(size_t size, kmem_buckets *b, gfp_t flags, kmalloc_token= _t token, >> + unsigned int alloc_flags) >> { >> unsigned int index; >> + enum kmalloc_cache_type type =3D kmalloc_type(flags, token); >> + >> + if (alloc_flags & SLAB_ALLOC_NO_OBJ_EXT) >> + type =3D KMALLOC_NO_OBJ_EXT; Hi Suren, thanks for the reviews. It's indeed helpful to have an eye for those bugfixes. > Why not let kmalloc_type() handle alloc_flags? Good point! > Other users (there are > only 4 of them) can pass SLAB_ALLOC_DEFAULT. That seems cleaner to me > and more robust. Hmm, there was a reason... *checks notes*, oh, there is no note. IIRC I was afraid of exposing SLAB_ALLOC_* flags to arbitrary users. should probably fine as long as it's not used in kmalloc/kmem_cache_alloc() APIs, not sure. >> if (!b) >> - b =3D &kmalloc_caches[kmalloc_type(flags, token)]; >> + b =3D &kmalloc_caches[type]; >> if (size <=3D 192) >> index =3D kmalloc_size_index[size_index_elem(size)]; >> else >> diff --git a/mm/slab_common.c b/mm/slab_common.c >> index b6426d7ceec9..03ecac12cd86 100644 >> --- a/mm/slab_common.c >> +++ b/mm/slab_common.c >> @@ -957,6 +968,12 @@ new_kmalloc_cache(int idx, enum kmalloc_cache_typ= e type) >> return; >> } >> flags |=3D SLAB_ACCOUNT; >> + } else if (IS_ENABLED(CONFIG_SLAB_OBJ_EXT) && type =3D=3D KMAL= LOC_NO_OBJ_EXT) { >=20 > Hmm, you have to check IS_ENABLED(CONFIG_SLAB_OBJ_EXT) here because > KMALLOC_NO_OBJ_EXT can be aliased with KMALLOC_NORMAL... Could we > instead have a helper function like this (maybe with a better name): Hmm that's fine, but I think that bit should not be part of -stable fixes at least. Here I tried to make it consistent with KMALLOC_RECLAIM and KMALLOC_DMA :) > #ifdef CONFIG_SLAB_OBJ_EXT > bool is_kmalloc_no_obj_ext_type(type) { return type =3D=3D KMALLOC_NO_O= BJ_EXT; } > #else > bool is_kmalloc_no_obj_ext_type(type) { return false; } > #endif > ? is_kmalloc_no_obj_ext_type(), kmalloc_type_is_no_obj_ext(), is_kmalloc_type_no_obj_ext(), =2E.. naming is hard, ugh :) >=20 >> + if (!need_kmalloc_no_objext()) { >> + kmalloc_caches[type][idx] =3D kmalloc_caches[K= MALLOC_NORMAL][idx]; >=20 > Could kmalloc_caches[KMALLOC_NORMAL][idx] be NULL here? No. KMALLOC_NORMAL caches are created before all other kmalloc caches. IIRC checking if kmalloc_caches[KMALLOC_NORMAL][idx] is NULL was added by commit 963e84b0f262 ("mm/slab: limit kmalloc() minimum alignment to dma_get_cache_alignment()") to avoid creating kmalloc caches of same type and size due to minimum alignment. > In general why do we special-case and do an early exit here? > > Can we do instead: >=20 > if (need_kmalloc_no_objext()) > flags |=3D SLAB_NO_OBJ_EXT | SLAB_NO_MERGE; >=20 > and use the common path? Hmm, but even without SLAB_NO_MERGE, we often end up not merging kmalloc caches e.g.) because of non-zero s->usersize. I think that's why we do special-case and an early exit? We could probably do some refactoring to change that, but in general I'm afraid of backporting refactoring work to -stable because I fear introducing very subtle behavioral changes that nobody would notice. >> + return; >> + } >> + flags |=3D SLAB_NO_OBJ_EXT | SLAB_NO_MERGE; >> } else if (IS_ENABLED(CONFIG_ZONE_DMA) && (type =3D=3D KMALLOC= _DMA)) { >> flags |=3D SLAB_CACHE_DMA; >> } >> diff --git a/mm/slub.c b/mm/slub.c >> index abe748b7dddb..a34f9b8770dc 100644 >> --- a/mm/slub.c >> +++ b/mm/slub.c >> @@ -2168,14 +2132,20 @@ int alloc_slab_obj_exts(struct slab *slab, str= uct kmem_cache *s, >> unsigned long new_exts; >> unsigned long old_exts; >> struct slabobj_ext *vec; >> - size_t sz; >> + size_t sz =3D sizeof(struct slabobj_ext) * slab->objects; >> >> gfp &=3D ~OBJCGS_CLEAR_MASK; >> - /* Prevent recursive extension vector allocation */ >> - alloc_flags |=3D SLAB_ALLOC_NO_RECURSE; >> - alloc_flags &=3D ~SLAB_ALLOC_NEW_SLAB; >> + /* >> + * In most cases, obj_exts arrays are allocated from normal km= alloc. >> + * However, normal kmalloc caches must allocate them from >> + * KMALLOC_NO_OBJ_EXT caches to prevent recursion. >=20 > For debugging it would have been convenient to allocate all obj_ext > vectors from dedicated caches... Maybe we can do that for > CONFIG_DEBUG_VM or someday when we add CONFIG_OBJ_EXT_DEBUG? Anyway, > not really a complaint but a wish. Vlastimil and I had a conversation on always (even w/o debug options) having dedicated caches (primarily to separate lifetime and for simplicity), it would be interesting to explore. https://lore.kernel.org/all/2436707a-b6ab-45ec-98e5-538e18589462@kernel.o= rg --=20 Cheers, Harry / Hyeonggon --------------2Cw0wARP2YgVbWWTJx8Rq53F-- --------------UVBlYSA9X76uRpIL4AKv05mR Content-Type: application/pgp-signature; name="OpenPGP_signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="OpenPGP_signature.asc" -----BEGIN PGP SIGNATURE----- iHUEARYKAB0WIQQQ1ub6gR5ogjaKRmOGXBN6rc5S1gUCalXGXAAKCRCGXBN6rc5S 1l8cAQC3kFEYeg3gH7846Wi5C2cKWOq9FC9HqLfR6CZBYttsbAEAmXMstvPLhZ1q FZRL7Vh4QgPoHbC5DYcp6+ZX0kuNlAw= =ZxKB -----END PGP SIGNATURE----- --------------UVBlYSA9X76uRpIL4AKv05mR--