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

[Bradley Morgan]

On 25 September 2026 20:52:45 BST, Mathieu Desnoyers
<mathieu.desnoyers@xxxxxxxxxxxx> wrote:
>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@xxxxxxxxxxxx>
>
>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

1: Didn't know you could self nak
2: I must've been dumb during review then, in respin I'll think heavier about it.

>
>
>> Reported-by: Boqun Feng <boqun@xxxxxxxxxx>
>> Cc: Paul E. McKenney <paulmck@xxxxxxxxxx>
>> Cc: Boqun Feng <boqun@xxxxxxxxxx>
>> Cc: Bradley Morgan <brads@xxxxxxxxxxxxxx>
>> Cc: Gary Guo <gary@xxxxxxxxxxx>
>> Cc: <rcu@xxxxxxxxxxxxxxx>
>> Cc: <lkmm@xxxxxxxxxxxxxxx>
>> ---
>>   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);
>>   
>
>
>

--- Thanks!
https://lore.kernel.org/all/EE579805-42F2-4C58-B752-F28779EEB717@xxxxxxxxx/