linux-kernel - Re: [PATCH] rcu/nocb: Add Safe checks for access offloaded rdp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <BC3AD2C0-E78E-4512-BC9A-1454FB13E0F6@joelfernandes.org>
Date: Tue, 6 May 2025 13:49:47 -0400
From: Joel Fernandes <joel@...lfernandes.org>
To: qiang Z <qiang.zhang1211@...il.com>
Cc: Joel Fernandes <joelagnelf@...dia.com>,
 Frederic Weisbecker <frederic@...nel.org>, paulmck@...nel.org,
 neeraj.upadhyay@...nel.org, urezki@...il.com, boqun.feng@...il.com,
 rcu@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rcu/nocb: Add Safe checks for access offloaded rdp



> On May 6, 2025, at 2:26 AM, Z qiang <qiang.zhang1211@...il.com> wrote:
> 
> 
>> 
>> 
>> 
>> 
>>> On 4/30/2025 12:14 PM, Joel Fernandes wrote:
>>> 
>>> 
>>> On 4/30/2025 10:57 AM, Z qiang wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 4/28/2025 6:59 AM, Z qiang wrote:
>>>>>>> 
>>>>>>> Le Mon, Apr 28, 2025 at 05:54:03PM +0800, Zqiang a écrit :
>>>>>>>> For Preempt-RT kernel, when enable CONFIG_PROVE_RCU Kconfig,
>>>>>>>> disable local bh in rcuc kthreads will not affect preempt_count(),
>>>>>>>> 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 rdp->rcu_cpu_kthread_task check for
>>>>>>>> Preempt-RT kernels.
>>>>>>>> 
>>>>>>>> Signed-off-by: Zqiang <qiang.zhang1211@...il.com>
>>>>>>>> ---
>>>>>>>> kernel/rcu/tree_plugin.h | 4 +++-
>>>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>> 
>>>>>>>> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
>>>>>>>> index 003e549f6514..fe728eded36e 100644
>>>>>>>> --- a/kernel/rcu/tree_plugin.h
>>>>>>>> +++ b/kernel/rcu/tree_plugin.h
>>>>>>>> @@ -31,7 +31,9 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
>>>>>>>>                lockdep_is_held(&rcu_state.nocb_mutex) ||
>>>>>>>>                (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) &&
>>>>>>>>                 rdp == this_cpu_ptr(&rcu_data)) ||
>>>>>>>> -               rcu_current_is_nocb_kthread(rdp)),
>>>>>>>> +               rcu_current_is_nocb_kthread(rdp) ||
>>>>>>>> +               (IS_ENABLED(CONFIG_PREEMPT_RT) &&
>>>>>>>> +                current == rdp->rcu_cpu_kthread_task)),
>>>>>>> 
>>>>>>> Isn't it safe also on !CONFIG_PREEMPT_RT ?
>>>>>> 
>>>>>> For !CONFIG_PREEMPT_RT and  in rcuc kthreads, it's also safe,
>>>>>> but the following check will passed :
>>>>>> 
>>>>>> (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) &&
>>>>>>          rdp == this_cpu_ptr(&rcu_data))
>>>>> 
>>>>> I think the fact that it already passes for !PREEMPT_RT does not matter, because
>>>>> it simplifies the code so drop the PREEMPT_RT check?
>>>>> 
>>>>> Or will softirq_count() not work? It appears to have special casing for
>>>>> PREEMPT_RT's local_bh_disable():
>>>>> 
>>>>> (   ( !(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) || softirq_count() )
>>>>>   && rdp == this_cpu_ptr(&rcu_data))  )
>>>> 
>>>> Thank you for Joel's reply,  I also willing to accept such
>>>> modifications and resend :) .
>>> Thanks, I am Ok with either approach whichever you and Frederic together decide.
>>> I can then pull this in for the v6.16 merge window once you resend, thanks!
>>> 
>> 
>> Frederic, there are a couple of ways we can move forward hear. Does the
>> softirq_count() approach sound good to you? If yes, I can fixup the patch myself.
> 
> Hello, Joel
> 
> If you send a patch to fix it, I'd be happy,  you can add me as the
> Reported-by ;)

Actually Z, could you send the patch with the suggestion above after appropriate testing? That way I will be more comfortable applying it for 6.16.

Sorry for any confusion, 

Thanks!

- Joel


> 
> Thanks
> Zqiang
> 
>> 
>> I am also Ok at this point to take it in for 6.16, though I've also stored it in
>> my rcu/dev branch for Neeraj's 6.17 PR, just in case :)
>> 
>> - Joel
>> 
>>