Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <9e72b7ea-9598-415d-bc55-f2f42b7fcb3b@kadam.mountain>
In-Reply-To: <9e72b7ea-9598-415d-bc55-f2f42b7fcb3b@kadam.mountain>
From:   Marco Elver <elver@google.com>
Date:   Thu, 27 Jul 2023 10:06:25 +0200
Message-ID: <CANpmjNMWqcseZAijhVP_TDORXxBTgj0GWbTzq5ive2e+0Xto+A@mail.gmail.com>
Subject: Re: mm/kfence/kfence_test.c:287 test_alloc() warn: use 'gfp' here
 instead of GFP_KERNEL?
To:     Dan Carpenter <dan.carpenter@linaro.org>
Cc:     oe-kbuild@lists.linux.dev, Helge Deller <deller@gmx.de>,
        lkp@intel.com, oe-kbuild-all@lists.linux.dev,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 27 Jul 2023 at 06:51, Dan Carpenter <dan.carpenter@linaro.org> 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);