* [bug report] percpu: Wire up cmpxchg128
@ 2025-04-08 11:03 Dan Carpenter
2025-04-08 11:57 ` Harry Yoo
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2025-04-08 11:03 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: linux-mm
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?
5039 instrument_read_write(__ai_oldp, sizeof(*__ai_oldp)); \
5040 raw_try_cmpxchg128_local(__ai_ptr, __ai_oldp, __VA_ARGS__); \
5041 })
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [bug report] percpu: Wire up cmpxchg128
2025-04-08 11:03 [bug report] percpu: Wire up cmpxchg128 Dan Carpenter
@ 2025-04-08 11:57 ` Harry Yoo
0 siblings, 0 replies; 2+ messages in thread
From: Harry Yoo @ 2025-04-08 11:57 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Peter Zijlstra, linux-mm
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)
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-04-08 11:57 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-04-08 11:03 [bug report] percpu: Wire up cmpxchg128 Dan Carpenter
2025-04-08 11:57 ` Harry Yoo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox