* mm/kfence/kfence_test.c:287 test_alloc() warn: use 'gfp' here instead of GFP_KERNEL?
@ 2023-07-27 4:51 Dan Carpenter
2023-07-27 5:23 ` Helge Deller
2023-07-27 8:06 ` Marco Elver
0 siblings, 2 replies; 4+ messages in thread
From: Dan Carpenter @ 2023-07-27 4:51 UTC (permalink / raw)
To: oe-kbuild, Helge Deller, Marco Elver; +Cc: lkp, oe-kbuild-all, linux-kernel
Hi Helge,
FYI, the error/warning was bisected to this commit, please ignore it if it's irrelevant.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: 18b44bc5a67275641fb26f2c54ba7eef80ac5950
commit: adf8e96a7ea670d45b5de7594acc67e8f4787ae6 parisc: Enable LOCKDEP support
config: parisc-randconfig-m041-20230726 (https://download.01.org/0day-ci/archive/20230727/202307270305.L19EfaJD-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230727/202307270305.L19EfaJD-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202307270305.L19EfaJD-lkp@intel.com/
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'
vim +/gfp +287 mm/kfence/kfence_test.c
bc8fbc5f305aec Marco Elver 2021-02-25 243 static void *test_alloc(struct kunit *test, size_t size, gfp_t gfp, enum allocation_policy policy)
bc8fbc5f305aec Marco Elver 2021-02-25 244 {
bc8fbc5f305aec Marco Elver 2021-02-25 245 void *alloc;
bc8fbc5f305aec Marco Elver 2021-02-25 246 unsigned long timeout, resched_after;
bc8fbc5f305aec Marco Elver 2021-02-25 247 const char *policy_name;
bc8fbc5f305aec Marco Elver 2021-02-25 248
bc8fbc5f305aec Marco Elver 2021-02-25 249 switch (policy) {
bc8fbc5f305aec Marco Elver 2021-02-25 250 case ALLOCATE_ANY:
bc8fbc5f305aec Marco Elver 2021-02-25 251 policy_name = "any";
bc8fbc5f305aec Marco Elver 2021-02-25 252 break;
bc8fbc5f305aec Marco Elver 2021-02-25 253 case ALLOCATE_LEFT:
bc8fbc5f305aec Marco Elver 2021-02-25 254 policy_name = "left";
bc8fbc5f305aec Marco Elver 2021-02-25 255 break;
bc8fbc5f305aec Marco Elver 2021-02-25 256 case ALLOCATE_RIGHT:
bc8fbc5f305aec Marco Elver 2021-02-25 257 policy_name = "right";
bc8fbc5f305aec Marco Elver 2021-02-25 258 break;
bc8fbc5f305aec Marco Elver 2021-02-25 259 case ALLOCATE_NONE:
bc8fbc5f305aec Marco Elver 2021-02-25 260 policy_name = "none";
bc8fbc5f305aec Marco Elver 2021-02-25 261 break;
bc8fbc5f305aec Marco Elver 2021-02-25 262 }
bc8fbc5f305aec Marco Elver 2021-02-25 263
bc8fbc5f305aec Marco Elver 2021-02-25 264 kunit_info(test, "%s: size=%zu, gfp=%x, policy=%s, cache=%i\n", __func__, size, gfp,
bc8fbc5f305aec Marco Elver 2021-02-25 265 policy_name, !!test_cache);
bc8fbc5f305aec Marco Elver 2021-02-25 266
bc8fbc5f305aec Marco Elver 2021-02-25 267 /*
bc8fbc5f305aec Marco Elver 2021-02-25 268 * 100x the sample interval should be more than enough to ensure we get
bc8fbc5f305aec Marco Elver 2021-02-25 269 * a KFENCE allocation eventually.
bc8fbc5f305aec Marco Elver 2021-02-25 270 */
8913c610014823 Peng Liu 2022-02-11 271 timeout = jiffies + msecs_to_jiffies(100 * kfence_sample_interval);
bc8fbc5f305aec Marco Elver 2021-02-25 272 /*
bc8fbc5f305aec Marco Elver 2021-02-25 273 * Especially for non-preemption kernels, ensure the allocation-gate
bc8fbc5f305aec Marco Elver 2021-02-25 274 * timer can catch up: after @resched_after, every failed allocation
bc8fbc5f305aec Marco Elver 2021-02-25 275 * attempt yields, to ensure the allocation-gate timer is scheduled.
bc8fbc5f305aec Marco Elver 2021-02-25 276 */
8913c610014823 Peng Liu 2022-02-11 277 resched_after = jiffies + msecs_to_jiffies(kfence_sample_interval);
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.
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);
bc8fbc5f305aec Marco Elver 2021-02-25 296
bc8fbc5f305aec Marco Elver 2021-02-25 297 if (policy == ALLOCATE_ANY)
bc8fbc5f305aec Marco Elver 2021-02-25 298 return alloc;
f403f22f8ccb12 Kefeng Wang 2022-05-20 299 if (policy == ALLOCATE_LEFT && PAGE_ALIGNED(alloc))
bc8fbc5f305aec Marco Elver 2021-02-25 300 return alloc;
f403f22f8ccb12 Kefeng Wang 2022-05-20 301 if (policy == ALLOCATE_RIGHT && !PAGE_ALIGNED(alloc))
bc8fbc5f305aec Marco Elver 2021-02-25 302 return alloc;
bc8fbc5f305aec Marco Elver 2021-02-25 303 } else if (policy == ALLOCATE_NONE)
bc8fbc5f305aec Marco Elver 2021-02-25 304 return alloc;
bc8fbc5f305aec Marco Elver 2021-02-25 305
bc8fbc5f305aec Marco Elver 2021-02-25 306 test_free(alloc);
bc8fbc5f305aec Marco Elver 2021-02-25 307
bc8fbc5f305aec Marco Elver 2021-02-25 308 if (time_after(jiffies, resched_after))
bc8fbc5f305aec Marco Elver 2021-02-25 309 cond_resched();
bc8fbc5f305aec Marco Elver 2021-02-25 310 } while (time_before(jiffies, timeout));
bc8fbc5f305aec Marco Elver 2021-02-25 311
bc8fbc5f305aec Marco Elver 2021-02-25 312 KUNIT_ASSERT_TRUE_MSG(test, false, "failed to allocate from KFENCE");
bc8fbc5f305aec Marco Elver 2021-02-25 313 return NULL; /* Unreachable. */
bc8fbc5f305aec Marco Elver 2021-02-25 314 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: mm/kfence/kfence_test.c:287 test_alloc() warn: use 'gfp' here instead of GFP_KERNEL?
2023-07-27 4:51 mm/kfence/kfence_test.c:287 test_alloc() warn: use 'gfp' here instead of GFP_KERNEL? Dan Carpenter
@ 2023-07-27 5:23 ` Helge Deller
2023-07-27 5:35 ` Dan Carpenter
2023-07-27 8:06 ` Marco Elver
1 sibling, 1 reply; 4+ messages in thread
From: Helge Deller @ 2023-07-27 5:23 UTC (permalink / raw)
To: Dan Carpenter, oe-kbuild, Marco Elver, Hyeonggon Yoo
Cc: lkp, oe-kbuild-all, linux-kernel
On 7/27/23 06:51, Dan Carpenter wrote:
> Hi Helge,
>
> FYI, the error/warning was bisected to this commit, please ignore it if it's irrelevant.
>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 18b44bc5a67275641fb26f2c54ba7eef80ac5950
> commit: adf8e96a7ea670d45b5de7594acc67e8f4787ae6 parisc: Enable LOCKDEP support
> config: parisc-randconfig-m041-20230726 (https://download.01.org/0day-ci/archive/20230727/202307270305.L19EfaJD-lkp@intel.com/config)
> compiler: hppa-linux-gcc (GCC) 12.3.0
> reproduce: (https://download.01.org/0day-ci/archive/20230727/202307270305.L19EfaJD-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202307270305.L19EfaJD-lkp@intel.com/
>
> 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'
>
> vim +/gfp +287 mm/kfence/kfence_test.c
>
> bc8fbc5f305aec Marco Elver 2021-02-25 243 static void *test_alloc(struct kunit *test, size_t size, gfp_t gfp, enum allocation_policy policy)
> bc8fbc5f305aec Marco Elver 2021-02-25 244 {
> bc8fbc5f305aec Marco Elver 2021-02-25 245 void *alloc;
> bc8fbc5f305aec Marco Elver 2021-02-25 246 unsigned long timeout, resched_after;
> bc8fbc5f305aec Marco Elver 2021-02-25 247 const char *policy_name;
> bc8fbc5f305aec Marco Elver 2021-02-25 248
> bc8fbc5f305aec Marco Elver 2021-02-25 249 switch (policy) {
> bc8fbc5f305aec Marco Elver 2021-02-25 250 case ALLOCATE_ANY:
> bc8fbc5f305aec Marco Elver 2021-02-25 251 policy_name = "any";
> bc8fbc5f305aec Marco Elver 2021-02-25 252 break;
> bc8fbc5f305aec Marco Elver 2021-02-25 253 case ALLOCATE_LEFT:
> bc8fbc5f305aec Marco Elver 2021-02-25 254 policy_name = "left";
> bc8fbc5f305aec Marco Elver 2021-02-25 255 break;
> bc8fbc5f305aec Marco Elver 2021-02-25 256 case ALLOCATE_RIGHT:
> bc8fbc5f305aec Marco Elver 2021-02-25 257 policy_name = "right";
> bc8fbc5f305aec Marco Elver 2021-02-25 258 break;
> bc8fbc5f305aec Marco Elver 2021-02-25 259 case ALLOCATE_NONE:
> bc8fbc5f305aec Marco Elver 2021-02-25 260 policy_name = "none";
> bc8fbc5f305aec Marco Elver 2021-02-25 261 break;
> bc8fbc5f305aec Marco Elver 2021-02-25 262 }
> bc8fbc5f305aec Marco Elver 2021-02-25 263
> bc8fbc5f305aec Marco Elver 2021-02-25 264 kunit_info(test, "%s: size=%zu, gfp=%x, policy=%s, cache=%i\n", __func__, size, gfp,
> bc8fbc5f305aec Marco Elver 2021-02-25 265 policy_name, !!test_cache);
> bc8fbc5f305aec Marco Elver 2021-02-25 266
> bc8fbc5f305aec Marco Elver 2021-02-25 267 /*
> bc8fbc5f305aec Marco Elver 2021-02-25 268 * 100x the sample interval should be more than enough to ensure we get
> bc8fbc5f305aec Marco Elver 2021-02-25 269 * a KFENCE allocation eventually.
> bc8fbc5f305aec Marco Elver 2021-02-25 270 */
> 8913c610014823 Peng Liu 2022-02-11 271 timeout = jiffies + msecs_to_jiffies(100 * kfence_sample_interval);
> bc8fbc5f305aec Marco Elver 2021-02-25 272 /*
> bc8fbc5f305aec Marco Elver 2021-02-25 273 * Especially for non-preemption kernels, ensure the allocation-gate
> bc8fbc5f305aec Marco Elver 2021-02-25 274 * timer can catch up: after @resched_after, every failed allocation
> bc8fbc5f305aec Marco Elver 2021-02-25 275 * attempt yields, to ensure the allocation-gate timer is scheduled.
> bc8fbc5f305aec Marco Elver 2021-02-25 276 */
> 8913c610014823 Peng Liu 2022-02-11 277 resched_after = jiffies + msecs_to_jiffies(kfence_sample_interval);
> 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.
Since this is a test program, I assume that "GFP_KERNEL" is used intentionally
instead of "gfp" here to check if the "kmalloc(size, gfp)" above gets the right kmalloc_caches[].
If so, is there a way to silence the smatch warning ("mm/kfence/kfence_test.c:287 test_alloc() warn: use 'gfp' here instead of GFP_KERNEL?") ?
But I'm not sure either, so adding Hyeonggon to the CC too...
Helge
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: mm/kfence/kfence_test.c:287 test_alloc() warn: use 'gfp' here instead of GFP_KERNEL?
2023-07-27 5:23 ` Helge Deller
@ 2023-07-27 5:35 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2023-07-27 5:35 UTC (permalink / raw)
To: Helge Deller
Cc: oe-kbuild, Marco Elver, Hyeonggon Yoo, lkp, oe-kbuild-all, linux-kernel
On Thu, Jul 27, 2023 at 07:23:23AM +0200, Helge Deller wrote:
> > 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.
>
> Since this is a test program, I assume that "GFP_KERNEL" is used intentionally
> instead of "gfp" here to check if the "kmalloc(size, gfp)" above gets the right kmalloc_caches[].
> If so, is there a way to silence the smatch warning ("mm/kfence/kfence_test.c:287 test_alloc() warn: use 'gfp' here instead of GFP_KERNEL?") ?
> But I'm not sure either, so adding Hyeonggon to the CC too...
Instead of silencing warnings, I prefer to just look at them one time
and move on.
Old warnings are included in the emails if someone touches the code
again. The other double free warnings are an example of this. (Except
that I never have forwarded them before because they're intentional as
part of testing kfence.)
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: mm/kfence/kfence_test.c:287 test_alloc() warn: use 'gfp' here instead of GFP_KERNEL?
2023-07-27 4:51 mm/kfence/kfence_test.c:287 test_alloc() warn: use 'gfp' here instead of GFP_KERNEL? Dan Carpenter
2023-07-27 5:23 ` Helge Deller
@ 2023-07-27 8:06 ` Marco Elver
1 sibling, 0 replies; 4+ messages in thread
From: Marco Elver @ 2023-07-27 8:06 UTC (permalink / raw)
To: Dan Carpenter; +Cc: oe-kbuild, Helge Deller, lkp, oe-kbuild-all, linux-kernel
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);
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-07-27 8:15 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-27 4:51 mm/kfence/kfence_test.c:287 test_alloc() warn: use 'gfp' here instead of GFP_KERNEL? Dan Carpenter
2023-07-27 5:23 ` Helge Deller
2023-07-27 5:35 ` Dan Carpenter
2023-07-27 8:06 ` Marco Elver
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®