mm/kasan: fix swapped read/write lock usage in stack ring

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

Posted by Xuewen Wang 1 week, 4 days ago

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@kylinos.cn>
---
 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

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

Posted by Andrey Konovalov 1 week, 4 days ago

On Tue, Jul 14, 2026 at 10:14 AM Xuewen Wang <wangxuewen@kylinos.cn> 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@kylinos.cn>
> ---
>  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.

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

Posted by Xuewen Wang 1 week, 3 days ago

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@kylinos.cn> 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@kylinos.cn>
>> ---
>>  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.