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

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

[zhidao su]

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.

Signed-off-by: zhidao su <suzhidao@xiaomi.com>
---
 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);
 				mutex_unlock(&helper_mutex);
 				return -ENOMEM;
 			}
-- 
2.43.0

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

[Andrea Righi]

Hi,

On Fri, Mar 06, 2026 at 06:59:01PM +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.
> 

There should be a:

Fixes: b06ccbabe2506 ("sched_ext: Fix starvation of scx_enable() under fair-class saturation")

Apart than that LGTM.

Acked-by: Andrea Righi <arighi@nvidia.com>

Thanks,
-Andrea

> Signed-off-by: zhidao su <suzhidao@xiaomi.com>
> ---
>  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);
>  				mutex_unlock(&helper_mutex);
>  				return -ENOMEM;
>  			}
> -- 
> 2.43.0
> 

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

[zhidao su]

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);
 				mutex_unlock(&helper_mutex);
 				return -ENOMEM;
 			}
-- 
2.43.0

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

[Tejun Heo]

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

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

[zhidao su]

scx_enable() uses double-checked locking to lazily initialize a static
kthread_worker pointer. The fast path reads helper locklessly:

    if (!READ_ONCE(helper)) {          // lockless read -- no helper_mutex

The write side initializes helper under helper_mutex, but previously
used a plain assignment:

        helper = kthread_run_worker(0, "scx_enable_helper");
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                 plain write -- KCSAN data race with READ_ONCE() above

Since READ_ONCE() on the fast path and the plain write on the
initialization path access the same variable without a common lock,
they constitute a data race. KCSAN requires that all sides of a
lock-free access use READ_ONCE()/WRITE_ONCE() consistently.

Use a temporary variable to stage the result of kthread_run_worker(),
and only WRITE_ONCE() into helper after confirming the pointer is
valid. This avoids a window where a concurrent caller on the fast path
could observe an ERR pointer via READ_ONCE(helper) before the error
check completes.

Fixes: b06ccbabe250 ("sched_ext: Fix starvation of scx_enable() under fair-class saturation")
Signed-off-by: zhidao su <suzhidao@xiaomi.com>
---
v3: Use a temporary variable to stage kthread_run_worker() result;
    only write to helper after confirming validity, eliminating the
    window where an ERR pointer could be observed via the lockless
    fast path (Tejun Heo)
v2: Add missing Fixes: tag (Andrea Righi)
---
 kernel/sched/ext.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c
index 9a1471ad5ae7..63dc7cf1e2a7 100644
--- a/kernel/sched/ext.c
+++ b/kernel/sched/ext.c
@@ -5355,13 +5355,14 @@ 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");
-			if (IS_ERR_OR_NULL(helper)) {
-				helper = NULL;
+			struct kthread_worker *w =
+				kthread_run_worker(0, "scx_enable_helper");
+			if (IS_ERR_OR_NULL(w)) {
 				mutex_unlock(&helper_mutex);
 				return -ENOMEM;
 			}
-			sched_set_fifo(helper->task);
+			sched_set_fifo(w->task);
+			WRITE_ONCE(helper, w);
 		}
 		mutex_unlock(&helper_mutex);
 	}
-- 
2.43.0

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

[zhidao su]

Tested v3 on a KCSAN-enabled kernel (CONFIG_KCSAN=y, non-strict mode)
built from this tree and booted via virtme-ng:

  Kernel: 7.0.0-rc2
  CPUs: 4 (QEMU/KVM)
  Memory: 4G

All 28 sched_ext selftests pass:

  PASSED:  28
  SKIPPED: 0
  FAILED:  0

No KCSAN reports related to the scx_enable helper pointer were
observed during the test run.

The fix (staging kthread_run_worker() result in a local variable and
only calling WRITE_ONCE(helper, w) after confirming validity) correctly
closes the window where a concurrent lockless reader could observe an
ERR pointer via READ_ONCE(helper).

zhidao su

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

[Tejun Heo]

> sched_ext: Use WRITE_ONCE() for the write side of scx_enable helper pointer

Applied to sched_ext/for-7.0-fixes.

Thanks.

--
tejun