linux-kernel - Re: [PATCH v2] mm/slab: simplify SLAB

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9c6e9064-89b0-4373-ae66-8aae5ade6fa2@suse.cz>
Date: Wed, 5 Feb 2025 11:38:51 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: Hyeonggon Yoo <42.hyeyoo@...il.com>, Kevin Brodsky <kevin.brodsky@....com>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 Christoph Lameter <cl@...ux.com>, Pekka Enberg <penberg@...nel.org>,
 David Rientjes <rientjes@...gle.com>, Joonsoo Kim <iamjoonsoo.kim@....com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Roman Gushchin <roman.gushchin@...ux.dev>
Subject: Re: [PATCH v2] mm/slab: simplify SLAB_* flag handling

On 2/2/25 07:57, Hyeonggon Yoo wrote:
> On Sat, Jan 25, 2025 at 1:49 AM Kevin Brodsky <kevin.brodsky@....com> wrote:
>>
>> SLUB is the only remaining allocator. We can therefore get rid of
>> the logic for allocator-specific flags:
>>
>> * Merge SLAB_CACHE_FLAGS into SLAB_CORE_FLAGS.
>>
>> * Remove CACHE_CREATE_MASK and instead mask out SLAB_DEBUG_FLAGS if
>>   !CONFIG_SLUB_DEBUG. SLAB_DEBUG_FLAGS is now defined
>>   unconditionally (no impact on existing code, which ignores it if
>>   !CONFIG_SLUB_DEBUG).
>>
>> * Define SLAB_FLAGS_PERMITTED in terms of SLAB_CORE_FLAGS and
>>   SLAB_DEBUG_FLAGS (no functional change).
>>
>> While at it also remove misleading comments that suggest that
>> multiple allocators are available.
>>
>> Signed-off-by: Kevin Brodsky <kevin.brodsky@....com>
> 
> Hi Kevin,
> This patch in general looks fine to me.
> 
> However, there are subtle changes that were not documented by the changelog.
> SLUB currently does not _completely_ ignore debug flags even when
> CONFIG_SLUB_DEBUG=n. For example, calculate_sizes() relocate the free pointer
> regardless of CONFIG_SLUB_DEBUG.

I think the patch makes no difference there effectively? Before the patch,
CACHE_CREATE_MASK would be used to filter flags and not include the debug
flags, so that free pointer relocation due to SLAB_POISON or SLAB_RED_ZONE
would not happen. After the patch, it's filtered effectively the same? Am I
missing something?

> I believe completely ignoring debug flags should be acceptable
> when CONFIG_SLUB_DEBUG=n, but this change should at least be documented
> in the changelog.
> 
> Additionally, beyond what this patch currently does, we can remove several
> CONFIG_SLUB_DEBUG #ifdefs in some functions (e.g., in calculate_sizes())
> on top of this patch.

So AFAIK that should be possible both before and after this patch and you
can try (it's out of scope of this patch). But I suspect the reason for the
#ifdefs is that it the code would reference structures or fields that don't
exist without CONFIG_SLUB_DEBUG. Maybe that's not (no longer?) true in some
cases and we could clean up. Another reason would be performance, but we
shouldn't care in code that's only executed during cache creation.

> How does that sound to you?
> 
> Best,
> Hyeonggon