[PATCH] rcutorture: Fix rcutorture_one_extend_check() splat in RT kernels

[PATCH] rcutorture: Fix rcutorture_one_extend_check() splat in RT kernels

[Zqiang]

For built with CONFIG_PREEMPT_RT=y kernels, running rcutorture
tests resulted in the following splat:

[   68.797425] rcutorture_one_extend_check during change: Current 0x1  To add 0x1  To remove 0x0  preempt_count() 0x0
[   68.797533] WARNING: CPU: 2 PID: 512 at kernel/rcu/rcutorture.c:1993 rcutorture_one_extend_check+0x419/0x560 [rcutorture]
[   68.797601] Call Trace:
[   68.797602]  <TASK>
[   68.797619]  ? lockdep_softirqs_off+0xa5/0x160
[   68.797631]  rcutorture_one_extend+0x18e/0xcc0 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
[   68.797646]  ? local_clock+0x19/0x40
[   68.797659]  rcu_torture_one_read+0xf0/0x280 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
[   68.797678]  ? __pfx_rcu_torture_one_read+0x10/0x10 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
[   68.797804]  ? __pfx_rcu_torture_timer+0x10/0x10 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
[   68.797815] rcu-torture: rcu_torture_reader task started
[   68.797824] rcu-torture: Creating rcu_torture_reader task
[   68.797824]  rcu_torture_reader+0x238/0x580 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
[   68.797836]  ? kvm_sched_clock_read+0x15/0x30

Disable BH does not change the SOFTIRQ corresponding bits in
preempt_count() for RT kernels, this commit therefore use
softirq_count() to check the if BH is disabled.

Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
---
 kernel/rcu/rcutorture.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index 373c65a6e103..ef439569f979 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -471,7 +471,7 @@ rcu_read_delay(struct torture_random_state *rrsp, struct rt_read_seg *rtrsp)
 	    !(torture_random(rrsp) % (nrealreaders * 2000 * longdelay_ms))) {
 		started = cur_ops->get_gp_seq();
 		ts = rcu_trace_clock_local();
-		if (preempt_count() & (SOFTIRQ_MASK | HARDIRQ_MASK))
+		if ((preempt_count() & HARDIRQ_MASK) || softirq_count())
 			longdelay_ms = 5; /* Avoid triggering BH limits. */
 		mdelay(longdelay_ms);
 		rtrsp->rt_delay_ms = longdelay_ms;
@@ -1990,7 +1990,7 @@ static void rcutorture_one_extend_check(char *s, int curstate, int new, int old,
 		return;
 
 	WARN_ONCE((curstate & (RCUTORTURE_RDR_BH | RCUTORTURE_RDR_RBH)) &&
-		  !(preempt_count() & SOFTIRQ_MASK), ROEC_ARGS);
+		  !softirq_count(), ROEC_ARGS);
 	WARN_ONCE((curstate & (RCUTORTURE_RDR_PREEMPT | RCUTORTURE_RDR_SCHED)) &&
 		  !(preempt_count() & PREEMPT_MASK), ROEC_ARGS);
 	WARN_ONCE(cur_ops->readlock_nesting &&
@@ -2004,7 +2004,7 @@ static void rcutorture_one_extend_check(char *s, int curstate, int new, int old,
 
 	WARN_ONCE(cur_ops->extendables &&
 		  !(curstate & (RCUTORTURE_RDR_BH | RCUTORTURE_RDR_RBH)) &&
-		  (preempt_count() & SOFTIRQ_MASK), ROEC_ARGS);
+		  softirq_count(), ROEC_ARGS);
 
 	/*
 	 * non-preemptible RCU in a preemptible kernel uses preempt_disable()
@@ -2025,6 +2025,9 @@ static void rcutorture_one_extend_check(char *s, int curstate, int new, int old,
 	if (!IS_ENABLED(CONFIG_PREEMPT_RCU))
 		mask |= RCUTORTURE_RDR_PREEMPT | RCUTORTURE_RDR_SCHED;
 
+	if (IS_ENABLED(CONFIG_PREEMPT_RT) && softirq_count())
+		mask |= RCUTORTURE_RDR_BH | RCUTORTURE_RDR_RBH;
+
 	WARN_ONCE(cur_ops->readlock_nesting && !(curstate & mask) &&
 		  cur_ops->readlock_nesting() > 0, ROEC_ARGS);
 }
-- 
2.17.1

[PATCH v2] rcu/nocb: Add Safe checks for access offloaded rdp

[Zqiang]

For built with CONFIG_PROVE_RCU=y and CONFIG_PREEMPT_RT=y kernels,
Disable BH does not change the SOFTIRQ corresponding bits in
preempt_count(), but change current->softirq_disable_cnt, this
resulted in the following splat:

WARNING: suspicious RCU usage
kernel/rcu/tree_plugin.h:36 Unsafe read of RCU_NOCB offloaded state!
stack backtrace:
CPU: 0 UID: 0 PID: 22 Comm: rcuc/0
Call Trace:
[    0.407907]  <TASK>
[    0.407910]  dump_stack_lvl+0xbb/0xd0
[    0.407917]  dump_stack+0x14/0x20
[    0.407920]  lockdep_rcu_suspicious+0x133/0x210
[    0.407932]  rcu_rdp_is_offloaded+0x1c3/0x270
[    0.407939]  rcu_core+0x471/0x900
[    0.407942]  ? lockdep_hardirqs_on+0xd5/0x160
[    0.407954]  rcu_cpu_kthread+0x25f/0x870
[    0.407959]  ? __pfx_rcu_cpu_kthread+0x10/0x10
[    0.407966]  smpboot_thread_fn+0x34c/0xa50
[    0.407970]  ? trace_preempt_on+0x54/0x120
[    0.407977]  ? __pfx_smpboot_thread_fn+0x10/0x10
[    0.407982]  kthread+0x40e/0x840
[    0.407990]  ? __pfx_kthread+0x10/0x10
[    0.407994]  ? rt_spin_unlock+0x4e/0xb0
[    0.407997]  ? rt_spin_unlock+0x4e/0xb0
[    0.408000]  ? __pfx_kthread+0x10/0x10
[    0.408006]  ? __pfx_kthread+0x10/0x10
[    0.408011]  ret_from_fork+0x40/0x70
[    0.408013]  ? __pfx_kthread+0x10/0x10
[    0.408018]  ret_from_fork_asm+0x1a/0x30
[    0.408042]  </TASK>

Currently, triggering an rdp offloaded state change need the
corresponding rdp's CPU goes offline, and at this time the rcuc
kthreads has already in parking state. this means the corresponding
rcuc kthreads can safely read offloaded state of rdp while it's
corresponding cpu is online.

This commit therefore add softirq_count() check for
Preempt-RT kernels.

Suggested-by: Joel Fernandes <joelagnelf@nvidia.com>
Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
---
 kernel/rcu/tree_plugin.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 003e549f6514..a91b2322a0cd 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -29,7 +29,7 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
 		  (IS_ENABLED(CONFIG_HOTPLUG_CPU) && lockdep_is_cpus_held()) ||
 		  lockdep_is_held(&rdp->nocb_lock) ||
 		  lockdep_is_held(&rcu_state.nocb_mutex) ||
-		  (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) &&
+		  ((!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) || softirq_count()) &&
 		   rdp == this_cpu_ptr(&rcu_data)) ||
 		  rcu_current_is_nocb_kthread(rdp)),
 		"Unsafe read of RCU_NOCB offloaded state"
-- 
2.17.1

[PATCH] rcu/nocb: Fix possible invalid rdp's->nocb_cb_kthread pointer access

[Zqiang]

In the preparation stage of CPU online, if the corresponding
the rdp's->nocb_cb_kthread does not exist, will be created,
there is a situation where the rdp's rcuop kthreads creation fails,
and then de-offload this CPU's rdp, does not assign this CPU's
rdp->nocb_cb_kthread pointer, but this rdp's->nocb_gp_rdp and
rdp's->rdp_gp->nocb_gp_kthread is still valid.

This will cause the subsequent re-offload operation of this offline
CPU, which will pass the conditional check and the kthread_unpark()
will access invalid rdp's->nocb_cb_kthread pointer.

This commit therefore use rdp's->nocb_gp_kthread instead of
rdp_gp's->nocb_gp_kthread for safety check.

Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
---
 kernel/rcu/tree_nocb.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
index 1596812f7f12..6679140bb0b5 100644
--- a/kernel/rcu/tree_nocb.h
+++ b/kernel/rcu/tree_nocb.h
@@ -1146,7 +1146,6 @@ static bool rcu_nocb_rdp_offload_wait_cond(struct rcu_data *rdp)
 static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
 {
 	int wake_gp;
-	struct rcu_data *rdp_gp = rdp->nocb_gp_rdp;
 
 	WARN_ON_ONCE(cpu_online(rdp->cpu));
 	/*
@@ -1156,7 +1155,7 @@ static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
 	if (!rdp->nocb_gp_rdp)
 		return -EINVAL;
 
-	if (WARN_ON_ONCE(!rdp_gp->nocb_gp_kthread))
+	if (WARN_ON_ONCE(!rdp->nocb_gp_kthread))
 		return -EINVAL;
 
 	pr_info("Offloading %d\n", rdp->cpu);
@@ -1166,7 +1165,7 @@ static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
 
 	wake_gp = rcu_nocb_queue_toggle_rdp(rdp);
 	if (wake_gp)
-		wake_up_process(rdp_gp->nocb_gp_kthread);
+		wake_up_process(rdp->nocb_gp_kthread);
 
 	swait_event_exclusive(rdp->nocb_state_wq,
 			      rcu_nocb_rdp_offload_wait_cond(rdp));
-- 
2.17.1

Re: [PATCH v2] rcu/nocb: Add Safe checks for access offloaded rdp

[Joel Fernandes]

On 5/7/2025 7:26 AM, Zqiang wrote:
> For built with CONFIG_PROVE_RCU=y and CONFIG_PREEMPT_RT=y kernels,
> Disable BH does not change the SOFTIRQ corresponding bits in
> preempt_count(), but change current->softirq_disable_cnt, this
> resulted in the following splat:
> 
> WARNING: suspicious RCU usage
> kernel/rcu/tree_plugin.h:36 Unsafe read of RCU_NOCB offloaded state!
> stack backtrace:
> CPU: 0 UID: 0 PID: 22 Comm: rcuc/0
> Call Trace:
> [    0.407907]  <TASK>
> [    0.407910]  dump_stack_lvl+0xbb/0xd0
> [    0.407917]  dump_stack+0x14/0x20
> [    0.407920]  lockdep_rcu_suspicious+0x133/0x210
> [    0.407932]  rcu_rdp_is_offloaded+0x1c3/0x270
> [    0.407939]  rcu_core+0x471/0x900
> [    0.407942]  ? lockdep_hardirqs_on+0xd5/0x160
> [    0.407954]  rcu_cpu_kthread+0x25f/0x870
> [    0.407959]  ? __pfx_rcu_cpu_kthread+0x10/0x10
> [    0.407966]  smpboot_thread_fn+0x34c/0xa50
> [    0.407970]  ? trace_preempt_on+0x54/0x120
> [    0.407977]  ? __pfx_smpboot_thread_fn+0x10/0x10
> [    0.407982]  kthread+0x40e/0x840
> [    0.407990]  ? __pfx_kthread+0x10/0x10
> [    0.407994]  ? rt_spin_unlock+0x4e/0xb0
> [    0.407997]  ? rt_spin_unlock+0x4e/0xb0
> [    0.408000]  ? __pfx_kthread+0x10/0x10
> [    0.408006]  ? __pfx_kthread+0x10/0x10
> [    0.408011]  ret_from_fork+0x40/0x70
> [    0.408013]  ? __pfx_kthread+0x10/0x10
> [    0.408018]  ret_from_fork_asm+0x1a/0x30
> [    0.408042]  </TASK>
> 
> Currently, triggering an rdp offloaded state change need the
> corresponding rdp's CPU goes offline, and at this time the rcuc
> kthreads has already in parking state. this means the corresponding
> rcuc kthreads can safely read offloaded state of rdp while it's
> corresponding cpu is online.
> 
> This commit therefore add softirq_count() check for
> Preempt-RT kernels.
> 
> Suggested-by: Joel Fernandes <joelagnelf@nvidia.com>
> Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> ---
>  kernel/rcu/tree_plugin.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 003e549f6514..a91b2322a0cd 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -29,7 +29,7 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
>  		  (IS_ENABLED(CONFIG_HOTPLUG_CPU) && lockdep_is_cpus_held()) ||
>  		  lockdep_is_held(&rdp->nocb_lock) ||
>  		  lockdep_is_held(&rcu_state.nocb_mutex) ||
> -		  (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) &&
> +		  ((!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) || softirq_count()) &&
>  		   rdp == this_cpu_ptr(&rcu_data)) ||
This looks good to me. Frederic told me he'll further review and give final
green signal. Then I'll pull this particular one.

One thing I was wondering -- it would be really nice if preemptible() itself
checked for softirq_count() by default. Or adding something like a
really_preemptible() which checks for both CONFIG_PREEMPT_COUNT and
softirq_count() along with preemptible().  I feel like this always comes back to
bite us in different ways, and not knowing atomicity complicates various code paths.

Maybe a summer holidays project? ;)

 - Joel

Re: [PATCH v2] rcu/nocb: Add Safe checks for access offloaded rdp

[Frederic Weisbecker]

Le Wed, May 07, 2025 at 07:26:04PM +0800, Zqiang a écrit :
> For built with CONFIG_PROVE_RCU=y and CONFIG_PREEMPT_RT=y kernels,
> Disable BH does not change the SOFTIRQ corresponding bits in
> preempt_count(), but change current->softirq_disable_cnt, this
> resulted in the following splat:
> 
> WARNING: suspicious RCU usage
> kernel/rcu/tree_plugin.h:36 Unsafe read of RCU_NOCB offloaded state!
> stack backtrace:
> CPU: 0 UID: 0 PID: 22 Comm: rcuc/0
> Call Trace:
> [    0.407907]  <TASK>
> [    0.407910]  dump_stack_lvl+0xbb/0xd0
> [    0.407917]  dump_stack+0x14/0x20
> [    0.407920]  lockdep_rcu_suspicious+0x133/0x210
> [    0.407932]  rcu_rdp_is_offloaded+0x1c3/0x270
> [    0.407939]  rcu_core+0x471/0x900
> [    0.407942]  ? lockdep_hardirqs_on+0xd5/0x160
> [    0.407954]  rcu_cpu_kthread+0x25f/0x870
> [    0.407959]  ? __pfx_rcu_cpu_kthread+0x10/0x10
> [    0.407966]  smpboot_thread_fn+0x34c/0xa50
> [    0.407970]  ? trace_preempt_on+0x54/0x120
> [    0.407977]  ? __pfx_smpboot_thread_fn+0x10/0x10
> [    0.407982]  kthread+0x40e/0x840
> [    0.407990]  ? __pfx_kthread+0x10/0x10
> [    0.407994]  ? rt_spin_unlock+0x4e/0xb0
> [    0.407997]  ? rt_spin_unlock+0x4e/0xb0
> [    0.408000]  ? __pfx_kthread+0x10/0x10
> [    0.408006]  ? __pfx_kthread+0x10/0x10
> [    0.408011]  ret_from_fork+0x40/0x70
> [    0.408013]  ? __pfx_kthread+0x10/0x10
> [    0.408018]  ret_from_fork_asm+0x1a/0x30
> [    0.408042]  </TASK>
> 
> Currently, triggering an rdp offloaded state change need the
> corresponding rdp's CPU goes offline, and at this time the rcuc
> kthreads has already in parking state. this means the corresponding
> rcuc kthreads can safely read offloaded state of rdp while it's
> corresponding cpu is online.
> 
> This commit therefore add softirq_count() check for
> Preempt-RT kernels.
> 
> Suggested-by: Joel Fernandes <joelagnelf@nvidia.com>
> Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> ---
>  kernel/rcu/tree_plugin.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 003e549f6514..a91b2322a0cd 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -29,7 +29,7 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
>  		  (IS_ENABLED(CONFIG_HOTPLUG_CPU) && lockdep_is_cpus_held()) ||
>  		  lockdep_is_held(&rdp->nocb_lock) ||
>  		  lockdep_is_held(&rcu_state.nocb_mutex) ||
> -		  (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) &&
> +		  ((!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) || softirq_count()) &&
>  		   rdp == this_cpu_ptr(&rcu_data)) ||

On a second thought, isn't "rdp == this_cpu_ptr(&rcu_data)" enough?
The offloaded state can only change if the CPU is completely offline.
But if the current CPU is looking at the local rdp, it means it is online
and the rdp can't be concurrently [de]offloaded, right?

Thanks.

>  		  rcu_current_is_nocb_kthread(rdp)),
>  		"Unsafe read of RCU_NOCB offloaded state"
> -- 
> 2.17.1
> 
> 

-- 
Frederic Weisbecker
SUSE Labs

Re: [PATCH v2] rcu/nocb: Add Safe checks for access offloaded rdp

[Frederic Weisbecker]

Le Wed, May 07, 2025 at 12:06:29PM -0400, Joel Fernandes a écrit :
> 
> 
> On 5/7/2025 7:26 AM, Zqiang wrote:
> > For built with CONFIG_PROVE_RCU=y and CONFIG_PREEMPT_RT=y kernels,
> > Disable BH does not change the SOFTIRQ corresponding bits in
> > preempt_count(), but change current->softirq_disable_cnt, this
> > resulted in the following splat:
> > 
> > WARNING: suspicious RCU usage
> > kernel/rcu/tree_plugin.h:36 Unsafe read of RCU_NOCB offloaded state!
> > stack backtrace:
> > CPU: 0 UID: 0 PID: 22 Comm: rcuc/0
> > Call Trace:
> > [    0.407907]  <TASK>
> > [    0.407910]  dump_stack_lvl+0xbb/0xd0
> > [    0.407917]  dump_stack+0x14/0x20
> > [    0.407920]  lockdep_rcu_suspicious+0x133/0x210
> > [    0.407932]  rcu_rdp_is_offloaded+0x1c3/0x270
> > [    0.407939]  rcu_core+0x471/0x900
> > [    0.407942]  ? lockdep_hardirqs_on+0xd5/0x160
> > [    0.407954]  rcu_cpu_kthread+0x25f/0x870
> > [    0.407959]  ? __pfx_rcu_cpu_kthread+0x10/0x10
> > [    0.407966]  smpboot_thread_fn+0x34c/0xa50
> > [    0.407970]  ? trace_preempt_on+0x54/0x120
> > [    0.407977]  ? __pfx_smpboot_thread_fn+0x10/0x10
> > [    0.407982]  kthread+0x40e/0x840
> > [    0.407990]  ? __pfx_kthread+0x10/0x10
> > [    0.407994]  ? rt_spin_unlock+0x4e/0xb0
> > [    0.407997]  ? rt_spin_unlock+0x4e/0xb0
> > [    0.408000]  ? __pfx_kthread+0x10/0x10
> > [    0.408006]  ? __pfx_kthread+0x10/0x10
> > [    0.408011]  ret_from_fork+0x40/0x70
> > [    0.408013]  ? __pfx_kthread+0x10/0x10
> > [    0.408018]  ret_from_fork_asm+0x1a/0x30
> > [    0.408042]  </TASK>
> > 
> > Currently, triggering an rdp offloaded state change need the
> > corresponding rdp's CPU goes offline, and at this time the rcuc
> > kthreads has already in parking state. this means the corresponding
> > rcuc kthreads can safely read offloaded state of rdp while it's
> > corresponding cpu is online.
> > 
> > This commit therefore add softirq_count() check for
> > Preempt-RT kernels.
> > 
> > Suggested-by: Joel Fernandes <joelagnelf@nvidia.com>
> > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> > ---
> >  kernel/rcu/tree_plugin.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index 003e549f6514..a91b2322a0cd 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -29,7 +29,7 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
> >  		  (IS_ENABLED(CONFIG_HOTPLUG_CPU) && lockdep_is_cpus_held()) ||
> >  		  lockdep_is_held(&rdp->nocb_lock) ||
> >  		  lockdep_is_held(&rcu_state.nocb_mutex) ||
> > -		  (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) &&
> > +		  ((!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) || softirq_count()) &&
> >  		   rdp == this_cpu_ptr(&rcu_data)) ||
> This looks good to me. Frederic told me he'll further review and give final
> green signal. Then I'll pull this particular one.
> 
> One thing I was wondering -- it would be really nice if preemptible() itself
> checked for softirq_count() by default. Or adding something like a
> really_preemptible() which checks for both CONFIG_PREEMPT_COUNT and
> softirq_count() along with preemptible().  I feel like this always comes back to
> bite us in different ways, and not knowing atomicity complicates various code paths.
> 
> Maybe a summer holidays project? ;)

I thought about that too but I think this is semantically incorrect.
In PREEMPT_RT, softirqs are actually preemptible.

Thanks.

> 
>  - Joel
> 

-- 
Frederic Weisbecker
SUSE Labs

Re: [PATCH v2] rcu/nocb: Add Safe checks for access offloaded rdp

[Joel Fernandes]

On 5/7/2025 12:31 PM, Frederic Weisbecker wrote:
> Le Wed, May 07, 2025 at 12:06:29PM -0400, Joel Fernandes a écrit :
>>
>>
>> On 5/7/2025 7:26 AM, Zqiang wrote:
>>> For built with CONFIG_PROVE_RCU=y and CONFIG_PREEMPT_RT=y kernels,
>>> Disable BH does not change the SOFTIRQ corresponding bits in
>>> preempt_count(), but change current->softirq_disable_cnt, this
>>> resulted in the following splat:
>>>
>>> WARNING: suspicious RCU usage
>>> kernel/rcu/tree_plugin.h:36 Unsafe read of RCU_NOCB offloaded state!
>>> stack backtrace:
>>> CPU: 0 UID: 0 PID: 22 Comm: rcuc/0
>>> Call Trace:
>>> [    0.407907]  <TASK>
>>> [    0.407910]  dump_stack_lvl+0xbb/0xd0
>>> [    0.407917]  dump_stack+0x14/0x20
>>> [    0.407920]  lockdep_rcu_suspicious+0x133/0x210
>>> [    0.407932]  rcu_rdp_is_offloaded+0x1c3/0x270
>>> [    0.407939]  rcu_core+0x471/0x900
>>> [    0.407942]  ? lockdep_hardirqs_on+0xd5/0x160
>>> [    0.407954]  rcu_cpu_kthread+0x25f/0x870
>>> [    0.407959]  ? __pfx_rcu_cpu_kthread+0x10/0x10
>>> [    0.407966]  smpboot_thread_fn+0x34c/0xa50
>>> [    0.407970]  ? trace_preempt_on+0x54/0x120
>>> [    0.407977]  ? __pfx_smpboot_thread_fn+0x10/0x10
>>> [    0.407982]  kthread+0x40e/0x840
>>> [    0.407990]  ? __pfx_kthread+0x10/0x10
>>> [    0.407994]  ? rt_spin_unlock+0x4e/0xb0
>>> [    0.407997]  ? rt_spin_unlock+0x4e/0xb0
>>> [    0.408000]  ? __pfx_kthread+0x10/0x10
>>> [    0.408006]  ? __pfx_kthread+0x10/0x10
>>> [    0.408011]  ret_from_fork+0x40/0x70
>>> [    0.408013]  ? __pfx_kthread+0x10/0x10
>>> [    0.408018]  ret_from_fork_asm+0x1a/0x30
>>> [    0.408042]  </TASK>
>>>
>>> Currently, triggering an rdp offloaded state change need the
>>> corresponding rdp's CPU goes offline, and at this time the rcuc
>>> kthreads has already in parking state. this means the corresponding
>>> rcuc kthreads can safely read offloaded state of rdp while it's
>>> corresponding cpu is online.
>>>
>>> This commit therefore add softirq_count() check for
>>> Preempt-RT kernels.
>>>
>>> Suggested-by: Joel Fernandes <joelagnelf@nvidia.com>
>>> Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
>>> ---
>>>  kernel/rcu/tree_plugin.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
>>> index 003e549f6514..a91b2322a0cd 100644
>>> --- a/kernel/rcu/tree_plugin.h
>>> +++ b/kernel/rcu/tree_plugin.h
>>> @@ -29,7 +29,7 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
>>>  		  (IS_ENABLED(CONFIG_HOTPLUG_CPU) && lockdep_is_cpus_held()) ||
>>>  		  lockdep_is_held(&rdp->nocb_lock) ||
>>>  		  lockdep_is_held(&rcu_state.nocb_mutex) ||
>>> -		  (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) &&
>>> +		  ((!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) || softirq_count()) &&
>>>  		   rdp == this_cpu_ptr(&rcu_data)) ||
>> This looks good to me. Frederic told me he'll further review and give final
>> green signal. Then I'll pull this particular one.
>>
>> One thing I was wondering -- it would be really nice if preemptible() itself
>> checked for softirq_count() by default. Or adding something like a
>> really_preemptible() which checks for both CONFIG_PREEMPT_COUNT and
>> softirq_count() along with preemptible().  I feel like this always comes back to
>> bite us in different ways, and not knowing atomicity complicates various code paths.
>>
>> Maybe a summer holidays project? ;)
> 
> I thought about that too but I think this is semantically incorrect.
> In PREEMPT_RT, softirqs are actually preemptible.
You are right, for this usecase it may not be that helpful. However, I do find
it odd that the caller has to check CONFIG_PREEMPT_COUNT before calling
preemptible() above, that should be implemented in the API itself.

I was more broadly referring to the recurring problem of "am I in atomic"
context or "can I be preempted" or "can I sleep", if so do this otherwise do
something else. For instance calling GFP_KERNEL vs GFP_ATOMIC in the memory
allocator. Though sleeping and preemption are perhaps separate concerns. We ran
into this during the kfree_rcu() early days as well.

IIRC we have perhaps too many APIs that do similar things like in_atomic() which
confuses everyone (I don't fully know the current state of all such APIs). Add
PREEMPT_RT to the mix, and now we have more complications. :-/

thanks,

 - Joel

Re: [PATCH] rcutorture: Fix rcutorture_one_extend_check() splat in RT kernels

[Paul E. McKenney]

On Wed, May 07, 2025 at 07:26:03PM +0800, Zqiang wrote:
> For built with CONFIG_PREEMPT_RT=y kernels, running rcutorture
> tests resulted in the following splat:
> 
> [   68.797425] rcutorture_one_extend_check during change: Current 0x1  To add 0x1  To remove 0x0  preempt_count() 0x0
> [   68.797533] WARNING: CPU: 2 PID: 512 at kernel/rcu/rcutorture.c:1993 rcutorture_one_extend_check+0x419/0x560 [rcutorture]
> [   68.797601] Call Trace:
> [   68.797602]  <TASK>
> [   68.797619]  ? lockdep_softirqs_off+0xa5/0x160
> [   68.797631]  rcutorture_one_extend+0x18e/0xcc0 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
> [   68.797646]  ? local_clock+0x19/0x40
> [   68.797659]  rcu_torture_one_read+0xf0/0x280 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
> [   68.797678]  ? __pfx_rcu_torture_one_read+0x10/0x10 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
> [   68.797804]  ? __pfx_rcu_torture_timer+0x10/0x10 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
> [   68.797815] rcu-torture: rcu_torture_reader task started
> [   68.797824] rcu-torture: Creating rcu_torture_reader task
> [   68.797824]  rcu_torture_reader+0x238/0x580 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
> [   68.797836]  ? kvm_sched_clock_read+0x15/0x30
> 
> Disable BH does not change the SOFTIRQ corresponding bits in
> preempt_count() for RT kernels, this commit therefore use
> softirq_count() to check the if BH is disabled.
> 
> Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> ---
>  kernel/rcu/rcutorture.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> index 373c65a6e103..ef439569f979 100644
> --- a/kernel/rcu/rcutorture.c
> +++ b/kernel/rcu/rcutorture.c
> @@ -471,7 +471,7 @@ rcu_read_delay(struct torture_random_state *rrsp, struct rt_read_seg *rtrsp)
>  	    !(torture_random(rrsp) % (nrealreaders * 2000 * longdelay_ms))) {
>  		started = cur_ops->get_gp_seq();
>  		ts = rcu_trace_clock_local();
> -		if (preempt_count() & (SOFTIRQ_MASK | HARDIRQ_MASK))
> +		if ((preempt_count() & HARDIRQ_MASK) || softirq_count())
>  			longdelay_ms = 5; /* Avoid triggering BH limits. */
>  		mdelay(longdelay_ms);
>  		rtrsp->rt_delay_ms = longdelay_ms;
> @@ -1990,7 +1990,7 @@ static void rcutorture_one_extend_check(char *s, int curstate, int new, int old,
>  		return;
>  
>  	WARN_ONCE((curstate & (RCUTORTURE_RDR_BH | RCUTORTURE_RDR_RBH)) &&
> -		  !(preempt_count() & SOFTIRQ_MASK), ROEC_ARGS);
> +		  !softirq_count(), ROEC_ARGS);
>  	WARN_ONCE((curstate & (RCUTORTURE_RDR_PREEMPT | RCUTORTURE_RDR_SCHED)) &&
>  		  !(preempt_count() & PREEMPT_MASK), ROEC_ARGS);
>  	WARN_ONCE(cur_ops->readlock_nesting &&
> @@ -2004,7 +2004,7 @@ static void rcutorture_one_extend_check(char *s, int curstate, int new, int old,
>  
>  	WARN_ONCE(cur_ops->extendables &&
>  		  !(curstate & (RCUTORTURE_RDR_BH | RCUTORTURE_RDR_RBH)) &&
> -		  (preempt_count() & SOFTIRQ_MASK), ROEC_ARGS);
> +		  softirq_count(), ROEC_ARGS);

Given that softirq_count is defined as (preempt_count() & SOFTIRQ_MASK)
for CONFIG_PREEMPT_RT=n, the above don't change anything in that case,
so good.  For CONFIG_PREEMPT_RT=y, softirq_count() looks to be the way
to check BH-disable nesting, so that is good as well.

>  	/*
>  	 * non-preemptible RCU in a preemptible kernel uses preempt_disable()
> @@ -2025,6 +2025,9 @@ static void rcutorture_one_extend_check(char *s, int curstate, int new, int old,
>  	if (!IS_ENABLED(CONFIG_PREEMPT_RCU))
>  		mask |= RCUTORTURE_RDR_PREEMPT | RCUTORTURE_RDR_SCHED;
>  
> +	if (IS_ENABLED(CONFIG_PREEMPT_RT) && softirq_count())
> +		mask |= RCUTORTURE_RDR_BH | RCUTORTURE_RDR_RBH;

At this point in the code, we are complaining if something is disabled
when it is not supposed to be.  So if I understand this correctly, this
added code would suppress complaints (but only in CONFIG_PREEMPT_RT=y
kernels) when there is an unexpected rcu_read_lock() in the case where
there was either local_bh_disable() or rcu_read_lock_bh() in effect.

So I would expect that the CONFIG_PREEMPT_RT=y version of both
local_bh_disable() and rcu_read_lock_bh() would contain rcu_read_lock().

And in fact, rcu_read_lock_bh() invokes local_bh_disable(),
which, for CONFIG_PREEMPT_RT=y invokes __local_bh_disable_ip() in
kernel/softirq.c, which on the outermost local_bh_disabe() really does
invoke rcu_read_lock().

So this one looks good as well!

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

> +
>  	WARN_ONCE(cur_ops->readlock_nesting && !(curstate & mask) &&
>  		  cur_ops->readlock_nesting() > 0, ROEC_ARGS);
>  }
> -- 
> 2.17.1
> 

Re: [PATCH v2] rcu/nocb: Add Safe checks for access offloaded rdp

[Z qiang]

On Thu, May 8, 2025 at 12:25 AM Frederic Weisbecker <frederic@kernel.org> wrote:
>
> Le Wed, May 07, 2025 at 07:26:04PM +0800, Zqiang a écrit :
> > For built with CONFIG_PROVE_RCU=y and CONFIG_PREEMPT_RT=y kernels,
> > Disable BH does not change the SOFTIRQ corresponding bits in
> > preempt_count(), but change current->softirq_disable_cnt, this
> > resulted in the following splat:
> >
> > WARNING: suspicious RCU usage
> > kernel/rcu/tree_plugin.h:36 Unsafe read of RCU_NOCB offloaded state!
> > stack backtrace:
> > CPU: 0 UID: 0 PID: 22 Comm: rcuc/0
> > Call Trace:
> > [    0.407907]  <TASK>
> > [    0.407910]  dump_stack_lvl+0xbb/0xd0
> > [    0.407917]  dump_stack+0x14/0x20
> > [    0.407920]  lockdep_rcu_suspicious+0x133/0x210
> > [    0.407932]  rcu_rdp_is_offloaded+0x1c3/0x270
> > [    0.407939]  rcu_core+0x471/0x900
> > [    0.407942]  ? lockdep_hardirqs_on+0xd5/0x160
> > [    0.407954]  rcu_cpu_kthread+0x25f/0x870
> > [    0.407959]  ? __pfx_rcu_cpu_kthread+0x10/0x10
> > [    0.407966]  smpboot_thread_fn+0x34c/0xa50
> > [    0.407970]  ? trace_preempt_on+0x54/0x120
> > [    0.407977]  ? __pfx_smpboot_thread_fn+0x10/0x10
> > [    0.407982]  kthread+0x40e/0x840
> > [    0.407990]  ? __pfx_kthread+0x10/0x10
> > [    0.407994]  ? rt_spin_unlock+0x4e/0xb0
> > [    0.407997]  ? rt_spin_unlock+0x4e/0xb0
> > [    0.408000]  ? __pfx_kthread+0x10/0x10
> > [    0.408006]  ? __pfx_kthread+0x10/0x10
> > [    0.408011]  ret_from_fork+0x40/0x70
> > [    0.408013]  ? __pfx_kthread+0x10/0x10
> > [    0.408018]  ret_from_fork_asm+0x1a/0x30
> > [    0.408042]  </TASK>
> >
> > Currently, triggering an rdp offloaded state change need the
> > corresponding rdp's CPU goes offline, and at this time the rcuc
> > kthreads has already in parking state. this means the corresponding
> > rcuc kthreads can safely read offloaded state of rdp while it's
> > corresponding cpu is online.
> >
> > This commit therefore add softirq_count() check for
> > Preempt-RT kernels.
> >
> > Suggested-by: Joel Fernandes <joelagnelf@nvidia.com>
> > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> > ---
> >  kernel/rcu/tree_plugin.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index 003e549f6514..a91b2322a0cd 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -29,7 +29,7 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
> >                 (IS_ENABLED(CONFIG_HOTPLUG_CPU) && lockdep_is_cpus_held()) ||
> >                 lockdep_is_held(&rdp->nocb_lock) ||
> >                 lockdep_is_held(&rcu_state.nocb_mutex) ||
> > -               (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) &&
> > +               ((!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) || softirq_count()) &&
> >                  rdp == this_cpu_ptr(&rcu_data)) ||
>
> On a second thought, isn't "rdp == this_cpu_ptr(&rcu_data)" enough?

If the CONFIG_DEBUG_PREEMPT=y, the following code will cause
a warning in rcuop kthreads:

WARN_ON_ONCE(!rcu_rdp_is_offloaded(rdp))

> The offloaded state can only change if the CPU is completely offline.
> But if the current CPU is looking at the local rdp, it means it is online
> and the rdp can't be concurrently [de]offloaded, right?

yes

Thanks
Zqiang

>
> Thanks.
>
> >                 rcu_current_is_nocb_kthread(rdp)),
> >               "Unsafe read of RCU_NOCB offloaded state"
> > --
> > 2.17.1
> >
> >
>
> --
> Frederic Weisbecker
> SUSE Labs

Re: [PATCH v2] rcu/nocb: Add Safe checks for access offloaded rdp

[Z qiang]

>
> On Thu, May 8, 2025 at 12:25 AM Frederic Weisbecker <frederic@kernel.org> wrote:
> >
> > Le Wed, May 07, 2025 at 07:26:04PM +0800, Zqiang a écrit :
> > > For built with CONFIG_PROVE_RCU=y and CONFIG_PREEMPT_RT=y kernels,
> > > Disable BH does not change the SOFTIRQ corresponding bits in
> > > preempt_count(), but change current->softirq_disable_cnt, this
> > > resulted in the following splat:
> > >
> > > WARNING: suspicious RCU usage
> > > kernel/rcu/tree_plugin.h:36 Unsafe read of RCU_NOCB offloaded state!
> > > stack backtrace:
> > > CPU: 0 UID: 0 PID: 22 Comm: rcuc/0
> > > Call Trace:
> > > [    0.407907]  <TASK>
> > > [    0.407910]  dump_stack_lvl+0xbb/0xd0
> > > [    0.407917]  dump_stack+0x14/0x20
> > > [    0.407920]  lockdep_rcu_suspicious+0x133/0x210
> > > [    0.407932]  rcu_rdp_is_offloaded+0x1c3/0x270
> > > [    0.407939]  rcu_core+0x471/0x900
> > > [    0.407942]  ? lockdep_hardirqs_on+0xd5/0x160
> > > [    0.407954]  rcu_cpu_kthread+0x25f/0x870
> > > [    0.407959]  ? __pfx_rcu_cpu_kthread+0x10/0x10
> > > [    0.407966]  smpboot_thread_fn+0x34c/0xa50
> > > [    0.407970]  ? trace_preempt_on+0x54/0x120
> > > [    0.407977]  ? __pfx_smpboot_thread_fn+0x10/0x10
> > > [    0.407982]  kthread+0x40e/0x840
> > > [    0.407990]  ? __pfx_kthread+0x10/0x10
> > > [    0.407994]  ? rt_spin_unlock+0x4e/0xb0
> > > [    0.407997]  ? rt_spin_unlock+0x4e/0xb0
> > > [    0.408000]  ? __pfx_kthread+0x10/0x10
> > > [    0.408006]  ? __pfx_kthread+0x10/0x10
> > > [    0.408011]  ret_from_fork+0x40/0x70
> > > [    0.408013]  ? __pfx_kthread+0x10/0x10
> > > [    0.408018]  ret_from_fork_asm+0x1a/0x30
> > > [    0.408042]  </TASK>
> > >
> > > Currently, triggering an rdp offloaded state change need the
> > > corresponding rdp's CPU goes offline, and at this time the rcuc
> > > kthreads has already in parking state. this means the corresponding
> > > rcuc kthreads can safely read offloaded state of rdp while it's
> > > corresponding cpu is online.
> > >
> > > This commit therefore add softirq_count() check for
> > > Preempt-RT kernels.
> > >
> > > Suggested-by: Joel Fernandes <joelagnelf@nvidia.com>
> > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> > > ---
> > >  kernel/rcu/tree_plugin.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > > index 003e549f6514..a91b2322a0cd 100644
> > > --- a/kernel/rcu/tree_plugin.h
> > > +++ b/kernel/rcu/tree_plugin.h
> > > @@ -29,7 +29,7 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
> > >                 (IS_ENABLED(CONFIG_HOTPLUG_CPU) && lockdep_is_cpus_held()) ||
> > >                 lockdep_is_held(&rdp->nocb_lock) ||
> > >                 lockdep_is_held(&rcu_state.nocb_mutex) ||
> > > -               (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) &&
> > > +               ((!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) || softirq_count()) &&
> > >                  rdp == this_cpu_ptr(&rcu_data)) ||
> >
> > On a second thought, isn't "rdp == this_cpu_ptr(&rcu_data)" enough?
>
> If the CONFIG_DEBUG_PREEMPT=y, the following code will cause
> a warning in rcuop kthreads:
>
Only "rdp == this_cpu_ptr(&rcu_data)", this_cpu_ptr() trigger warnings.

> WARN_ON_ONCE(!rcu_rdp_is_offloaded(rdp))
>
> > The offloaded state can only change if the CPU is completely offline.
> > But if the current CPU is looking at the local rdp, it means it is online
> > and the rdp can't be concurrently [de]offloaded, right?
>
> yes
>
> Thanks
> Zqiang
>
> >
> > Thanks.
> >
> > >                 rcu_current_is_nocb_kthread(rdp)),
> > >               "Unsafe read of RCU_NOCB offloaded state"
> > > --
> > > 2.17.1
> > >
> > >
> >
> > --
> > Frederic Weisbecker
> > SUSE Labs

Re: [PATCH] rcu/nocb: Fix possible invalid rdp's->nocb_cb_kthread pointer access

[Paul E. McKenney]

On Wed, May 07, 2025 at 07:26:05PM +0800, Zqiang wrote:
> In the preparation stage of CPU online, if the corresponding
> the rdp's->nocb_cb_kthread does not exist, will be created,
> there is a situation where the rdp's rcuop kthreads creation fails,
> and then de-offload this CPU's rdp, does not assign this CPU's
> rdp->nocb_cb_kthread pointer, but this rdp's->nocb_gp_rdp and
> rdp's->rdp_gp->nocb_gp_kthread is still valid.
> 
> This will cause the subsequent re-offload operation of this offline
> CPU, which will pass the conditional check and the kthread_unpark()
> will access invalid rdp's->nocb_cb_kthread pointer.
> 
> This commit therefore use rdp's->nocb_gp_kthread instead of
> rdp_gp's->nocb_gp_kthread for safety check.

Let's see...

The rcu_nocb_cpu_offload() and rcu_nocb_cpu_deoffload() functions invoke
cpus_read_lock(), and thus exclude all the CPU-hotplug notifiers,
including the one that invokes rcutree_prepare_cpu().  There is also
rcu_spawn_gp_kthread(), but that is an early_initcall() that happens
before CPU hotplug can happen, at least for non-boot CPUs.

So rcu_spawn_cpu_nocb_kthread() cannot run concurrently with either
rcu_nocb_cpu_offload() or rcu_nocb_cpu_deoffload(), correct?

It appears that all CPUs (try to) create their rcuoc and rcuog kthreads
when they come online, regardless of the nohz_full and rcu_nocbs kernel
boot parameters, some old tree_nocb.h comments notwithstanding.  ;-) The
rcu_organize_nocb_kthreads() function looks to cover all CPUs as well,
so ->nocb_gp_rdp will always be set after very early boot (give or take
alloc_bootmem_cpumask_var() failure in rcu_nocb_setup() and checked for
by the cpumask_available() in rcu_organize_nocb_kthreads()).

The rcu_spawn_cpu_nocb_kthread() can fail to spawn the GP kthread,
in which case both ->nocb_cb_kthread and ->nocb_gp_kthread remain
NULL.

If so, LGTM.

							Thanx, Paul

> Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> ---
>  kernel/rcu/tree_nocb.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> index 1596812f7f12..6679140bb0b5 100644
> --- a/kernel/rcu/tree_nocb.h
> +++ b/kernel/rcu/tree_nocb.h
> @@ -1146,7 +1146,6 @@ static bool rcu_nocb_rdp_offload_wait_cond(struct rcu_data *rdp)
>  static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
>  {
>  	int wake_gp;
> -	struct rcu_data *rdp_gp = rdp->nocb_gp_rdp;
>  
>  	WARN_ON_ONCE(cpu_online(rdp->cpu));
>  	/*
> @@ -1156,7 +1155,7 @@ static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
>  	if (!rdp->nocb_gp_rdp)
>  		return -EINVAL;
>  
> -	if (WARN_ON_ONCE(!rdp_gp->nocb_gp_kthread))
> +	if (WARN_ON_ONCE(!rdp->nocb_gp_kthread))
>  		return -EINVAL;
>  
>  	pr_info("Offloading %d\n", rdp->cpu);
> @@ -1166,7 +1165,7 @@ static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
>  
>  	wake_gp = rcu_nocb_queue_toggle_rdp(rdp);
>  	if (wake_gp)
> -		wake_up_process(rdp_gp->nocb_gp_kthread);
> +		wake_up_process(rdp->nocb_gp_kthread);
>  
>  	swait_event_exclusive(rdp->nocb_state_wq,
>  			      rcu_nocb_rdp_offload_wait_cond(rdp));
> -- 
> 2.17.1
> 

Re: [PATCH] rcu/nocb: Fix possible invalid rdp's->nocb_cb_kthread pointer access

[Z qiang]

>
> On Wed, May 07, 2025 at 07:26:05PM +0800, Zqiang wrote:
> > In the preparation stage of CPU online, if the corresponding
> > the rdp's->nocb_cb_kthread does not exist, will be created,
> > there is a situation where the rdp's rcuop kthreads creation fails,
> > and then de-offload this CPU's rdp, does not assign this CPU's
> > rdp->nocb_cb_kthread pointer, but this rdp's->nocb_gp_rdp and
> > rdp's->rdp_gp->nocb_gp_kthread is still valid.
> >
> > This will cause the subsequent re-offload operation of this offline
> > CPU, which will pass the conditional check and the kthread_unpark()
> > will access invalid rdp's->nocb_cb_kthread pointer.
> >
> > This commit therefore use rdp's->nocb_gp_kthread instead of
> > rdp_gp's->nocb_gp_kthread for safety check.
>
> Let's see...
>
> The rcu_nocb_cpu_offload() and rcu_nocb_cpu_deoffload() functions invoke
> cpus_read_lock(), and thus exclude all the CPU-hotplug notifiers,
> including the one that invokes rcutree_prepare_cpu().  There is also
> rcu_spawn_gp_kthread(), but that is an early_initcall() that happens
> before CPU hotplug can happen, at least for non-boot CPUs.
>
> So rcu_spawn_cpu_nocb_kthread() cannot run concurrently with either
> rcu_nocb_cpu_offload() or rcu_nocb_cpu_deoffload(), correct?

Yes, the rcutree_prepare_cpu() is invoked under the cpus_write_lock()
protection.

>
> It appears that all CPUs (try to) create their rcuoc and rcuog kthreads
> when they come online, regardless of the nohz_full and rcu_nocbs kernel
> boot parameters, some old tree_nocb.h comments notwithstanding.  ;-) The
> rcu_organize_nocb_kthreads() function looks to cover all CPUs as well,
> so ->nocb_gp_rdp will always be set after very early boot (give or take
> alloc_bootmem_cpumask_var() failure in rcu_nocb_setup() and checked for
> by the cpumask_available() in rcu_organize_nocb_kthreads()).
>
> The rcu_spawn_cpu_nocb_kthread() can fail to spawn the GP kthread,
> in which case both ->nocb_cb_kthread and ->nocb_gp_kthread remain
> NULL.

This is a low probability event, but it is possible, if this happens,
and we test it with rcutorture configured with parameters
nocbs_toggle and onoff_interval, it will trigger a null ptr access.

Thanks
Zqiang


>
> If so, LGTM.
>
>                                                         Thanx, Paul
>
> > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> > ---
> >  kernel/rcu/tree_nocb.h | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> > index 1596812f7f12..6679140bb0b5 100644
> > --- a/kernel/rcu/tree_nocb.h
> > +++ b/kernel/rcu/tree_nocb.h
> > @@ -1146,7 +1146,6 @@ static bool rcu_nocb_rdp_offload_wait_cond(struct rcu_data *rdp)
> >  static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
> >  {
> >       int wake_gp;
> > -     struct rcu_data *rdp_gp = rdp->nocb_gp_rdp;
> >
> >       WARN_ON_ONCE(cpu_online(rdp->cpu));
> >       /*
> > @@ -1156,7 +1155,7 @@ static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
> >       if (!rdp->nocb_gp_rdp)
> >               return -EINVAL;
> >
> > -     if (WARN_ON_ONCE(!rdp_gp->nocb_gp_kthread))
> > +     if (WARN_ON_ONCE(!rdp->nocb_gp_kthread))
> >               return -EINVAL;
> >
> >       pr_info("Offloading %d\n", rdp->cpu);
> > @@ -1166,7 +1165,7 @@ static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
> >
> >       wake_gp = rcu_nocb_queue_toggle_rdp(rdp);
> >       if (wake_gp)
> > -             wake_up_process(rdp_gp->nocb_gp_kthread);
> > +             wake_up_process(rdp->nocb_gp_kthread);
> >
> >       swait_event_exclusive(rdp->nocb_state_wq,
> >                             rcu_nocb_rdp_offload_wait_cond(rdp));
> > --
> > 2.17.1
> >

Re: [PATCH] rcu/nocb: Fix possible invalid rdp's->nocb_cb_kthread pointer access

[Paul E. McKenney]

On Fri, May 09, 2025 at 11:32:13AM +0800, Z qiang wrote:
> >
> > On Wed, May 07, 2025 at 07:26:05PM +0800, Zqiang wrote:
> > > In the preparation stage of CPU online, if the corresponding
> > > the rdp's->nocb_cb_kthread does not exist, will be created,
> > > there is a situation where the rdp's rcuop kthreads creation fails,
> > > and then de-offload this CPU's rdp, does not assign this CPU's
> > > rdp->nocb_cb_kthread pointer, but this rdp's->nocb_gp_rdp and
> > > rdp's->rdp_gp->nocb_gp_kthread is still valid.
> > >
> > > This will cause the subsequent re-offload operation of this offline
> > > CPU, which will pass the conditional check and the kthread_unpark()
> > > will access invalid rdp's->nocb_cb_kthread pointer.
> > >
> > > This commit therefore use rdp's->nocb_gp_kthread instead of
> > > rdp_gp's->nocb_gp_kthread for safety check.
> >
> > Let's see...
> >
> > The rcu_nocb_cpu_offload() and rcu_nocb_cpu_deoffload() functions invoke
> > cpus_read_lock(), and thus exclude all the CPU-hotplug notifiers,
> > including the one that invokes rcutree_prepare_cpu().  There is also
> > rcu_spawn_gp_kthread(), but that is an early_initcall() that happens
> > before CPU hotplug can happen, at least for non-boot CPUs.
> >
> > So rcu_spawn_cpu_nocb_kthread() cannot run concurrently with either
> > rcu_nocb_cpu_offload() or rcu_nocb_cpu_deoffload(), correct?
> 
> Yes, the rcutree_prepare_cpu() is invoked under the cpus_write_lock()
> protection.

Very good!

> > It appears that all CPUs (try to) create their rcuoc and rcuog kthreads
> > when they come online, regardless of the nohz_full and rcu_nocbs kernel
> > boot parameters, some old tree_nocb.h comments notwithstanding.  ;-) The
> > rcu_organize_nocb_kthreads() function looks to cover all CPUs as well,
> > so ->nocb_gp_rdp will always be set after very early boot (give or take
> > alloc_bootmem_cpumask_var() failure in rcu_nocb_setup() and checked for
> > by the cpumask_available() in rcu_organize_nocb_kthreads()).
> >
> > The rcu_spawn_cpu_nocb_kthread() can fail to spawn the GP kthread,
> > in which case both ->nocb_cb_kthread and ->nocb_gp_kthread remain
> > NULL.
> 
> This is a low probability event, but it is possible, if this happens,
> and we test it with rcutorture configured with parameters
> nocbs_toggle and onoff_interval, it will trigger a null ptr access.

Understood, and that might be another patch if you would like to
persue it.

							Thanx, Paul

> Thanks
> Zqiang
> 
> 
> >
> > If so, LGTM.
> >
> >                                                         Thanx, Paul
> >
> > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> > > ---
> > >  kernel/rcu/tree_nocb.h | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> > > index 1596812f7f12..6679140bb0b5 100644
> > > --- a/kernel/rcu/tree_nocb.h
> > > +++ b/kernel/rcu/tree_nocb.h
> > > @@ -1146,7 +1146,6 @@ static bool rcu_nocb_rdp_offload_wait_cond(struct rcu_data *rdp)
> > >  static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
> > >  {
> > >       int wake_gp;
> > > -     struct rcu_data *rdp_gp = rdp->nocb_gp_rdp;
> > >
> > >       WARN_ON_ONCE(cpu_online(rdp->cpu));
> > >       /*
> > > @@ -1156,7 +1155,7 @@ static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
> > >       if (!rdp->nocb_gp_rdp)
> > >               return -EINVAL;
> > >
> > > -     if (WARN_ON_ONCE(!rdp_gp->nocb_gp_kthread))
> > > +     if (WARN_ON_ONCE(!rdp->nocb_gp_kthread))
> > >               return -EINVAL;
> > >
> > >       pr_info("Offloading %d\n", rdp->cpu);
> > > @@ -1166,7 +1165,7 @@ static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
> > >
> > >       wake_gp = rcu_nocb_queue_toggle_rdp(rdp);
> > >       if (wake_gp)
> > > -             wake_up_process(rdp_gp->nocb_gp_kthread);
> > > +             wake_up_process(rdp->nocb_gp_kthread);
> > >
> > >       swait_event_exclusive(rdp->nocb_state_wq,
> > >                             rcu_nocb_rdp_offload_wait_cond(rdp));
> > > --
> > > 2.17.1
> > >

Re: [PATCH v2] rcu/nocb: Add Safe checks for access offloaded rdp

[Frederic Weisbecker]

Le Thu, May 08, 2025 at 02:43:11PM +0800, Z qiang a écrit :
> On Thu, May 8, 2025 at 12:25 AM Frederic Weisbecker <frederic@kernel.org> wrote:
> > On a second thought, isn't "rdp == this_cpu_ptr(&rcu_data)" enough?
> 
> If the CONFIG_DEBUG_PREEMPT=y, the following code will cause
> a warning in rcuop kthreads:
> 
> WARN_ON_ONCE(!rcu_rdp_is_offloaded(rdp))

I keep forgetting that, indeed!

Looks good then, thanks.

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

-- 
Frederic Weisbecker
SUSE Labs

Re: [PATCH v2] rcu/nocb: Add Safe checks for access offloaded rdp

[Joel Fernandes]

On 5/9/2025 9:33 AM, Frederic Weisbecker wrote:
> Le Thu, May 08, 2025 at 02:43:11PM +0800, Z qiang a écrit :
>> On Thu, May 8, 2025 at 12:25 AM Frederic Weisbecker <frederic@kernel.org> wrote:
>>> On a second thought, isn't "rdp == this_cpu_ptr(&rcu_data)" enough?
>>
>> If the CONFIG_DEBUG_PREEMPT=y, the following code will cause
>> a warning in rcuop kthreads:
>>
>> WARN_ON_ONCE(!rcu_rdp_is_offloaded(rdp))
> 
> I keep forgetting that, indeed!
> 
> Looks good then, thanks.
> 
> Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
> 

Applying for v6.16 with the tag.

thanks,

 - Joel

Re: [PATCH] rcu/nocb: Fix possible invalid rdp's->nocb_cb_kthread pointer access

[Joel Fernandes]

On 5/7/2025 7:26 AM, Zqiang wrote:
> In the preparation stage of CPU online, if the corresponding
> the rdp's->nocb_cb_kthread does not exist, will be created,
> there is a situation where the rdp's rcuop kthreads creation fails,
> and then de-offload this CPU's rdp, does not assign this CPU's
> rdp->nocb_cb_kthread pointer, but this rdp's->nocb_gp_rdp and
> rdp's->rdp_gp->nocb_gp_kthread is still valid.
> 
> This will cause the subsequent re-offload operation of this offline
> CPU, which will pass the conditional check and the kthread_unpark()
> will access invalid rdp's->nocb_cb_kthread pointer.
> 
> This commit therefore use rdp's->nocb_gp_kthread instead of
> rdp_gp's->nocb_gp_kthread for safety check.
> 
> Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>

Is it possible that rdp_gp->nocb_gp_kthread is valid, but  rdp->nocb_cb_kthread
is still invalid (because its creation failed?). This seems a bit fragile to me
to assume that if rdp_gp->nocb_gp_kthread then rdp->nocb_cb_kthread is valid. Or
is there a path that makes sure this invariant is always satisfied? If so, can
we add additional warnings for checking this invariant?

Also from the other thread, it sounds like there is more work to do here
(related patches so I'd like to defer this to 6.17 - feel free to keep posting
patches for this work though).

Thanks!

 - Joel

> ---
>  kernel/rcu/tree_nocb.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> index 1596812f7f12..6679140bb0b5 100644
> --- a/kernel/rcu/tree_nocb.h
> +++ b/kernel/rcu/tree_nocb.h
> @@ -1146,7 +1146,6 @@ static bool rcu_nocb_rdp_offload_wait_cond(struct rcu_data *rdp)
>  static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
>  {
>  	int wake_gp;
> -	struct rcu_data *rdp_gp = rdp->nocb_gp_rdp;
>  
>  	WARN_ON_ONCE(cpu_online(rdp->cpu));
>  	/*
> @@ -1156,7 +1155,7 @@ static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
>  	if (!rdp->nocb_gp_rdp)
>  		return -EINVAL;
>  
> -	if (WARN_ON_ONCE(!rdp_gp->nocb_gp_kthread))
> +	if (WARN_ON_ONCE(!rdp->nocb_gp_kthread))
>  		return -EINVAL;
>  
>  	pr_info("Offloading %d\n", rdp->cpu);
> @@ -1166,7 +1165,7 @@ static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
>  
>  	wake_gp = rcu_nocb_queue_toggle_rdp(rdp);
>  	if (wake_gp)
> -		wake_up_process(rdp_gp->nocb_gp_kthread);
> +		wake_up_process(rdp->nocb_gp_kthread);
>  
>  	swait_event_exclusive(rdp->nocb_state_wq,
>  			      rcu_nocb_rdp_offload_wait_cond(rdp));

Re: [PATCH] rcu/nocb: Fix possible invalid rdp's->nocb_cb_kthread pointer access

[Joel Fernandes]

On 5/9/2025 3:07 PM, Joel Fernandes wrote:
> 
> 
> On 5/7/2025 7:26 AM, Zqiang wrote:
>> In the preparation stage of CPU online, if the corresponding
>> the rdp's->nocb_cb_kthread does not exist, will be created,
>> there is a situation where the rdp's rcuop kthreads creation fails,
>> and then de-offload this CPU's rdp, does not assign this CPU's
>> rdp->nocb_cb_kthread pointer, but this rdp's->nocb_gp_rdp and
>> rdp's->rdp_gp->nocb_gp_kthread is still valid.

Maybe I mixed up what you're doing but this commit message is a bit hard to
parse. You're saying in the problematic scenario, "rdp's->nocb_gp_rdp" is valid,
but in the patch below you're checking for "rdp's->nocb_gp_rdp". So I am a bit
confused, why you would not run into the same problematic scenario. I think I
agree with your patch but it would be good to refine and clarify the problematic
condition, the commit message, and also please add some comments to the code :).

Do you have a reproducer for this? If not, maybe we can work on some test cases
Both CPU hotplug and offload is tested by rcutorture, so I'd expect we run into
it. But perhaps not, because it requires kthread creation to fail?

thanks,

 - Joel


>>
>> This will cause the subsequent re-offload operation of this offline
>> CPU, which will pass the conditional check and the kthread_unpark()
>> will access invalid rdp's->nocb_cb_kthread pointer.
>>
>> This commit therefore use rdp's->nocb_gp_kthread instead of
>> rdp_gp's->nocb_gp_kthread for safety check.
>>
>> Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> 
> Is it possible that rdp_gp->nocb_gp_kthread is valid, but  rdp->nocb_cb_kthread
> is still invalid (because its creation failed?). This seems a bit fragile to me
> to assume that if rdp_gp->nocb_gp_kthread then rdp->nocb_cb_kthread is valid. Or
> is there a path that makes sure this invariant is always satisfied? If so, can
> we add additional warnings for checking this invariant?
> 
> Also from the other thread, it sounds like there is more work to do here
> (related patches so I'd like to defer this to 6.17 - feel free to keep posting
> patches for this work though).
> 
> Thanks!
> 
>  - Joel
> 
>> ---
>>  kernel/rcu/tree_nocb.h | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
>> index 1596812f7f12..6679140bb0b5 100644
>> --- a/kernel/rcu/tree_nocb.h
>> +++ b/kernel/rcu/tree_nocb.h
>> @@ -1146,7 +1146,6 @@ static bool rcu_nocb_rdp_offload_wait_cond(struct rcu_data *rdp)
>>  static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
>>  {
>>  	int wake_gp;
>> -	struct rcu_data *rdp_gp = rdp->nocb_gp_rdp;
>>  
>>  	WARN_ON_ONCE(cpu_online(rdp->cpu));
>>  	/*
>> @@ -1156,7 +1155,7 @@ static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
>>  	if (!rdp->nocb_gp_rdp)
>>  		return -EINVAL;
>>  
>> -	if (WARN_ON_ONCE(!rdp_gp->nocb_gp_kthread))
>> +	if (WARN_ON_ONCE(!rdp->nocb_gp_kthread))
>>  		return -EINVAL;
>>  
>>  	pr_info("Offloading %d\n", rdp->cpu);
>> @@ -1166,7 +1165,7 @@ static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
>>  
>>  	wake_gp = rcu_nocb_queue_toggle_rdp(rdp);
>>  	if (wake_gp)
>> -		wake_up_process(rdp_gp->nocb_gp_kthread);
>> +		wake_up_process(rdp->nocb_gp_kthread);
>>  
>>  	swait_event_exclusive(rdp->nocb_state_wq,
>>  			      rcu_nocb_rdp_offload_wait_cond(rdp));
> 

Re: [PATCH] rcutorture: Fix rcutorture_one_extend_check() splat in RT kernels

[Joel Fernandes]

On 5/7/2025 5:04 PM, Paul E. McKenney wrote:
> On Wed, May 07, 2025 at 07:26:03PM +0800, Zqiang wrote:
>> For built with CONFIG_PREEMPT_RT=y kernels, running rcutorture
>> tests resulted in the following splat:
>>
>> [   68.797425] rcutorture_one_extend_check during change: Current 0x1  To add 0x1  To remove 0x0  preempt_count() 0x0
>> [   68.797533] WARNING: CPU: 2 PID: 512 at kernel/rcu/rcutorture.c:1993 rcutorture_one_extend_check+0x419/0x560 [rcutorture]
>> [   68.797601] Call Trace:
>> [   68.797602]  <TASK>
>> [   68.797619]  ? lockdep_softirqs_off+0xa5/0x160
>> [   68.797631]  rcutorture_one_extend+0x18e/0xcc0 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
>> [   68.797646]  ? local_clock+0x19/0x40
>> [   68.797659]  rcu_torture_one_read+0xf0/0x280 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
>> [   68.797678]  ? __pfx_rcu_torture_one_read+0x10/0x10 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
>> [   68.797804]  ? __pfx_rcu_torture_timer+0x10/0x10 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
>> [   68.797815] rcu-torture: rcu_torture_reader task started
>> [   68.797824] rcu-torture: Creating rcu_torture_reader task
>> [   68.797824]  rcu_torture_reader+0x238/0x580 [rcutorture 2466dbd2ff34dbaa36049cb323a80c3306ac997c]
>> [   68.797836]  ? kvm_sched_clock_read+0x15/0x30
>>
>> Disable BH does not change the SOFTIRQ corresponding bits in
>> preempt_count() for RT kernels, this commit therefore use
>> softirq_count() to check the if BH is disabled.
>>
>> Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
>> ---
>>  kernel/rcu/rcutorture.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
>> index 373c65a6e103..ef439569f979 100644
>> --- a/kernel/rcu/rcutorture.c
>> +++ b/kernel/rcu/rcutorture.c
>> @@ -471,7 +471,7 @@ rcu_read_delay(struct torture_random_state *rrsp, struct rt_read_seg *rtrsp)
>>  	    !(torture_random(rrsp) % (nrealreaders * 2000 * longdelay_ms))) {
>>  		started = cur_ops->get_gp_seq();
>>  		ts = rcu_trace_clock_local();
>> -		if (preempt_count() & (SOFTIRQ_MASK | HARDIRQ_MASK))
>> +		if ((preempt_count() & HARDIRQ_MASK) || softirq_count())
>>  			longdelay_ms = 5; /* Avoid triggering BH limits. */
>>  		mdelay(longdelay_ms);
>>  		rtrsp->rt_delay_ms = longdelay_ms;
>> @@ -1990,7 +1990,7 @@ static void rcutorture_one_extend_check(char *s, int curstate, int new, int old,
>>  		return;
>>  
>>  	WARN_ONCE((curstate & (RCUTORTURE_RDR_BH | RCUTORTURE_RDR_RBH)) &&
>> -		  !(preempt_count() & SOFTIRQ_MASK), ROEC_ARGS);
>> +		  !softirq_count(), ROEC_ARGS);
>>  	WARN_ONCE((curstate & (RCUTORTURE_RDR_PREEMPT | RCUTORTURE_RDR_SCHED)) &&
>>  		  !(preempt_count() & PREEMPT_MASK), ROEC_ARGS);
>>  	WARN_ONCE(cur_ops->readlock_nesting &&
>> @@ -2004,7 +2004,7 @@ static void rcutorture_one_extend_check(char *s, int curstate, int new, int old,
>>  
>>  	WARN_ONCE(cur_ops->extendables &&
>>  		  !(curstate & (RCUTORTURE_RDR_BH | RCUTORTURE_RDR_RBH)) &&
>> -		  (preempt_count() & SOFTIRQ_MASK), ROEC_ARGS);
>> +		  softirq_count(), ROEC_ARGS);
> Given that softirq_count is defined as (preempt_count() & SOFTIRQ_MASK)
> for CONFIG_PREEMPT_RT=n, the above don't change anything in that case,
> so good.  For CONFIG_PREEMPT_RT=y, softirq_count() looks to be the way
> to check BH-disable nesting, so that is good as well.
> 
>>  	/*
>>  	 * non-preemptible RCU in a preemptible kernel uses preempt_disable()
>> @@ -2025,6 +2025,9 @@ static void rcutorture_one_extend_check(char *s, int curstate, int new, int old,
>>  	if (!IS_ENABLED(CONFIG_PREEMPT_RCU))
>>  		mask |= RCUTORTURE_RDR_PREEMPT | RCUTORTURE_RDR_SCHED;
>>  
>> +	if (IS_ENABLED(CONFIG_PREEMPT_RT) && softirq_count())
>> +		mask |= RCUTORTURE_RDR_BH | RCUTORTURE_RDR_RBH;
> At this point in the code, we are complaining if something is disabled
> when it is not supposed to be.  So if I understand this correctly, this
> added code would suppress complaints (but only in CONFIG_PREEMPT_RT=y
> kernels) when there is an unexpected rcu_read_lock() in the case where
> there was either local_bh_disable() or rcu_read_lock_bh() in effect.
> 
> So I would expect that the CONFIG_PREEMPT_RT=y version of both
> local_bh_disable() and rcu_read_lock_bh() would contain rcu_read_lock().
> 
> And in fact, rcu_read_lock_bh() invokes local_bh_disable(),
> which, for CONFIG_PREEMPT_RT=y invokes __local_bh_disable_ip() in
> kernel/softirq.c, which on the outermost local_bh_disabe() really does
> invoke rcu_read_lock().
> 
> So this one looks good as well!
> 
> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

It is a fix so applying with the review tag, for 6.16, thanks!

 - Joel

Re: [PATCH] rcu/nocb: Fix possible invalid rdp's->nocb_cb_kthread pointer access

[Z qiang]

>
> On 5/9/2025 3:07 PM, Joel Fernandes wrote:
> >
> >
> > On 5/7/2025 7:26 AM, Zqiang wrote:
> >> In the preparation stage of CPU online, if the corresponding
> >> the rdp's->nocb_cb_kthread does not exist, will be created,
> >> there is a situation where the rdp's rcuop kthreads creation fails,
> >> and then de-offload this CPU's rdp, does not assign this CPU's
> >> rdp->nocb_cb_kthread pointer, but this rdp's->nocb_gp_rdp and
> >> rdp's->rdp_gp->nocb_gp_kthread is still valid.
>
> Maybe I mixed up what you're doing but this commit message is a bit hard to
> parse. You're saying in the problematic scenario, "rdp's->nocb_gp_rdp" is valid,
> but in the patch below you're checking for "rdp's->nocb_gp_rdp". So I am a bit
> confused, why you would not run into the same problematic scenario. I think I
> agree with your patch but it would be good to refine and clarify the problematic
> condition, the commit message, and also please add some comments to the code :).

Hello, Joel

Maybe my description is not clear enough.
The scenario I want to describe is:

This is a small probability event，the rdp->nocb_cb_kthread creation
may fail in rcu_spawn_cpu_nocb_kthread(), this rdp->nocb_cb_kthread
will not set，is null pointer.  but this rdp->rdp_gp->nocb_gp_kthread
and rdp->nocb_gp_rdp pointer is valid.

but in rcu_nocb_rdp_offload(),  we only check rdp->nocb_gp_rdp and
rdp->rdp_gp->nocb_gp_kthread,  this will cause kthread_unpark() access
null rdp->nocb_cb_kthread pointer.

so I use rdp->nocb_gp_kthread condition to make a judgment,
if the rdp->nocb_gp_kthread is valid,  this means that rdp->nocb_cb_kthread
is also valid and there is no kthreads creation failure in
rcu_spawn_cpu_nocb_kthread()

Thanks
Zqiang

>
> Do you have a reproducer for this? If not, maybe we can work on some test cases
> Both CPU hotplug and offload is tested by rcutorture, so I'd expect we run into
> it. But perhaps not, because it requires kthread creation to fail?
>
> thanks,
>
>  - Joel
>
>
> >>
> >> This will cause the subsequent re-offload operation of this offline
> >> CPU, which will pass the conditional check and the kthread_unpark()
> >> will access invalid rdp's->nocb_cb_kthread pointer.
> >>
> >> This commit therefore use rdp's->nocb_gp_kthread instead of
> >> rdp_gp's->nocb_gp_kthread for safety check.
> >>
> >> Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> >
> > Is it possible that rdp_gp->nocb_gp_kthread is valid, but  rdp->nocb_cb_kthread
> > is still invalid (because its creation failed?). This seems a bit fragile to me
> > to assume that if rdp_gp->nocb_gp_kthread then rdp->nocb_cb_kthread is valid. Or
> > is there a path that makes sure this invariant is always satisfied? If so, can
> > we add additional warnings for checking this invariant?
> >
> > Also from the other thread, it sounds like there is more work to do here
> > (related patches so I'd like to defer this to 6.17 - feel free to keep posting
> > patches for this work though).
> >
> > Thanks!
> >
> >  - Joel
> >
> >> ---
> >>  kernel/rcu/tree_nocb.h | 5 ++---
> >>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> >> index 1596812f7f12..6679140bb0b5 100644
> >> --- a/kernel/rcu/tree_nocb.h
> >> +++ b/kernel/rcu/tree_nocb.h
> >> @@ -1146,7 +1146,6 @@ static bool rcu_nocb_rdp_offload_wait_cond(struct rcu_data *rdp)
> >>  static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
> >>  {
> >>      int wake_gp;
> >> -    struct rcu_data *rdp_gp = rdp->nocb_gp_rdp;
> >>
> >>      WARN_ON_ONCE(cpu_online(rdp->cpu));
> >>      /*
> >> @@ -1156,7 +1155,7 @@ static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
> >>      if (!rdp->nocb_gp_rdp)
> >>              return -EINVAL;
> >>
> >> -    if (WARN_ON_ONCE(!rdp_gp->nocb_gp_kthread))
> >> +    if (WARN_ON_ONCE(!rdp->nocb_gp_kthread))
> >>              return -EINVAL;
> >>
> >>      pr_info("Offloading %d\n", rdp->cpu);
> >> @@ -1166,7 +1165,7 @@ static int rcu_nocb_rdp_offload(struct rcu_data *rdp)
> >>
> >>      wake_gp = rcu_nocb_queue_toggle_rdp(rdp);
> >>      if (wake_gp)
> >> -            wake_up_process(rdp_gp->nocb_gp_kthread);
> >> +            wake_up_process(rdp->nocb_gp_kthread);
> >>
> >>      swait_event_exclusive(rdp->nocb_state_wq,
> >>                            rcu_nocb_rdp_offload_wait_cond(rdp));
> >
>

Re: [PATCH] rcu/nocb: Fix possible invalid rdp's->nocb_cb_kthread pointer access

[Frederic Weisbecker]

Le Wed, May 07, 2025 at 07:26:05PM +0800, Zqiang a écrit :
> In the preparation stage of CPU online, if the corresponding
> the rdp's->nocb_cb_kthread does not exist, will be created,
> there is a situation where the rdp's rcuop kthreads creation fails,
> and then de-offload this CPU's rdp, does not assign this CPU's
> rdp->nocb_cb_kthread pointer, but this rdp's->nocb_gp_rdp and
> rdp's->rdp_gp->nocb_gp_kthread is still valid.
> 
> This will cause the subsequent re-offload operation of this offline
> CPU, which will pass the conditional check and the kthread_unpark()
> will access invalid rdp's->nocb_cb_kthread pointer.
> 
> This commit therefore use rdp's->nocb_gp_kthread instead of
> rdp_gp's->nocb_gp_kthread for safety check.
> 
> Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

-- 
Frederic Weisbecker
SUSE Labs