Re: [PATCH v1] mm/kasan: fix swapped read/write lock usage in stack ring

[Xuewen Wang]

Hi Andrey,
Thank you for your clarification and correction.
I misunderstood the design intention of this rwlock before. I only judged it from conventional read-write lock semantics, and failed to notice that concurrent writes are guaranteed by slot-level atomic operations and cmpxchg reservation. The lock is specifically used to serialize the report path against all writers, while allowing multiple save_stack_info() instances to run in parallel without lock contention.
This patch is not appropriate, so I will drop it and not proceed with a new version.
Thanks again for your guidance.
Best regards,
Xuewen Wang

在 2026/7/14 21:59, Andrey Konovalov 写道:
> On Tue, Jul 14, 2026 at 10:14 AM Xuewen Wang <wangxuewen@xxxxxxxxxx> wrote:
>>
>> The read-write lock semantics for stack_ring.lock are inverted:
>>
>> - save_stack_info() modifies stack ring entries but uses read_lock,
>>   which does not protect against concurrent writers and does not
>>   block readers from observing partial updates.
>> - kasan_complete_mode_report_info() only reads the stack ring but
>>   uses write_lock_irqsave(), which prevents concurrent readers from
>>   walking the ring simultaneously.
>>
>> Swap the lock types to match their actual access patterns:
>> - save_stack_info(): read_lock -> write_lock
>> - kasan_complete_mode_report_info(): write_lock -> read_lock
>>
>> Fixes: 7bc0584e5d2a ("kasan: implement stack ring for tag-based modes")
>> Signed-off-by: Xuewen Wang <wangxuewen@xxxxxxxxxx>
>> ---
>>  mm/kasan/report_tags.c | 4 ++--
>>  mm/kasan/tags.c        | 4 ++--
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/kasan/report_tags.c b/mm/kasan/report_tags.c
>> index d15f8f580e2c..62576b1470e1 100644
>> --- a/mm/kasan/report_tags.c
>> +++ b/mm/kasan/report_tags.c
>> @@ -39,7 +39,7 @@ void kasan_complete_mode_report_info(struct kasan_report_info *info)
>>                 return;
>>         }
>>
>> -       write_lock_irqsave(&stack_ring.lock, flags);
>> +       read_lock_irqsave(&stack_ring.lock, flags);
>>
>>         pos = atomic64_read(&stack_ring.pos);
>>
>> @@ -99,7 +99,7 @@ void kasan_complete_mode_report_info(struct kasan_report_info *info)
>>                 }
>>         }
>>
>> -       write_unlock_irqrestore(&stack_ring.lock, flags);
>> +       read_unlock_irqrestore(&stack_ring.lock, flags);
>>
>>         /* Assign the common bug type if no entries were found. */
>>         if (!info->bug_type)
>> diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
>> index d65d48b85f90..6aa09122f09e 100644
>> --- a/mm/kasan/tags.c
>> +++ b/mm/kasan/tags.c
>> @@ -110,7 +110,7 @@ static void save_stack_info(struct kmem_cache *cache, void *object,
>>          * Prevent save_stack_info() from modifying stack ring
>>          * when kasan_complete_mode_report_info() is walking it.
>>          */
>> -       read_lock_irqsave(&stack_ring.lock, flags);
>> +       write_lock_irqsave(&stack_ring.lock, flags);
>>
>>  next:
>>         pos = atomic64_fetch_add(1, &stack_ring.pos);
>> @@ -131,7 +131,7 @@ static void save_stack_info(struct kmem_cache *cache, void *object,
>>
>>         entry->ptr = object;
>>
>> -       read_unlock_irqrestore(&stack_ring.lock, flags);
>> +       write_unlock_irqrestore(&stack_ring.lock, flags);
>>
>>         if (old_stack)
>>                 stack_depot_put(old_stack);
>> --
>> 2.25.1
>>
> 
> This is no good.
> 
> RW lock here is used to prevent kasan_complete_mode_report_info() from
> running concurrently with save_stack_info(). But multiple
> save_stack_info() must be allowed to execute concurrently without
> contending for a lock. Concurrent writes coming from multiple
> save_stack_info() executing at the same time are handled via
> atomic64_fetch_add/try_cmpxchg() slot reservation.