[PATCH -next] mm: kence: add __kmem_cache_free to function skip list

[Feng Tang]

When testing the linux-next kernel, kfence's kunit test reported some
errors:

[ 12.812412] not ok 7 - test_double_free
[ 13.011968] not ok 9 - test_invalid_addr_free
[ 13.438947] not ok 11 - test_corruption
[ 18.635647] not ok 18 - test_kmalloc_aligned_oob_write

Further check shows there is the "common kmalloc" patchset from
Hyeonggon Yoo, which cleanup the kmalloc code and make a better
sharing of slab/slub. There is some function name change around it,
which was not recognized by current kfence function name handling
code, and interpreted as error.

Add new function name "__kmem_cache_free" to make it known to kfence.

Signed-off-by: Feng Tang <feng...@intel.com>
---
mm/kfence/report.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index f5a6d8ba3e21..7e496856c2eb 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -86,6 +86,7 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
/* Also the *_bulk() variants by only checking prefixes. */
if (str_has_prefix(buf, ARCH_FUNC_PREFIX "kfree") ||
str_has_prefix(buf, ARCH_FUNC_PREFIX "kmem_cache_free") ||
+ str_has_prefix(buf, ARCH_FUNC_PREFIX "__kmem_cache_free") ||
str_has_prefix(buf, ARCH_FUNC_PREFIX "__kmalloc") ||
str_has_prefix(buf, ARCH_FUNC_PREFIX "kmem_cache_alloc"))
goto found;
--
2.27.0

[Marco Elver]

On Wed, 31 Aug 2022 at 09:30, Feng Tang <feng...@intel.com> wrote:
>
> When testing the linux-next kernel, kfence's kunit test reported some
> errors:
>
> [ 12.812412] not ok 7 - test_double_free
> [ 13.011968] not ok 9 - test_invalid_addr_free
> [ 13.438947] not ok 11 - test_corruption
> [ 18.635647] not ok 18 - test_kmalloc_aligned_oob_write
>
> Further check shows there is the "common kmalloc" patchset from
> Hyeonggon Yoo, which cleanup the kmalloc code and make a better
> sharing of slab/slub. There is some function name change around it,
> which was not recognized by current kfence function name handling
> code, and interpreted as error.
>
> Add new function name "__kmem_cache_free" to make it known to kfence.
>
> Signed-off-by: Feng Tang <feng...@intel.com>

Reviewed-by: Marco Elver <el...@google.com>

Thank you for catching this.

> ---
> mm/kfence/report.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> index f5a6d8ba3e21..7e496856c2eb 100644
> --- a/mm/kfence/report.c
> +++ b/mm/kfence/report.c
> @@ -86,6 +86,7 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
> /* Also the *_bulk() variants by only checking prefixes. */
> if (str_has_prefix(buf, ARCH_FUNC_PREFIX "kfree") ||
> str_has_prefix(buf, ARCH_FUNC_PREFIX "kmem_cache_free") ||
> + str_has_prefix(buf, ARCH_FUNC_PREFIX "__kmem_cache_free") ||
> str_has_prefix(buf, ARCH_FUNC_PREFIX "__kmalloc") ||
> str_has_prefix(buf, ARCH_FUNC_PREFIX "kmem_cache_alloc"))
> goto found;
> --
> 2.27.0
>

> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20220831073051.3032-1-feng.tang%40intel.com.

[Vlastimil Babka]

On 8/31/2022 9:35 AM, Marco Elver wrote:
> On Wed, 31 Aug 2022 at 09:30, Feng Tang <feng...@intel.com> wrote:
>>
>> When testing the linux-next kernel, kfence's kunit test reported some
>> errors:
>>
>> [ 12.812412] not ok 7 - test_double_free
>> [ 13.011968] not ok 9 - test_invalid_addr_free
>> [ 13.438947] not ok 11 - test_corruption
>> [ 18.635647] not ok 18 - test_kmalloc_aligned_oob_write
>>
>> Further check shows there is the "common kmalloc" patchset from
>> Hyeonggon Yoo, which cleanup the kmalloc code and make a better
>> sharing of slab/slub. There is some function name change around it,
>> which was not recognized by current kfence function name handling
>> code, and interpreted as error.
>>
>> Add new function name "__kmem_cache_free" to make it known to kfence.
>>
>> Signed-off-by: Feng Tang <feng...@intel.com>
>
> Reviewed-by: Marco Elver <el...@google.com>
>
> Thank you for catching this.

Thanks, will incorporate there.

[Hyeonggon Yoo]

Thank you for catching this!

Unfortunately not reproducible on my environment with linux-next (IDK why).

Maybe you can include those functions too?

- __kmem_cache_alloc_node
- kmalloc_[node_]trace, kmalloc_large[_node]

--
Thanks,
Hyeonggon

[Marco Elver]

On Wed, 31 Aug 2022 at 16:04, Hyeonggon Yoo <42.h...@gmail.com> wrote:

> Maybe you can include those functions too?
>
> - __kmem_cache_alloc_node
> - kmalloc_[node_]trace, kmalloc_large[_node]

This is only required if they are allocator "root" functions when
entering allocator code (or may be tail called by a allocator "root"
function). Because get_stack_skipnr() looks for one of the listed
function prefixes in the whole stack trace.

The reason __kmem_cache_free() is now required is because it is tail
called by kfree() which disappears from the stack trace if the
compiler does tail-call-optimization.

[Feng Tang]

Hi Hyeonggon,

Maybe it's about the kernel config, or gcc version?

The head commit of next tree I tested is at:

7fd22855300e [Stephen Rothwell] Add linux-next specific files for 20220831 Wed Aug 31 15:48:36 2022 +1000

My gcc version is: "gcc (Ubuntu 10.3.0-1ubuntu1~20.10) 10.3.0"

I also attached the kernel config and the dmesg log which has the kfence
unit text info (the kernel is boot in a qemu with debian rootfs).

> Maybe you can include those functions too?
>
> - __kmem_cache_alloc_node
> - kmalloc_[node_]trace, kmalloc_large[_node]

Though I threw the patch out, I have to admit I know very little about kfence
internals, will leave it to kfence developers :) (I saw Macro Elver already
replied).

Thanks,
Feng


> --
> Thanks,
> Hyeonggon

[Vlastimil Babka]

I checked and I have this jmp tail call, yet all test pass here.
But I assume the right commit to amend is
05a1c2e50809 ("mm/sl[au]b: generalize kmalloc subsystem")

Could you Feng maybe verify that that commit is the first that fails the
tests, and parent commit of that is OK? Thanks.

[Feng Tang]

Yes, 05a1c2e50809 is the first commit that I saw the 4 kfence failed
kunit cases.

Thanks,
Feng

[Vlastimil Babka]

Thanks, squashed your patch there and pushed new for-next.

> Thanks,
> Feng
>

[Feng Tang]

Thanks! Just re-pulled slab tree's 'for-next' branch and the error
can't be reproduced with it.

- Feng