Re: [PATCH v2] sched_ext: Use WRITE_ONCE() for the write side of scx_enable helper pointer

[Tejun Heo <tj@kernel.org>]

On Fri, Mar 06, 2026 at 10:17:18PM +0800, zhidao su wrote:
> scx_enable() uses double-checked locking to lazily initialize a static
> kthread_worker pointer:
> 
>     if (!READ_ONCE(helper)) {
>         mutex_lock(&helper_mutex);
>         if (!helper) {
>             helper = kthread_run_worker(0, "scx_enable_helper");
>                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                      plain write -- KCSAN data race
> 
> The outer READ_ONCE() annotates the lockless fast-path read, but the
> write side uses a plain assignment without the matching WRITE_ONCE().
> The KCSAN documentation requires that if one accessor uses READ_ONCE()
> or WRITE_ONCE() on a variable to annotate lock-free access, all other
> accesses must also use the appropriate accessor. A plain write leaves
> the pair incomplete and will trigger KCSAN warnings.
> 
> The error path also has the same issue:
> 
>             helper = NULL;
>             ^^^^^^^^^^
>             plain write -- KCSAN data race
> 
> Fix both plain writes by using WRITE_ONCE() to complete the concurrent
> access annotation and make the code KCSAN-clean.
> 
> Fixes: b06ccbabe250 ("sched_ext: Fix starvation of scx_enable() under fair-class saturation")
> Signed-off-by: zhidao su <suzhidao@xiaomi.com>
> ---
> v2: Add missing Fixes: tag (Andrea Righi)
> ---
>  kernel/sched/ext.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c
> index 9a1471ad5ae7..c4ccd685259f 100644
> --- a/kernel/sched/ext.c
> +++ b/kernel/sched/ext.c
> @@ -5355,9 +5355,9 @@ static int scx_enable(struct sched_ext_ops *ops, struct bpf_link *link)
>  	if (!READ_ONCE(helper)) {
>  		mutex_lock(&helper_mutex);
>  		if (!helper) {
> -			helper = kthread_run_worker(0, "scx_enable_helper");
> +			WRITE_ONCE(helper, kthread_run_worker(0, "scx_enable_helper"));
>  			if (IS_ERR_OR_NULL(helper)) {
> -				helper = NULL;
> +				WRITE_ONCE(helper, NULL);

I think this is racy. Another enable instance can race and read an ERR value
and try to use it as a pointer. Can you add a temporary variable to hold the
returned kworker pointer so that it only writes to helper iff it's valid.

Thanks.

-- 
tejun