[PATCH] kasan: Don't call find_vm_area() in RT kernel

[PATCH] kasan: Don't call find_vm_area() in RT kernel

[Waiman Long]

The following bug report appeared with a test run in a RT debug kernel.

[ 3359.353842] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
[ 3359.353848] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 140605, name: kunit_try_catch
[ 3359.353853] preempt_count: 1, expected: 0
  :
[ 3359.353933] Call trace:
  :
[ 3359.353955]  rt_spin_lock+0x70/0x140
[ 3359.353959]  find_vmap_area+0x84/0x168
[ 3359.353963]  find_vm_area+0x1c/0x50
[ 3359.353966]  print_address_description.constprop.0+0x2a0/0x320
[ 3359.353972]  print_report+0x108/0x1f8
[ 3359.353976]  kasan_report+0x90/0xc8
[ 3359.353980]  __asan_load1+0x60/0x70

The print_address_description() is run with a raw_spinlock_t acquired
and interrupt disabled. The find_vm_area() function needs to acquire
a spinlock_t which becomes a sleeping lock in the RT kernel. IOW,
we can't call find_vm_area() in a RT kernel. Fix this bug report
by skipping the find_vm_area() call in this case and just print out
the address as is.

For !RT kernel, follow the example set in commit 0cce06ba859a
("debugobjects,locking: Annotate debug_object_fill_pool() wait type
violation") and use DEFINE_WAIT_OVERRIDE_MAP() to avoid a spinlock_t
inside raw_spinlock_t warning.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 mm/kasan/report.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 3fe77a360f1c..e1ee687966aa 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -398,9 +398,20 @@ static void print_address_description(void *addr, u8 tag,
 		pr_err("\n");
 	}
 
-	if (is_vmalloc_addr(addr)) {
-		struct vm_struct *va = find_vm_area(addr);
+	if (!is_vmalloc_addr(addr))
+		goto print_page;
 
+	/*
+	 * RT kernel cannot call find_vm_area() in atomic context.
+	 * For !RT kernel, prevent spinlock_t inside raw_spinlock_t warning
+	 * by raising wait-type to WAIT_SLEEP.
+	 */
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
+		static DEFINE_WAIT_OVERRIDE_MAP(vmalloc_map, LD_WAIT_SLEEP);
+		struct vm_struct *va;
+
+		lock_map_acquire_try(&vmalloc_map);
+		va = find_vm_area(addr);
 		if (va) {
 			pr_err("The buggy address belongs to the virtual mapping at\n"
 			       " [%px, %px) created by:\n"
@@ -410,8 +421,13 @@ static void print_address_description(void *addr, u8 tag,
 
 			page = vmalloc_to_page(addr);
 		}
+		lock_map_release(&vmalloc_map);
+	} else {
+		pr_err("The buggy address %px belongs to a vmalloc virtual mapping\n",
+			addr);
 	}
 
+print_page:
 	if (page) {
 		pr_err("The buggy address belongs to the physical page:\n");
 		dump_page(page, "kasan: bad access detected");
-- 
2.48.1

Re: [PATCH] kasan: Don't call find_vm_area() in RT kernel

[Andrew Morton]

On Tue, 11 Feb 2025 11:07:50 -0500 Waiman Long <longman@redhat.com> wrote:

> The following bug report appeared with a test run in a RT debug kernel.
> 
> [ 3359.353842] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
> [ 3359.353848] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 140605, name: kunit_try_catch
> [ 3359.353853] preempt_count: 1, expected: 0
>   :
> [ 3359.353933] Call trace:
>   :
> [ 3359.353955]  rt_spin_lock+0x70/0x140
> [ 3359.353959]  find_vmap_area+0x84/0x168
> [ 3359.353963]  find_vm_area+0x1c/0x50
> [ 3359.353966]  print_address_description.constprop.0+0x2a0/0x320
> [ 3359.353972]  print_report+0x108/0x1f8
> [ 3359.353976]  kasan_report+0x90/0xc8
> [ 3359.353980]  __asan_load1+0x60/0x70
> 
> The print_address_description() is run with a raw_spinlock_t acquired
> and interrupt disabled. The find_vm_area() function needs to acquire
> a spinlock_t which becomes a sleeping lock in the RT kernel. IOW,
> we can't call find_vm_area() in a RT kernel. Fix this bug report
> by skipping the find_vm_area() call in this case and just print out
> the address as is.
> 
> For !RT kernel, follow the example set in commit 0cce06ba859a
> ("debugobjects,locking: Annotate debug_object_fill_pool() wait type
> violation") and use DEFINE_WAIT_OVERRIDE_MAP() to avoid a spinlock_t
> inside raw_spinlock_t warning.
> 

Thanks.  I added it and shall await review from the KASAN developers.

I'm thinking we add

Fixes: c056a364e954 ("kasan: print virtual mapping info in reports")
Cc: <stable@vger.kernel.org>

but c056a364e954 is 3 years old and I don't think we care about -rt in
such old kernels.  Thoughts?

Re: [PATCH] kasan: Don't call find_vm_area() in RT kernel

[Steven Rostedt]

On Tue, 11 Feb 2025 14:57:30 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> I'm thinking we add
> 
> Fixes: c056a364e954 ("kasan: print virtual mapping info in reports")
> Cc: <stable@vger.kernel.org>
> 
> but c056a364e954 is 3 years old and I don't think we care about -rt in
> such old kernels.  Thoughts?

We still support -rt in older kernels back to 5.4, and we merge in stable
releases. If this fixes an -rt issue, please do mark it for stable.

Thanks,

-- Steve

Re: [PATCH] kasan: Don't call find_vm_area() in RT kernel

[Waiman Long]

On 2/11/25 5:57 PM, Andrew Morton wrote:
> On Tue, 11 Feb 2025 11:07:50 -0500 Waiman Long <longman@redhat.com> wrote:
>
>> The following bug report appeared with a test run in a RT debug kernel.
>>
>> [ 3359.353842] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
>> [ 3359.353848] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 140605, name: kunit_try_catch
>> [ 3359.353853] preempt_count: 1, expected: 0
>>    :
>> [ 3359.353933] Call trace:
>>    :
>> [ 3359.353955]  rt_spin_lock+0x70/0x140
>> [ 3359.353959]  find_vmap_area+0x84/0x168
>> [ 3359.353963]  find_vm_area+0x1c/0x50
>> [ 3359.353966]  print_address_description.constprop.0+0x2a0/0x320
>> [ 3359.353972]  print_report+0x108/0x1f8
>> [ 3359.353976]  kasan_report+0x90/0xc8
>> [ 3359.353980]  __asan_load1+0x60/0x70
>>
>> The print_address_description() is run with a raw_spinlock_t acquired
>> and interrupt disabled. The find_vm_area() function needs to acquire
>> a spinlock_t which becomes a sleeping lock in the RT kernel. IOW,
>> we can't call find_vm_area() in a RT kernel. Fix this bug report
>> by skipping the find_vm_area() call in this case and just print out
>> the address as is.
>>
>> For !RT kernel, follow the example set in commit 0cce06ba859a
>> ("debugobjects,locking: Annotate debug_object_fill_pool() wait type
>> violation") and use DEFINE_WAIT_OVERRIDE_MAP() to avoid a spinlock_t
>> inside raw_spinlock_t warning.
>>
> Thanks.  I added it and shall await review from the KASAN developers.
>
> I'm thinking we add
>
> Fixes: c056a364e954 ("kasan: print virtual mapping info in reports")
> Cc: <stable@vger.kernel.org>
>
> but c056a364e954 is 3 years old and I don't think we care about -rt in
> such old kernels.  Thoughts?

The KASAN report_lock was changed to a raw_spinlock_t in v6.13 kernel 
with commit e30a0361b851 ("kasan: make report_lock a raw spinlock") to 
fix a similar RT problem. The report_lock is acquired before calling 
print_address_description(). Before commit e30a0361b851, this 
find_vm_area() is a secondary issue. We may consider commit e30a0361b851 
isn't complete and this is a fix for that.

The DEFINE_WAIT_OVERRIDE_MAP() macro was introduced in v6.4. So this 
patch cannot be backported to a version earlier than that unless commit 
0cce06ba859a is there.

Cheers,
Longman

Re: [PATCH] kasan: Don't call find_vm_area() in RT kernel

[Andrew Morton]

On Tue, 11 Feb 2025 19:16:34 -0500 Waiman Long <llong@redhat.com> wrote:

> > Fixes: c056a364e954 ("kasan: print virtual mapping info in reports")
> > Cc: <stable@vger.kernel.org>
> >
> > but c056a364e954 is 3 years old and I don't think we care about -rt in
> > such old kernels.  Thoughts?
> 
> The KASAN report_lock was changed to a raw_spinlock_t in v6.13 kernel 
> with commit e30a0361b851 ("kasan: make report_lock a raw spinlock") to 
> fix a similar RT problem. The report_lock is acquired before calling 
> print_address_description(). Before commit e30a0361b851, this 
> find_vm_area() is a secondary issue. We may consider commit e30a0361b851 
> isn't complete and this is a fix for that.

Great, thanks, updated...

Re: [PATCH] kasan: Don't call find_vm_area() in RT kernel

[Andrey Ryabinin]

On Tue, Feb 11, 2025 at 5:08 PM Waiman Long <longman@redhat.com> wrote:
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 3fe77a360f1c..e1ee687966aa 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -398,9 +398,20 @@ static void print_address_description(void *addr, u8 tag,
>                 pr_err("\n");
>         }
>
> -       if (is_vmalloc_addr(addr)) {
> -               struct vm_struct *va = find_vm_area(addr);
> +       if (!is_vmalloc_addr(addr))
> +               goto print_page;
>
> +       /*
> +        * RT kernel cannot call find_vm_area() in atomic context.
> +        * For !RT kernel, prevent spinlock_t inside raw_spinlock_t warning
> +        * by raising wait-type to WAIT_SLEEP.
> +        */
> +       if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
> +               static DEFINE_WAIT_OVERRIDE_MAP(vmalloc_map, LD_WAIT_SLEEP);
> +               struct vm_struct *va;
> +
> +               lock_map_acquire_try(&vmalloc_map);
> +               va = find_vm_area(addr);

Can we hide all this logic behind some function like
kasan_find_vm_area() which would return NULL for -rt?

>                 if (va) {
>                         pr_err("The buggy address belongs to the virtual mapping at\n"
>                                " [%px, %px) created by:\n"
> @@ -410,8 +421,13 @@ static void print_address_description(void *addr, u8 tag,
>
>                         page = vmalloc_to_page(addr);

Or does vmalloc_to_page() secretly take  some lock somewhere so we
need to guard it with this 'vmalloc_map' too?
So my suggestion above wouldn't be enough, if that's the case.

>                 }
> +               lock_map_release(&vmalloc_map);
> +       } else {
> +               pr_err("The buggy address %px belongs to a vmalloc virtual mapping\n",
> +                       addr);
>         }
>

Re: [PATCH] kasan: Don't call find_vm_area() in RT kernel

[Waiman Long]

On 2/12/25 6:59 AM, Andrey Ryabinin wrote:
> On Tue, Feb 11, 2025 at 5:08 PM Waiman Long <longman@redhat.com> wrote:
>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
>> index 3fe77a360f1c..e1ee687966aa 100644
>> --- a/mm/kasan/report.c
>> +++ b/mm/kasan/report.c
>> @@ -398,9 +398,20 @@ static void print_address_description(void *addr, u8 tag,
>>                  pr_err("\n");
>>          }
>>
>> -       if (is_vmalloc_addr(addr)) {
>> -               struct vm_struct *va = find_vm_area(addr);
>> +       if (!is_vmalloc_addr(addr))
>> +               goto print_page;
>>
>> +       /*
>> +        * RT kernel cannot call find_vm_area() in atomic context.
>> +        * For !RT kernel, prevent spinlock_t inside raw_spinlock_t warning
>> +        * by raising wait-type to WAIT_SLEEP.
>> +        */
>> +       if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
>> +               static DEFINE_WAIT_OVERRIDE_MAP(vmalloc_map, LD_WAIT_SLEEP);
>> +               struct vm_struct *va;
>> +
>> +               lock_map_acquire_try(&vmalloc_map);
>> +               va = find_vm_area(addr);
> Can we hide all this logic behind some function like
> kasan_find_vm_area() which would return NULL for -rt?
Sure. We can certainly do that.
>
>>                  if (va) {
>>                          pr_err("The buggy address belongs to the virtual mapping at\n"
>>                                 " [%px, %px) created by:\n"
>> @@ -410,8 +421,13 @@ static void print_address_description(void *addr, u8 tag,
>>
>>                          page = vmalloc_to_page(addr);
> Or does vmalloc_to_page() secretly take  some lock somewhere so we
> need to guard it with this 'vmalloc_map' too?
> So my suggestion above wouldn't be enough, if that's the case.

AFAICS, vmalloc_to_page() doesn't seem to take any lock.  Even if it 
takes another spinlock, it will still be under the vmalloc_map 
protection until lock_map_release() is called.

Cheers,
Longman

Re: [PATCH] kasan: Don't call find_vm_area() in RT kernel

[Andrey Ryabinin]

On 2/12/25 2:34 PM, Waiman Long wrote:
> 
> On 2/12/25 6:59 AM, Andrey Ryabinin wrote:
>> On Tue, Feb 11, 2025 at 5:08 PM Waiman Long <longman@redhat.com> wrote:
>>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
>>> index 3fe77a360f1c..e1ee687966aa 100644
>>> --- a/mm/kasan/report.c
>>> +++ b/mm/kasan/report.c
>>> @@ -398,9 +398,20 @@ static void print_address_description(void *addr, u8 tag,
>>>                  pr_err("\n");
>>>          }
>>>
>>> -       if (is_vmalloc_addr(addr)) {
>>> -               struct vm_struct *va = find_vm_area(addr);
>>> +       if (!is_vmalloc_addr(addr))
>>> +               goto print_page;
>>>
>>> +       /*
>>> +        * RT kernel cannot call find_vm_area() in atomic context.
>>> +        * For !RT kernel, prevent spinlock_t inside raw_spinlock_t warning
>>> +        * by raising wait-type to WAIT_SLEEP.
>>> +        */
>>> +       if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
>>> +               static DEFINE_WAIT_OVERRIDE_MAP(vmalloc_map, LD_WAIT_SLEEP);
>>> +               struct vm_struct *va;
>>> +
>>> +               lock_map_acquire_try(&vmalloc_map);
>>> +               va = find_vm_area(addr);
>> Can we hide all this logic behind some function like
>> kasan_find_vm_area() which would return NULL for -rt?
> Sure. We can certainly do that.
>>
>>>                  if (va) {
>>>                          pr_err("The buggy address belongs to the virtual mapping at\n"
>>>                                 " [%px, %px) created by:\n"
>>> @@ -410,8 +421,13 @@ static void print_address_description(void *addr, u8 tag,
>>>
>>>                          page = vmalloc_to_page(addr);
>> Or does vmalloc_to_page() secretly take  some lock somewhere so we
>> need to guard it with this 'vmalloc_map' too?
>> So my suggestion above wouldn't be enough, if that's the case.
> 
> AFAICS, vmalloc_to_page() doesn't seem to take any lock.  Even if it takes another spinlock, it will still be under the vmalloc_map protection until lock_map_release() is called.
> 

I meant to do something like bellow, which would leave vmalloc_to_page() out of vmalloc_map scope.
That's why I raised this question.

---
 mm/kasan/report.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 3fe77a360f1c..f3683215f4ca 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -370,6 +370,20 @@ static inline bool init_task_stack_addr(const void *addr)
 			sizeof(init_thread_union.stack));
 }
 
+static inline struct vm_struct *kasan_find_vm_area(void *addr)
+{
+	static DEFINE_WAIT_OVERRIDE_MAP(vmalloc_map, LD_WAIT_SLEEP);
+	struct vm_struct *va;
+
+	if (IS_ENABLED(CONFIG_PREEMPT_RT))
+		return NULL;
+
+	lock_map_acquire_try(&vmalloc_map);
+	va = find_vm_area(addr);
+	lock_map_release(&vmalloc_map);
+	return va;
+}
+
 static void print_address_description(void *addr, u8 tag,
 				      struct kasan_report_info *info)
 {
@@ -399,8 +413,7 @@ static void print_address_description(void *addr, u8 tag,
 	}
 
 	if (is_vmalloc_addr(addr)) {
-		struct vm_struct *va = find_vm_area(addr);
-
+		struct vm_area *va = kasan_find_vm_area(addr);
 		if (va) {
 			pr_err("The buggy address belongs to the virtual mapping at\n"
 			       " [%px, %px) created by:\n"
-- 
2.45.3