LinuxLists.cc - mm/kfence/kfence_test.c:287 test_alloc() warn: use 'gfp' here instead of GFP

2023-07-27 05:57:07

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

Hi Helge,

FYI, the error/warning
tree: If you fix the issue the same patch/commit), | Reported-by: kernel | Reported-by: Dan | Closes:
was bisected to this commit, please ignore it if it's irrelevant.
git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git">https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
b26f2c54ba7eef80ac5950
5de7594acc67e8f4787ae6 parisc: Enable LOCKDEP support
(https://download.01.org/0day-ci/archive/20230727/[email protected]/config)
(GCC) 12.3.0
s://download.01.org/0day-ci/archive/20230727/202307270305.L19EfaJD-lkp@intel.com/reproduce)">https://download.01.org/0day-ci/archive/20230727/[email protected]/reproduce)
in a separate patch/commit (i.e. not just a new version of
kindly add following tags
test robot <[email protected]>
Carpenter <[email protected]>
//lore.kernel.org/r/202307270305.L19EfaJD-lkp@intel.com/">https://lore.kernel.org/r/[email protected]/
test_alloc() warn: use 'gfp' here instead of GFP_KERNEL?
for the LOLs)
test_double_free() error: double free of 'expect.addr'
test_memcache_typesafe_by_rcu() error: dereferencing freed memory 'expect.addr'
fence_test.c
2021-02-25 243 static void *test_alloc(struct kunit *test, size_t size, gfp_t gfp, enum allocation_policy policy)
2021-02-25 244 {
2021-02-25 245 void *alloc;
2021-02-25 246 unsigned long timeout, resched_after;
2021-02-25 247 const char *policy_name;
2021-02-25 248
2021-02-25 249 switch (policy) {
2021-02-25 250 case ALLOCATE_ANY:
2021-02-25 251 policy_name = "any";
2021-02-25 252 break;
2021-02-25 253 case ALLOCATE_LEFT:
2021-02-25 254 policy_name = "left";
2021-02-25 255 break;
2021-02-25 256 case ALLOCATE_RIGHT:
2021-02-25 257 policy_name = "right";
2021-02-25 258 break;
2021-02-25 259 case ALLOCATE_NONE:
2021-02-25 260 policy_name = "none";
2021-02-25 261 break;
2021-02-25 262 }
2021-02-25 263
2021-02-25 264 kunit_info(test, "%s: size=%zu, gfp=%x, policy=%s, cache=%i\n", __func__, size, gfp,
2021-02-25 265 policy_name, !!test_cache);
2021-02-25 266
2021-02-25 267 /*
2021-02-25 268 * 100x the sample interval should be more than enough to ensure we get
2021-02-25 269 * a KFENCE allocation eventually.
2021-02-25 270 */
2022-02-11 271 timeout = jiffies + msecs_to_jiffies(100 * kfence_sample_interval);
2021-02-25 272 /*
2021-02-25 273 * Especially for non-preemption kernels, ensure the allocation-gate
2021-02-25 274 * timer can catch up: after @resched_after, every failed allocation
2021-02-25 275 * attempt yields, to ensure the allocation-gate timer is scheduled.
2021-02-25 276 */
2022-02-11 277 resched_after = jiffies + msecs_to_jiffies(kfence_sample_interval);
2021-02-25 278 do {
2021-02-25 279 if (test_cache)
2021-02-25 280 alloc = kmem_cache_alloc(test_cache, gfp);
2021-02-25 281 else
2021-02-25 282 alloc = kmalloc(size, gfp);
^^^
2021-02-25 283
2021-02-25 284 if (is_kfence_address(alloc)) {
Babka 2021-11-03 285 struct slab *slab = virt_to_slab(alloc);
2021-06-28 286 struct kmem_cache *s = test_cache ?:
2021-06-28 @287 kmalloc_caches[kmalloc_type(GFP_KERNEL)][__kmalloc_index(size, false)];
^^^^^^^^^^
gfp might be correct but I'm not sure? This code
Let's add Marco to the CC.
2021-02-25 288
2021-02-25 289 /*
2021-02-25 290 * Verify that various helpers return the right values
2021-02-25 291 * even for KFENCE objects; these are required so that
2021-02-25 292 * memcg accounting works correctly.
2021-02-25 293 */
Babka 2021-11-03 294 KUNIT_EXPECT_EQ(test, obj_to_index(s, slab, alloc), 0U);
Babka 2021-11-03 295 KUNIT_EXPECT_EQ(test, objs_per_slab(s, slab), 1);
2021-02-25 296
2021-02-25 297 if (policy == ALLOCATE_ANY)
2021-02-25 298 return alloc;
2022-05-20 299 if (policy == ALLOCATE_LEFT && PAGE_ALIGNED(alloc))
2021-02-25 300 return alloc;
2022-05-20 301 if (policy == ALLOCATE_RIGHT && !PAGE_ALIGNED(alloc))
2021-02-25 302 return alloc;
2021-02-25 303 } else if (policy == ALLOCATE_NONE)
2021-02-25 304 return alloc;
2021-02-25 305
2021-02-25 306 test_free(alloc);
2021-02-25 307
2021-02-25 308 if (time_after(jiffies, resched_after))
2021-02-25 309 cond_resched();
2021-02-25 310 } while (time_before(jiffies, timeout));
2021-02-25 311
2021-02-25 312 KUNIT_ASSERT_TRUE_MSG(test, false, "failed to allocate from KFENCE");
2021-02-25 313 return NULL; /* Unreachable. */
2021-02-25 314 }
om/intel/lkp-tests/wiki">https://github.com/intel/lkp-tests/wiki

2023-07-27 09:00:07

by Marco Elver

[permalink] [raw]

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

On Thu, 27 Jul 2023 at 06:51, Dan Carpenter <[email protected]> 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);