Re: [PATCH] srcu: Fix a rare race issue in __srcu_read_(un)lock()

[Frederic Weisbecker <frederic@kernel.org>]

On Thu, Nov 03, 2022 at 09:13:13PM +0800, Pingfan Liu wrote:
> Clarify at first:
>   This issue is totally detected by code suspicion, not a real experience.
> 
> Scene:
> 
>   __srcu_read_(un)lock() uses percpu variable srcu_(un)lock_count[2].
> Normally, the percpu can help avoid the non-atomic RMW issue, but in
> some rare cases, it can not.
> 
> Supposing that __srcu_read_lock() runs on cpuX, the statement
>     this_cpu_inc(ssp->sda->srcu_lock_count[idx]);
> can be decomposed into two sub group:
>     -1. get the address of this_cpu_ptr(ssp->sda)->srcu_lock_count[idx],
> denoted as addressX and let unsigned long *pX = addressX;
>     -2. *pX = *pX + 1;

It's not supposed to happen:

* The weak version of this_cpu_inc() disables interrupts during the whole.
* x86 adds directly to gs/fs memory
* arm64, loongarch, s390 disable preemption

This has to be a fundamental constraint of this_cpu_*() ops implementation.

Thanks.



> 
> Now, assuming there are two tasks, denoted as taskA and taskB, three
> cpus, denoted as cpuX, cpuY and cpuZ. Let both taskA and taskB finish
> the first step as above on cpuX, but migrate to cpuY and cpuZ
> individually just before the second step. Then both of them continue to
> execute concurrently from the second step. This will raise a typical
> non-atomic RMW issue.
> 
> Solution:
> 
>   This issue can be tackled by disable preemption around the two sub
> groups.
> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> To: rcu@vger.kernel.org
> ---
>  kernel/rcu/srcutree.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 1c304fec89c0..a38a3779dc01 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -636,7 +636,11 @@ int __srcu_read_lock(struct srcu_struct *ssp)
>  	int idx;
>  
>  	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
> +	__preempt_count_inc();
> +	barrier();
>  	this_cpu_inc(ssp->sda->srcu_lock_count[idx]);
> +	barrier();
> +	__preempt_count_dec();
>  	smp_mb(); /* B */  /* Avoid leaking the critical section. */
>  	return idx;
>  }
> @@ -650,7 +654,11 @@ EXPORT_SYMBOL_GPL(__srcu_read_lock);
>  void __srcu_read_unlock(struct srcu_struct *ssp, int idx)
>  {
>  	smp_mb(); /* C */  /* Avoid leaking the critical section. */
> +	__preempt_count_inc();
> +	barrier();
>  	this_cpu_inc(ssp->sda->srcu_unlock_count[idx]);
> +	barrier();
> +	__preempt_count_dec();
>  }
>  EXPORT_SYMBOL_GPL(__srcu_read_unlock);
>  
> -- 
> 2.31.1
>