linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
@ 2021-09-29 21:23 Rustam Kovhaev
  2021-09-30  4:42 ` Dave Chinner
  0 siblings, 1 reply; 14+ messages in thread
From: Rustam Kovhaev @ 2021-09-29 21:23 UTC (permalink / raw)
  To: djwong, linux-xfs, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka
  Cc: linux-kernel, linux-mm, gregkh, Rustam Kovhaev

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>
---
 fs/xfs/xfs_extfree_item.c | 6 +++---
 mm/slob.c                 | 6 ++++--
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 3f8a0713573a..a4b8caa2c601 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -482,7 +482,7 @@ xfs_extent_free_finish_item(
 			free->xefi_startblock,
 			free->xefi_blockcount,
 			&free->xefi_oinfo, free->xefi_skip_discard);
-	kmem_free(free);
+	kmem_cache_free(xfs_bmap_free_item_zone, free);
 	return error;
 }
 
@@ -502,7 +502,7 @@ xfs_extent_free_cancel_item(
 	struct xfs_extent_free_item	*free;
 
 	free = container_of(item, struct xfs_extent_free_item, xefi_list);
-	kmem_free(free);
+	kmem_cache_free(xfs_bmap_free_item_zone, free);
 }
 
 const struct xfs_defer_op_type xfs_extent_free_defer_type = {
@@ -564,7 +564,7 @@ xfs_agfl_free_finish_item(
 	extp->ext_len = free->xefi_blockcount;
 	efdp->efd_next_extent++;
 
-	kmem_free(free);
+	kmem_cache_free(xfs_bmap_free_item_zone, free);
 	return error;
 }
 
diff --git a/mm/slob.c b/mm/slob.c
index 74d3f6e60666..d2d859ded5f8 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -389,7 +389,6 @@ static void slob_free(void *block, int size)
 
 	if (unlikely(ZERO_OR_NULL_PTR(block)))
 		return;
-	BUG_ON(!size);
 
 	sp = virt_to_page(block);
 	units = SLOB_UNITS(size);
@@ -556,6 +555,7 @@ void kfree(const void *block)
 	if (PageSlab(sp)) {
 		int align = max_t(size_t, ARCH_KMALLOC_MINALIGN, ARCH_SLAB_MINALIGN);
 		unsigned int *m = (unsigned int *)(block - align);
+		BUG_ON(!*m || *m > (PAGE_SIZE - align));
 		slob_free(m, *m + align);
 	} else {
 		unsigned int order = compound_order(sp);
@@ -649,8 +649,10 @@ EXPORT_SYMBOL(kmem_cache_alloc_node);
 
 static void __kmem_cache_free(void *b, int size)
 {
-	if (size < PAGE_SIZE)
+	if (size < PAGE_SIZE) {
+		BUG_ON(!size);
 		slob_free(b, size);
+	}
 	else
 		slob_free_pages(b, get_order(size));
 }
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
  2021-09-29 21:23 [PATCH] xfs: use kmem_cache_free() for kmem_cache objects Rustam Kovhaev
@ 2021-09-30  4:42 ` Dave Chinner
  2021-09-30  8:13   ` Vlastimil Babka
  0 siblings, 1 reply; 14+ messages in thread
From: Dave Chinner @ 2021-09-30  4:42 UTC (permalink / raw)
  To: Rustam Kovhaev
  Cc: djwong, linux-xfs, cl, penberg, rientjes, iamjoonsoo.kim, akpm,
	vbabka, linux-kernel, linux-mm, gregkh

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...

Thoughts?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
  2021-09-30  4:42 ` Dave Chinner
@ 2021-09-30  8:13   ` Vlastimil Babka
  2021-09-30 18:48     ` Rustam Kovhaev
  0 siblings, 1 reply; 14+ messages in thread
From: Vlastimil Babka @ 2021-09-30  8:13 UTC (permalink / raw)
  To: Dave Chinner, Rustam Kovhaev
  Cc: djwong, linux-xfs, cl, penberg, rientjes, iamjoonsoo.kim, akpm,
	linux-kernel, linux-mm, gregkh, Al Viro

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.
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
  2021-09-30  8:13   ` Vlastimil Babka
@ 2021-09-30 18:48     ` Rustam Kovhaev
  2021-09-30 21:10       ` Vlastimil Babka
  0 siblings, 1 reply; 14+ messages in thread
From: Rustam Kovhaev @ 2021-09-30 18:48 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Dave Chinner, djwong, linux-xfs, cl, penberg, rientjes,
	iamjoonsoo.kim, akpm, linux-kernel, linux-mm, gregkh, Al Viro

On Thu, Sep 30, 2021 at 10:13:40AM +0200, Vlastimil Babka wrote:
> 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.

+1 for adding Kconfig option, it seems like some things are not meant to
be together.

> 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.

I'll send a patch to add a 4-byte header for kmem_cache_alloc()
allocations.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
  2021-09-30 18:48     ` Rustam Kovhaev
@ 2021-09-30 21:10       ` Vlastimil Babka
  2021-10-01  0:32         ` Rustam Kovhaev
  0 siblings, 1 reply; 14+ messages in thread
From: Vlastimil Babka @ 2021-09-30 21:10 UTC (permalink / raw)
  To: Rustam Kovhaev
  Cc: Dave Chinner, djwong, linux-xfs, cl, penberg, rientjes,
	iamjoonsoo.kim, akpm, linux-kernel, linux-mm, gregkh, Al Viro

On 9/30/21 8:48 PM, Rustam Kovhaev wrote:
> On Thu, Sep 30, 2021 at 10:13:40AM +0200, Vlastimil Babka wrote:
>>
>> 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.
> 
> +1 for adding Kconfig option, it seems like some things are not meant to
> be together.

But if we patch SLOB, we won't need it.

>> 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.
> 
> I'll send a patch to add a 4-byte header for kmem_cache_alloc()
> allocations.

Thanks. Please report in the changelog slab usage from /proc/meminfo
before and after patch (at least a snapshot after a full boot).

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
  2021-09-30 21:10       ` Vlastimil Babka
@ 2021-10-01  0:32         ` Rustam Kovhaev
  2021-10-04  1:07           ` David Rientjes
  0 siblings, 1 reply; 14+ messages in thread
From: Rustam Kovhaev @ 2021-10-01  0:32 UTC (permalink / raw)
  To: Vlastimil Babka, Dave Chinner
  Cc: djwong, linux-xfs, cl, penberg, rientjes, iamjoonsoo.kim, akpm,
	linux-kernel, linux-mm, gregkh, Al Viro, dvyukov

On Thu, Sep 30, 2021 at 11:10:10PM +0200, Vlastimil Babka wrote:
> On 9/30/21 8:48 PM, Rustam Kovhaev wrote:
> > On Thu, Sep 30, 2021 at 10:13:40AM +0200, Vlastimil Babka wrote:
> >>
> >> 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.
> > 
> > +1 for adding Kconfig option, it seems like some things are not meant to
> > be together.
> 
> But if we patch SLOB, we won't need it.

OK, so we consider XFS on SLOB a supported configuration that might be
used and should be tested.
I'll look into maybe adding a config with CONFIG_SLOB and CONFIG_XFS_FS
to syzbot.

It seems that we need to patch SLOB anyway, because any other code can
hit the very same issue.

> >> 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.
> > 
> > I'll send a patch to add a 4-byte header for kmem_cache_alloc()
> > allocations.
> 
> Thanks. Please report in the changelog slab usage from /proc/meminfo
> before and after patch (at least a snapshot after a full boot).

OK.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
  2021-10-01  0:32         ` Rustam Kovhaev
@ 2021-10-04  1:07           ` David Rientjes
  2021-10-12 20:43             ` Darrick J. Wong
  0 siblings, 1 reply; 14+ messages in thread
From: David Rientjes @ 2021-10-04  1:07 UTC (permalink / raw)
  To: Rustam Kovhaev
  Cc: Vlastimil Babka, Dave Chinner, djwong, linux-xfs, cl, penberg,
	iamjoonsoo.kim, akpm, linux-kernel, linux-mm, gregkh, Al Viro,
	dvyukov

On Thu, 30 Sep 2021, Rustam Kovhaev wrote:

> > >> 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.
> > > 
> > > +1 for adding Kconfig option, it seems like some things are not meant to
> > > be together.
> > 
> > But if we patch SLOB, we won't need it.
> 
> OK, so we consider XFS on SLOB a supported configuration that might be
> used and should be tested.
> I'll look into maybe adding a config with CONFIG_SLOB and CONFIG_XFS_FS
> to syzbot.
> 
> It seems that we need to patch SLOB anyway, because any other code can
> hit the very same issue.
> 

It's probably best to introduce both (SLOB fix and Kconfig change for 
XFS), at least in the interim because the combo of XFS and SLOB could be 
broken in other ways.  If syzbot doesn't complain with a patched kernel to 
allow SLOB to be used with XFS, then we could potentially allow them to be 
used together.

(I'm not sure that this freeing issue is the *only* thing that is broken, 
nor that we have sufficient information to make that determination right 
now..)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
  2021-10-04  1:07           ` David Rientjes
@ 2021-10-12 20:43             ` Darrick J. Wong
  2021-10-12 20:43               ` Darrick J. Wong
  0 siblings, 1 reply; 14+ messages in thread
From: Darrick J. Wong @ 2021-10-12 20:43 UTC (permalink / raw)
  To: David Rientjes
  Cc: Rustam Kovhaev, Vlastimil Babka, Dave Chinner, linux-xfs, cl,
	penberg, iamjoonsoo.kim, akpm, linux-kernel, linux-mm, gregkh,
	Al Viro, dvyukov

On Sun, Oct 03, 2021 at 06:07:20PM -0700, David Rientjes wrote:
> On Thu, 30 Sep 2021, Rustam Kovhaev wrote:
> 
> > > >> 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.
> > > > 
> > > > +1 for adding Kconfig option, it seems like some things are not meant to
> > > > be together.
> > > 
> > > But if we patch SLOB, we won't need it.
> > 
> > OK, so we consider XFS on SLOB a supported configuration that might be
> > used and should be tested.
> > I'll look into maybe adding a config with CONFIG_SLOB and CONFIG_XFS_FS
> > to syzbot.
> > 
> > It seems that we need to patch SLOB anyway, because any other code can
> > hit the very same issue.
> > 
> 
> It's probably best to introduce both (SLOB fix and Kconfig change for 
> XFS), at least in the interim because the combo of XFS and SLOB could be 
> broken in other ways.  If syzbot doesn't complain with a patched kernel to 
> allow SLOB to be used with XFS, then we could potentially allow them to be 
> used together.
> 
> (I'm not sure that this freeing issue is the *only* thing that is broken, 
> nor that we have sufficient information to make that determination right 
> now..)

I audited the entire xfs (kernel) codebase and didn't find any other
usage errors.  Thanks for the patch; I'll apply it to for-next.

--D

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
  2021-10-12 20:43             ` Darrick J. Wong
@ 2021-10-12 20:43               ` Darrick J. Wong
  2021-10-12 21:32                 ` Vlastimil Babka
  0 siblings, 1 reply; 14+ messages in thread
From: Darrick J. Wong @ 2021-10-12 20:43 UTC (permalink / raw)
  To: David Rientjes
  Cc: Rustam Kovhaev, Vlastimil Babka, Dave Chinner, linux-xfs, cl,
	penberg, iamjoonsoo.kim, akpm, linux-kernel, linux-mm, gregkh,
	Al Viro, dvyukov

On Tue, Oct 12, 2021 at 01:43:20PM -0700, Darrick J. Wong wrote:
> On Sun, Oct 03, 2021 at 06:07:20PM -0700, David Rientjes wrote:
> > On Thu, 30 Sep 2021, Rustam Kovhaev wrote:
> > 
> > > > >> 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.
> > > > > 
> > > > > +1 for adding Kconfig option, it seems like some things are not meant to
> > > > > be together.
> > > > 
> > > > But if we patch SLOB, we won't need it.
> > > 
> > > OK, so we consider XFS on SLOB a supported configuration that might be
> > > used and should be tested.
> > > I'll look into maybe adding a config with CONFIG_SLOB and CONFIG_XFS_FS
> > > to syzbot.
> > > 
> > > It seems that we need to patch SLOB anyway, because any other code can
> > > hit the very same issue.
> > > 
> > 
> > It's probably best to introduce both (SLOB fix and Kconfig change for 
> > XFS), at least in the interim because the combo of XFS and SLOB could be 
> > broken in other ways.  If syzbot doesn't complain with a patched kernel to 
> > allow SLOB to be used with XFS, then we could potentially allow them to be 
> > used together.
> > 
> > (I'm not sure that this freeing issue is the *only* thing that is broken, 
> > nor that we have sufficient information to make that determination right 
> > now..)
> 
> I audited the entire xfs (kernel) codebase and didn't find any other
> usage errors.  Thanks for the patch; I'll apply it to for-next.

Also, the obligatory

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> 
> --D

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
  2021-10-12 20:43               ` Darrick J. Wong
@ 2021-10-12 21:32                 ` Vlastimil Babka
  2021-10-12 23:22                   ` Darrick J. Wong
  0 siblings, 1 reply; 14+ messages in thread
From: Vlastimil Babka @ 2021-10-12 21:32 UTC (permalink / raw)
  To: Darrick J. Wong, David Rientjes
  Cc: Rustam Kovhaev, Dave Chinner, linux-xfs, cl, penberg,
	iamjoonsoo.kim, akpm, linux-kernel, linux-mm, gregkh, Al Viro,
	dvyukov

On 10/12/2021 10:43 PM, Darrick J. Wong wrote:
> On Tue, Oct 12, 2021 at 01:43:20PM -0700, Darrick J. Wong wrote:
>> On Sun, Oct 03, 2021 at 06:07:20PM -0700, David Rientjes wrote:
>>> On Thu, 30 Sep 2021, Rustam Kovhaev wrote:
>>>
>>>>>>> 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.
>>>>>>
>>>>>> +1 for adding Kconfig option, it seems like some things are not meant to
>>>>>> be together.
>>>>>
>>>>> But if we patch SLOB, we won't need it.
>>>>
>>>> OK, so we consider XFS on SLOB a supported configuration that might be
>>>> used and should be tested.
>>>> I'll look into maybe adding a config with CONFIG_SLOB and CONFIG_XFS_FS
>>>> to syzbot.
>>>>
>>>> It seems that we need to patch SLOB anyway, because any other code can
>>>> hit the very same issue.
>>>>
>>>
>>> It's probably best to introduce both (SLOB fix and Kconfig change for 
>>> XFS), at least in the interim because the combo of XFS and SLOB could be 
>>> broken in other ways.  If syzbot doesn't complain with a patched kernel to 
>>> allow SLOB to be used with XFS, then we could potentially allow them to be 
>>> used together.
>>>
>>> (I'm not sure that this freeing issue is the *only* thing that is broken, 
>>> nor that we have sufficient information to make that determination right 
>>> now..)
>>
>> I audited the entire xfs (kernel) codebase and didn't find any other
>> usage errors.  Thanks for the patch; I'll apply it to for-next.

Which patch, the one that started this thread and uses kmem_cache_free() instead
of kfree()? I thought we said it's not the best way?

> Also, the obligatory
> 
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> --D
> 
>>
>> --D


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
  2021-10-12 21:32                 ` Vlastimil Babka
@ 2021-10-12 23:22                   ` Darrick J. Wong
  2021-10-13  7:38                     ` Vlastimil Babka
  0 siblings, 1 reply; 14+ messages in thread
From: Darrick J. Wong @ 2021-10-12 23:22 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: David Rientjes, Rustam Kovhaev, Dave Chinner, linux-xfs, cl,
	penberg, iamjoonsoo.kim, akpm, linux-kernel, linux-mm, gregkh,
	Al Viro, dvyukov

On Tue, Oct 12, 2021 at 11:32:25PM +0200, Vlastimil Babka wrote:
> On 10/12/2021 10:43 PM, Darrick J. Wong wrote:
> > On Tue, Oct 12, 2021 at 01:43:20PM -0700, Darrick J. Wong wrote:
> >> On Sun, Oct 03, 2021 at 06:07:20PM -0700, David Rientjes wrote:
> >>> On Thu, 30 Sep 2021, Rustam Kovhaev wrote:
> >>>
> >>>>>>> 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.
> >>>>>>
> >>>>>> +1 for adding Kconfig option, it seems like some things are not meant to
> >>>>>> be together.
> >>>>>
> >>>>> But if we patch SLOB, we won't need it.
> >>>>
> >>>> OK, so we consider XFS on SLOB a supported configuration that might be
> >>>> used and should be tested.
> >>>> I'll look into maybe adding a config with CONFIG_SLOB and CONFIG_XFS_FS
> >>>> to syzbot.
> >>>>
> >>>> It seems that we need to patch SLOB anyway, because any other code can
> >>>> hit the very same issue.
> >>>>
> >>>
> >>> It's probably best to introduce both (SLOB fix and Kconfig change for 
> >>> XFS), at least in the interim because the combo of XFS and SLOB could be 
> >>> broken in other ways.  If syzbot doesn't complain with a patched kernel to 
> >>> allow SLOB to be used with XFS, then we could potentially allow them to be 
> >>> used together.
> >>>
> >>> (I'm not sure that this freeing issue is the *only* thing that is broken, 
> >>> nor that we have sufficient information to make that determination right 
> >>> now..)
> >>
> >> I audited the entire xfs (kernel) codebase and didn't find any other
> >> usage errors.  Thanks for the patch; I'll apply it to for-next.
> 
> Which patch, the one that started this thread and uses kmem_cache_free() instead
> of kfree()? I thought we said it's not the best way?

It's probably better to fix slob to be able to tell that a kmem_free'd
object actually belongs to a cache and should get freed that way, just
like its larger sl[ua]b cousins.

However, even if that does come to pass, anybody /else/ who wants to
start(?) using XFS on a SLOB system will need this patch to fix the
minor papercut.  Now that I've checked the rest of the codebase, I don't
find it reasonable to make XFS mutually exclusive with SLOB over two
instances of slab cache misuse.  Hence the RVB. :)

--D

> > Also, the obligatory
> > 
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > 
> > --D
> > 
> >>
> >> --D
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
  2021-10-12 23:22                   ` Darrick J. Wong
@ 2021-10-13  7:38                     ` Vlastimil Babka
  2021-10-13 16:56                       ` Rustam Kovhaev
  0 siblings, 1 reply; 14+ messages in thread
From: Vlastimil Babka @ 2021-10-13  7:38 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: David Rientjes, Rustam Kovhaev, Dave Chinner, linux-xfs, cl,
	penberg, iamjoonsoo.kim, akpm, linux-kernel, linux-mm, gregkh,
	Al Viro, dvyukov

On 10/13/21 01:22, Darrick J. Wong wrote:
> On Tue, Oct 12, 2021 at 11:32:25PM +0200, Vlastimil Babka wrote:
>> On 10/12/2021 10:43 PM, Darrick J. Wong wrote:
>> > On Tue, Oct 12, 2021 at 01:43:20PM -0700, Darrick J. Wong wrote:
>> >> On Sun, Oct 03, 2021 at 06:07:20PM -0700, David Rientjes wrote:
>> >>
>> >> I audited the entire xfs (kernel) codebase and didn't find any other
>> >> usage errors.  Thanks for the patch; I'll apply it to for-next.
>> 
>> Which patch, the one that started this thread and uses kmem_cache_free() instead
>> of kfree()? I thought we said it's not the best way?
> 
> It's probably better to fix slob to be able to tell that a kmem_free'd
> object actually belongs to a cache and should get freed that way, just
> like its larger sl[ua]b cousins.

Agreed. Rustam, do you still plan to do that?

> However, even if that does come to pass, anybody /else/ who wants to
> start(?) using XFS on a SLOB system will need this patch to fix the
> minor papercut.  Now that I've checked the rest of the codebase, I don't
> find it reasonable to make XFS mutually exclusive with SLOB over two
> instances of slab cache misuse.  Hence the RVB. :)

Ok. I was just wondering because Dave's first reply was that actually you'll
need to expand the use of kfree() instead of kmem_cache_free().


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
  2021-10-13  7:38                     ` Vlastimil Babka
@ 2021-10-13 16:56                       ` Rustam Kovhaev
  2021-10-15  0:57                         ` Darrick J. Wong
  0 siblings, 1 reply; 14+ messages in thread
From: Rustam Kovhaev @ 2021-10-13 16:56 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Darrick J. Wong, David Rientjes, Dave Chinner, linux-xfs, cl,
	penberg, iamjoonsoo.kim, akpm, linux-kernel, linux-mm, gregkh,
	Al Viro, dvyukov

On Wed, Oct 13, 2021 at 09:38:31AM +0200, Vlastimil Babka wrote:
> On 10/13/21 01:22, Darrick J. Wong wrote:
> > On Tue, Oct 12, 2021 at 11:32:25PM +0200, Vlastimil Babka wrote:
> >> On 10/12/2021 10:43 PM, Darrick J. Wong wrote:
> >> > On Tue, Oct 12, 2021 at 01:43:20PM -0700, Darrick J. Wong wrote:
> >> >> On Sun, Oct 03, 2021 at 06:07:20PM -0700, David Rientjes wrote:
> >> >>
> >> >> I audited the entire xfs (kernel) codebase and didn't find any other
> >> >> usage errors.  Thanks for the patch; I'll apply it to for-next.
> >> 
> >> Which patch, the one that started this thread and uses kmem_cache_free() instead
> >> of kfree()? I thought we said it's not the best way?
> > 
> > It's probably better to fix slob to be able to tell that a kmem_free'd
> > object actually belongs to a cache and should get freed that way, just
> > like its larger sl[ua]b cousins.
> 
> Agreed. Rustam, do you still plan to do that?

Yes, I do, thank you.

> 
> > However, even if that does come to pass, anybody /else/ who wants to
> > start(?) using XFS on a SLOB system will need this patch to fix the
> > minor papercut.  Now that I've checked the rest of the codebase, I don't
> > find it reasonable to make XFS mutually exclusive with SLOB over two
> > instances of slab cache misuse.  Hence the RVB. :)
> 
> Ok. I was just wondering because Dave's first reply was that actually you'll
> need to expand the use of kfree() instead of kmem_cache_free().
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfs: use kmem_cache_free() for kmem_cache objects
  2021-10-13 16:56                       ` Rustam Kovhaev
@ 2021-10-15  0:57                         ` Darrick J. Wong
  0 siblings, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2021-10-15  0:57 UTC (permalink / raw)
  To: Rustam Kovhaev
  Cc: Vlastimil Babka, David Rientjes, Dave Chinner, linux-xfs, cl,
	penberg, iamjoonsoo.kim, akpm, linux-kernel, linux-mm, gregkh,
	Al Viro, dvyukov

On Wed, Oct 13, 2021 at 09:56:41AM -0700, Rustam Kovhaev wrote:
> On Wed, Oct 13, 2021 at 09:38:31AM +0200, Vlastimil Babka wrote:
> > On 10/13/21 01:22, Darrick J. Wong wrote:
> > > On Tue, Oct 12, 2021 at 11:32:25PM +0200, Vlastimil Babka wrote:
> > >> On 10/12/2021 10:43 PM, Darrick J. Wong wrote:
> > >> > On Tue, Oct 12, 2021 at 01:43:20PM -0700, Darrick J. Wong wrote:
> > >> >> On Sun, Oct 03, 2021 at 06:07:20PM -0700, David Rientjes wrote:
> > >> >>
> > >> >> I audited the entire xfs (kernel) codebase and didn't find any other
> > >> >> usage errors.  Thanks for the patch; I'll apply it to for-next.
> > >> 
> > >> Which patch, the one that started this thread and uses kmem_cache_free() instead
> > >> of kfree()? I thought we said it's not the best way?
> > > 
> > > It's probably better to fix slob to be able to tell that a kmem_free'd
> > > object actually belongs to a cache and should get freed that way, just
> > > like its larger sl[ua]b cousins.
> > 
> > Agreed. Rustam, do you still plan to do that?
> 
> Yes, I do, thank you.

Note that I left out the parts of the patch that changed mm/slob.c
because I didn't think that was appropriate for a patch titled 'xfs:'.

> 
> > 
> > > However, even if that does come to pass, anybody /else/ who wants to
> > > start(?) using XFS on a SLOB system will need this patch to fix the
> > > minor papercut.  Now that I've checked the rest of the codebase, I don't
> > > find it reasonable to make XFS mutually exclusive with SLOB over two
> > > instances of slab cache misuse.  Hence the RVB. :)
> > 
> > Ok. I was just wondering because Dave's first reply was that actually you'll
> > need to expand the use of kfree() instead of kmem_cache_free().

I look forward to doing this, but since XFS is a downstream consumer of
the kmem apis, we'll have to wait until the slob changes land to do
that.

--D

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-10-15  0:57 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-29 21:23 [PATCH] xfs: use kmem_cache_free() for kmem_cache objects Rustam Kovhaev
2021-09-30  4:42 ` Dave Chinner
2021-09-30  8:13   ` Vlastimil Babka
2021-09-30 18:48     ` Rustam Kovhaev
2021-09-30 21:10       ` Vlastimil Babka
2021-10-01  0:32         ` Rustam Kovhaev
2021-10-04  1:07           ` David Rientjes
2021-10-12 20:43             ` Darrick J. Wong
2021-10-12 20:43               ` Darrick J. Wong
2021-10-12 21:32                 ` Vlastimil Babka
2021-10-12 23:22                   ` Darrick J. Wong
2021-10-13  7:38                     ` Vlastimil Babka
2021-10-13 16:56                       ` Rustam Kovhaev
2021-10-15  0:57                         ` Darrick J. Wong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).