Re: mm/kfence/kfence_test.c:287 test_alloc() warn: use 'gfp' here instead of GFP_KERNEL?

[Marco Elver]

On Thu, 27 Jul 2023 at 06:51, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
[...]
> smatch warnings:
> mm/kfence/kfence_test.c:287 test_alloc() warn: use 'gfp' here instead of GFP_KERNEL?
>
> (Just included these for the LOLs)
> mm/kfence/kfence_test.c:395 test_double_free() error: double free of 'expect.addr'
> mm/kfence/kfence_test.c:671 test_memcache_typesafe_by_rcu() error: dereferencing freed memory 'expect.addr'

Nice! ;-)

> vim +/gfp +287 mm/kfence/kfence_test.c
>
[...]
> bc8fbc5f305aec Marco Elver     2021-02-25  278          do {
> bc8fbc5f305aec Marco Elver     2021-02-25  279                  if (test_cache)
> bc8fbc5f305aec Marco Elver     2021-02-25  280                          alloc = kmem_cache_alloc(test_cache, gfp);
> bc8fbc5f305aec Marco Elver     2021-02-25  281                  else
> bc8fbc5f305aec Marco Elver     2021-02-25  282                          alloc = kmalloc(size, gfp);
>                                                                                               ^^^
>
> bc8fbc5f305aec Marco Elver     2021-02-25  283
> bc8fbc5f305aec Marco Elver     2021-02-25  284                  if (is_kfence_address(alloc)) {
> 8dae0cfed57357 Vlastimil Babka 2021-11-03  285                          struct slab *slab = virt_to_slab(alloc);
> 588c7fa022d7b2 Hyeonggon Yoo   2021-06-28  286                          struct kmem_cache *s = test_cache ?:
> 588c7fa022d7b2 Hyeonggon Yoo   2021-06-28 @287                                          kmalloc_caches[kmalloc_type(GFP_KERNEL)][__kmalloc_index(size, false)];
>                                                                                                                     ^^^^^^^^^^
> I feel like using gfp might be correct but I'm not sure?  This code
> is from prior to this commit.  Let's add Marco to the CC.

It's not a bug today: If we were testing something other than
GFP_KERNEL, then yes, we should use gfp here. But the only reason
"gfp" is a function arg at all, is that 1 test case also uses
__GFP_ZERO, but that's irrelevant in getting the kmalloc type (at
least today).

The cache is used to test the below "helpers return the right values
even for KFENCE objects".

I think when I wrote the test I just chose GFP_KERNEL, because none of
the test cases use something else, and didn't give it a second
thought. Not sure it's worth changing, because functionally it doesn't
matter.

I'm open to changing it, if it makes this warning go away. Preferences?

> bc8fbc5f305aec Marco Elver     2021-02-25  288
> bc8fbc5f305aec Marco Elver     2021-02-25  289                          /*
> bc8fbc5f305aec Marco Elver     2021-02-25  290                           * Verify that various helpers return the right values
> bc8fbc5f305aec Marco Elver     2021-02-25  291                           * even for KFENCE objects; these are required so that
> bc8fbc5f305aec Marco Elver     2021-02-25  292                           * memcg accounting works correctly.
> bc8fbc5f305aec Marco Elver     2021-02-25  293                           */
> 8dae0cfed57357 Vlastimil Babka 2021-11-03  294                          KUNIT_EXPECT_EQ(test, obj_to_index(s, slab, alloc), 0U);
> 8dae0cfed57357 Vlastimil Babka 2021-11-03  295                          KUNIT_EXPECT_EQ(test, objs_per_slab(s, slab), 1);