Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects

From: Vlastimil Babka <vbabka@suse.cz>
To: Dave Chinner <david@fromorbit.com>, Rustam Kovhaev <rkovhaev@gmail.com>
Cc: djwong@kernel.org, linux-xfs@vger.kernel.org, cl@linux.com,
	penberg@kernel.org, rientjes@google.com, iamjoonsoo.kim@lge.com,
	akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, gregkh@linuxfoundation.org,
	Al Viro <viro@ZenIV.linux.org.uk>
Subject: Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
Date: Thu, 30 Sep 2021 10:13:40 +0200	[thread overview]
Message-ID: <17f537b3-e2eb-5d0a-1465-20f3d3c960e2@suse.cz> (raw)
In-Reply-To: <20210930044202.GP2361455@dread.disaster.area>

On 9/30/21 06:42, Dave Chinner wrote:
> On Wed, Sep 29, 2021 at 02:23:47PM -0700, Rustam Kovhaev wrote:
>> For kmalloc() allocations SLOB prepends the blocks with a 4-byte header,
>> and it puts the size of the allocated blocks in that header.
>> Blocks allocated with kmem_cache_alloc() allocations do not have that
>> header.
>> 
>> SLOB explodes when you allocate memory with kmem_cache_alloc() and then
>> try to free it with kfree() instead of kmem_cache_free().
>> SLOB will assume that there is a header when there is none, read some
>> garbage to size variable and corrupt the adjacent objects, which
>> eventually leads to hang or panic.
>> 
>> Let's make XFS work with SLOB by using proper free function.
>> 
>> Fixes: 9749fee83f38 ("xfs: enable the xfs_defer mechanism to process extents to free")
>> Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com>
> 
> IOWs, XFS has been broken on SLOB for over 5 years and nobody
> anywhere has noticed.
> 
> And we've just had a discussion where the very best solution was to
> use kfree() on kmem_cache_alloc() objects so we didn't ahve to spend
> CPU doing global type table lookups or use an extra 8 bytes of
> memory per object to track the slab cache just so we could call
> kmem_cache_free() with the correct slab cache.
> 
> But, of course, SLOB doesn't allow this and I was really tempted to
> solve that by adding a Kconfig "depends on SLAB|SLUB" option so that
> we don't have to care about SLOB not working.
> 
> However, as it turns out that XFS on SLOB has already been broken
> for so long, maybe we should just not care about SLOB code and
> seriously consider just adding a specific dependency on SLAB|SLUB...

I think it's fair if something like XFS (not meant for tiny systems AFAIK?)
excludes SLOB (meant for tiny systems). Clearly nobody tried to use these
two together last 5 years anyway.
Maybe we could also just add the 4 bytes to all SLOB objects, declare
kfree() is always fine and be done with it. Yes, it will make SLOB footprint
somewhat less tiny, but even whan we added kmalloc power of two alignment
guarantees, the impact on SLOB was negligible.

> Thoughts?
> 
> Cheers,
> 
> Dave.
> 