Re: [PATCH] mm/slub: disable slab merging in the default configuration

[Kees Cook]

On Thu, Jun 29, 2023 at 09:21:14AM +0200, Vlastimil Babka wrote:
> On 6/28/23 18:44, Roman Gushchin wrote:
> > On Tue, Jun 27, 2023 at 12:32:15PM -0700, David Rientjes wrote:
> >> On Tue, 27 Jun 2023, Julian Pidancet wrote:
> >> 
> >> > Make CONFIG_SLAB_MERGE_DEFAULT default to n unless CONFIG_SLUB_TINY is
> >> > enabled. Benefits of slab merging is limited on systems that are not
> >> > memory constrained: the overhead is negligible and evidence of its
> >> > effect on cache hotness is hard to come by.
> >> > 
> >> 
> >> I don't have an objection to this, I think it makes sense.
> > 
> > +1
> > 
> > I believe the overhead was much larger when we had per-memcg slab caches,
> > but now it should be fairly small on most systems.
> > 
> > But I wonder if we need a new flag (SLAB_MERGE?) to explicitly force merging
> > on per-slab cache basis.
> 
> Damn, we just tried to add SLAB_NO_MERGE, that is if Linus pulls the PR, as
> I've just found out that the last time he hated the idea [1] :) (but at the
> same time I think the current attempt is very different in that it's not
> coming via a random tree, and the comments make it clear that it's not for
> everyone to enable in production configs just because they think they are
> special).
> 
> But SLAB_MERGE, I doubt it would get many users being opt-in. People would
> have to consciously opt-in to not being special.
> 
> As for changing the default, we definitely need to see the memory usage
> results first, as was mentioned. It's not expected that disabling merging
> would decrease performance, so no wonder the test didn't find such decrease,
> but the expected downside is really increased memory overhead.

Did this analysis happen? Apologies if I missed it...

> But then again it's just a default and most people would use a distro config
> anyway, and neither option seems to be an obvious winner to me? As for the
> "security by default" argument, AFAIK we don't enable freelist
> hardening/randomization by default, and I thought (not being the expert on
> this) the heap spraying attacks concerned mainly generic kmalloc cache users
> (see also [2]) and not some specific named caches being merged?
> 
> [1] https://lore.kernel.org/all/CA+55aFyepmdpbg9U2Pvp+aHjKmmGCrTK2ywzqfmaOTMXQasYNw@xxxxxxxxxxxxxx/
> [2] https://lore.kernel.org/all/20230626031835.2279738-1-gongruiqi@xxxxxxxxxxxxxxx/

I'm a fan of turning on any of these "by default", as that's been the
historical approach, which tends to span years:

- security feature introduced, default off in the kernel
- distros enable it by default
- kernel makes it default on

So perhaps we're better off making the other hardening features on by
default since distros have been shipping with them for years now?

-Kees

-- 
Kees Cook