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/[email protected]/config)
> compiler: hppa-linux-gcc (GCC) 12.3.0
> reproduce: (https://download.01.org/0day-ci/archive/20230727/[email protected]/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 <[email protected]>
> | Reported-by: Dan Carpenter <[email protected]>
> | Closes: https://lore.kernel.org/r/[email protected]/
>
> 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
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