Re: [bug report] percpu: Wire up cmpxchg128

[Harry Yoo <harry.yoo@oracle.com>]

On Tue, Apr 08, 2025 at 02:03:11PM +0300, Dan Carpenter wrote:
> Hello Peter Zijlstra,
> 
> This code is old, and so it must work.  I can't explain why the warning
> is only showing up now.  I was just curious what's going on.
> 
> Commit 6d12c8d308e6 ("percpu: Wire up cmpxchg128") from May 31, 2023
> (linux-next), leads to the following Smatch static checker warning:
> 
> 	mm/slub.c:3609 __update_cpu_freelist_fast()
> 	warn: sizeof(void)
> 
> mm/slub.c
>     3599 
>     3600 #ifndef CONFIG_SLUB_TINY
>     3601 static inline bool
>     3602 __update_cpu_freelist_fast(struct kmem_cache *s,
>     3603                            void *freelist_old, void *freelist_new,
>     3604                            unsigned long tid)
>     3605 {
>     3606         freelist_aba_t old = { .freelist = freelist_old, .counter = tid };
>     3607         freelist_aba_t new = { .freelist = freelist_new, .counter = next_tid(tid) };
>     3608 
> --> 3609         return this_cpu_try_cmpxchg_freelist(s->cpu_slab->freelist_tid.full,
>     3610                                              &old.full, new.full);
>     3611 }
> 
> The problem is:
> 
> arch/arm64/include/asm/percpu.h
>    239  #define this_cpu_cmpxchg128(pcp, o, n)                                  \
>    240  ({                                                                      \
>    241          typedef typeof(pcp) pcp_op_T__;                                 \
>    242          u128 old__, new__, ret__;                                       \
>    243          pcp_op_T__ *ptr__;                                              \
>    244          old__ = o;                                                      \
>    245          new__ = n;                                                      \
>    246          preempt_disable_notrace();                                      \
>    247          ptr__ = raw_cpu_ptr(&(pcp));                                    \
>    248          ret__ = cmpxchg128_local((void *)ptr__, old__, new__);          \
>                                          ^^^^^^^^^^^^^^
> We're passing a void pointer.
> 
>    249          preempt_enable_notrace();                                       \
>    250          ret__;                                                          \
>    251  })
> 
> include/linux/atomic/atomic-instrumented.h
>   5034  #define try_cmpxchg128_local(ptr, oldp, ...) \
>   5035  ({ \
>   5036          typeof(ptr) __ai_ptr = (ptr); \
>   5037          typeof(oldp) __ai_oldp = (oldp); \
>   5038          instrument_atomic_read_write(__ai_ptr, sizeof(*__ai_ptr)); \
>                                                        ^^^^^^^^^^^^^^^^^
> Here we're doing a sizeof() of a void pointer which is 1.  I think maybe
> this is just for KASAN so it's not a big deal in terms of affecting
> runtime?

As far as I can tell,

instrument_* helpers are used to let KASAN/KCSAN/KMSAN check if there are
bugs (e.g., KASAN checks shadow memory and determine if this read or write
is a memory access violation).

When enabled, usually the compiler automatically inserts those checks into
the kernel binary. But those helpers are used where the compiler can't.

I argue that not being able to precisely catch memory errors for
cmpxchg128 users (SLUB and few drivers) has not been a critical
problem.

But it'd be better to fix it :)

> 
>   5039          instrument_read_write(__ai_oldp, sizeof(*__ai_oldp)); \
>   5040          raw_try_cmpxchg128_local(__ai_ptr, __ai_oldp, __VA_ARGS__); \
>   5041  })
> 
> 
> regards,
> dan carpenter
> 

-- 
Cheers,
Harry (formerly known as Hyeonggon)