Re: [PATCH] hazptr: Fix two-phase hazptr_synchronize race with detach

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

On 2026-09-25 15:49, Mathieu Desnoyers wrote:
> Boqun Feng pointed out that a detach happening concurrently with
> hazptr_synchronize can miss a slot. Indeed, scanning the per-CPU
> slots needs to be done *before* scanning the overflow lists. Fix the
> implementation accordingly.
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Self-NACK. I did not migrate the detach code to move it to the new
hazptr_overflow_list_phase. Sorry about the noise. I'll prepare an
updated version.

Thanks,

Mathieu


> Reported-by: Boqun Feng <boqun@kernel.org>
> Cc: Paul E. McKenney <paulmck@kernel.org>
> Cc: Boqun Feng <boqun@kernel.org>
> Cc: Bradley Morgan <brads@mainlining.org>
> Cc: Gary Guo <gary@garyguo.net>
> Cc: <rcu@vger.kernel.org>
> Cc: <lkmm@lists.linux.dev>
> ---
>   kernel/hazptr.c | 43 ++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/hazptr.c b/kernel/hazptr.c
> index d3d1050d92cf..ccc19843edbd 100644
> --- a/kernel/hazptr.c
> +++ b/kernel/hazptr.c
> @@ -24,6 +24,9 @@ static DEFINE_MUTEX(hazptr_wildcard_lock);	/* Protect the wildcard flip. */
>   void *hazptr_wildcard = (void *) 1UL;
>   EXPORT_SYMBOL_GPL(hazptr_wildcard);
>   
> +/* The current overflow list phase. */
> +static unsigned int hazptr_overflow_list_phase;
> +
>   struct hazptr_overflow_list {
>   	raw_spinlock_t lock;		/* Lock protecting overflow list and list generation. */
>   	struct hlist_head head;		/* Overflow list head. */
> @@ -53,6 +56,12 @@ void *flip_wildcard(void *wildcard)
>   	return ((unsigned long) wildcard == 1UL) ? (void *) 2UL : (void *) 1UL;
>   }
>   
> +static
> +unsigned int flip_list_phase(unsigned int phase)
> +{
> +	return 1 - phase;
> +}
> +
>   static
>   bool is_wildcard(void *addr)
>   {
> @@ -178,15 +187,12 @@ void hazptr_synchronize_cpu_slots(int cpu, void *addr, void *scan_wildcard)
>   }
>   
>   static
> -void hazptr_scan_period(void *addr, void *scan_wildcard)
> +void hazptr_scan_cpu_slots_period(void *addr, void *scan_wildcard)
>   {
> -	unsigned int scan_idx = (unsigned long) scan_wildcard - 1;
>   	int cpu;
>   
>   	/* Scan all CPUs slots. */
>   	for_each_possible_cpu(cpu) {
> -		struct hazptr_overflow_list_flip *overflow_list_flip = per_cpu_ptr(&percpu_overflow_list_flip, cpu);
> -
>   		/*
>   		 * Scan CPU slots.
>   		 * Forward progress against recurring wildcards is guaranteed
> @@ -199,6 +205,17 @@ void hazptr_scan_period(void *addr, void *scan_wildcard)
>   		 * to acquire that same hazard pointer value.
>   		 */
>   		hazptr_synchronize_cpu_slots(cpu, addr, scan_wildcard);
> +	}
> +}
> +
> +static
> +void hazptr_scan_overflow_list_period(void *addr, unsigned int scan_idx)
> +{
> +	int cpu;
> +
> +	/* Scan all CPUs overflow lists. */
> +	for_each_possible_cpu(cpu) {
> +		struct hazptr_overflow_list_flip *overflow_list_flip = per_cpu_ptr(&percpu_overflow_list_flip, cpu);
>   
>   		/*
>   		 * Scan backup slots in percpu overflow lists.
> @@ -218,6 +235,7 @@ void hazptr_scan_period(void *addr, void *scan_wildcard)
>    */
>   void hazptr_synchronize(void *addr)
>   {
> +	unsigned int scan_list_phase;
>   	void *scan_wildcard;
>   
>   	/*
> @@ -236,10 +254,21 @@ void hazptr_synchronize(void *addr)
>   	smp_mb();
>   
>   	guard(mutex)(&hazptr_wildcard_lock);
> +
> +	/* Scan per-CPU slots. */
>   	scan_wildcard = flip_wildcard(hazptr_wildcard);
> -	hazptr_scan_period(addr, scan_wildcard);
> -	WRITE_ONCE(hazptr_wildcard, scan_wildcard);	/* Flip the current wildcard. */
> -	hazptr_scan_period(addr, flip_wildcard(scan_wildcard));
> +	hazptr_scan_cpu_slots_period(addr, scan_wildcard);
> +	WRITE_ONCE(hazptr_wildcard, scan_wildcard);			/* Flip the current wildcard. */
> +	hazptr_scan_cpu_slots_period(addr, flip_wildcard(scan_wildcard));
> +
> +	/*
> +	 * Scan overflow lists *after* scanning per-CPU slots. See
> +	 * hazptr_promote_to_backup_slot() for scan ordering requirement.
> +	 */
> +	scan_list_phase = flip_list_phase(hazptr_overflow_list_phase);
> +	hazptr_scan_overflow_list_period(addr, scan_list_phase);
> +	WRITE_ONCE(hazptr_overflow_list_phase, scan_list_phase);	/* Flip the current list phase. */
> +	hazptr_scan_overflow_list_period(addr, flip_list_phase(scan_list_phase));
>   }
>   EXPORT_SYMBOL_GPL(hazptr_synchronize);
>   


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com