2022-04-01 18:47:32

by Zqiang

[permalink] [raw]
Subject: [PATCH] kasan: Fix sleeping function called from invalid context in PREEMPT_RT

BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:46
in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 1, name: swapper/0
preempt_count: 1, expected: 0
...........
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.17.1-rt16-yocto-preempt-rt #22
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
BIOS rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014
Call Trace:
<TASK>
dump_stack_lvl+0x60/0x8c
dump_stack+0x10/0x12
__might_resched.cold+0x13b/0x173
rt_spin_lock+0x5b/0xf0
___cache_free+0xa5/0x180
qlist_free_all+0x7a/0x160
per_cpu_remove_cache+0x5f/0x70
smp_call_function_many_cond+0x4c4/0x4f0
on_each_cpu_cond_mask+0x49/0xc0
kasan_quarantine_remove_cache+0x54/0xf0
kasan_cache_shrink+0x9/0x10
kmem_cache_shrink+0x13/0x20
acpi_os_purge_cache+0xe/0x20
acpi_purge_cached_objects+0x21/0x6d
acpi_initialize_objects+0x15/0x3b
acpi_init+0x130/0x5ba
do_one_initcall+0xe5/0x5b0
kernel_init_freeable+0x34f/0x3ad
kernel_init+0x1e/0x140
ret_from_fork+0x22/0x30

When the kmem_cache_shrink() be called, the IPI was triggered, the
___cache_free() is called in IPI interrupt context, the local lock
or spin lock will be acquired. on PREEMPT_RT kernel, these lock is
replaced with sleepbale rt spin lock, so the above problem is triggered.
fix it by migrating the release action from the IPI interrupt context
to the task context on RT kernel.

Signed-off-by: Zqiang <[email protected]>
---
mm/kasan/quarantine.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 08291ed33e93..c26fa6473119 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -90,6 +90,7 @@ static void qlist_move_all(struct qlist_head *from, struct qlist_head *to)
*/
static DEFINE_PER_CPU(struct qlist_head, cpu_quarantine);

+static DEFINE_PER_CPU(struct qlist_head, cpu_shrink_qlist);
/* Round-robin FIFO array of batches. */
static struct qlist_head global_quarantine[QUARANTINE_BATCHES];
static int quarantine_head;
@@ -311,12 +312,14 @@ static void qlist_move_cache(struct qlist_head *from,
static void per_cpu_remove_cache(void *arg)
{
struct kmem_cache *cache = arg;
- struct qlist_head to_free = QLIST_INIT;
+ struct qlist_head *to_free;
struct qlist_head *q;

+ to_free = this_cpu_ptr(&cpu_shrink_qlist);
q = this_cpu_ptr(&cpu_quarantine);
- qlist_move_cache(q, &to_free, cache);
- qlist_free_all(&to_free, cache);
+ qlist_move_cache(q, to_free, cache);
+ if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+ qlist_free_all(to_free, cache);
}

/* Free all quarantined objects belonging to cache. */
@@ -324,6 +327,7 @@ void kasan_quarantine_remove_cache(struct kmem_cache *cache)
{
unsigned long flags, i;
struct qlist_head to_free = QLIST_INIT;
+ int cpu;

/*
* Must be careful to not miss any objects that are being moved from
@@ -334,6 +338,11 @@ void kasan_quarantine_remove_cache(struct kmem_cache *cache)
*/
on_each_cpu(per_cpu_remove_cache, cache, 1);

+ if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
+ for_each_possible_cpu(cpu)
+ qlist_free_all(per_cpu_ptr(&cpu_shrink_qlist, cpu), cache);
+ }
+
raw_spin_lock_irqsave(&quarantine_lock, flags);
for (i = 0; i < QUARANTINE_BATCHES; i++) {
if (qlist_empty(&global_quarantine[i]))
--
2.25.1


2022-04-01 23:07:18

by Dmitry Vyukov

[permalink] [raw]
Subject: Re: [PATCH] kasan: Fix sleeping function called from invalid context in PREEMPT_RT

On Fri, 1 Apr 2022 at 11:09, Zqiang <[email protected]> wrote:
>
> BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:46
> in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 1, name: swapper/0
> preempt_count: 1, expected: 0
> ...........
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.17.1-rt16-yocto-preempt-rt #22
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
> BIOS rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014
> Call Trace:
> <TASK>
> dump_stack_lvl+0x60/0x8c
> dump_stack+0x10/0x12
> __might_resched.cold+0x13b/0x173
> rt_spin_lock+0x5b/0xf0
> ___cache_free+0xa5/0x180
> qlist_free_all+0x7a/0x160
> per_cpu_remove_cache+0x5f/0x70
> smp_call_function_many_cond+0x4c4/0x4f0
> on_each_cpu_cond_mask+0x49/0xc0
> kasan_quarantine_remove_cache+0x54/0xf0
> kasan_cache_shrink+0x9/0x10
> kmem_cache_shrink+0x13/0x20
> acpi_os_purge_cache+0xe/0x20
> acpi_purge_cached_objects+0x21/0x6d
> acpi_initialize_objects+0x15/0x3b
> acpi_init+0x130/0x5ba
> do_one_initcall+0xe5/0x5b0
> kernel_init_freeable+0x34f/0x3ad
> kernel_init+0x1e/0x140
> ret_from_fork+0x22/0x30
>
> When the kmem_cache_shrink() be called, the IPI was triggered, the
> ___cache_free() is called in IPI interrupt context, the local lock
> or spin lock will be acquired. on PREEMPT_RT kernel, these lock is
> replaced with sleepbale rt spin lock, so the above problem is triggered.
> fix it by migrating the release action from the IPI interrupt context
> to the task context on RT kernel.
>
> Signed-off-by: Zqiang <[email protected]>
> ---
> mm/kasan/quarantine.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 08291ed33e93..c26fa6473119 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -90,6 +90,7 @@ static void qlist_move_all(struct qlist_head *from, struct qlist_head *to)
> */
> static DEFINE_PER_CPU(struct qlist_head, cpu_quarantine);
>
> +static DEFINE_PER_CPU(struct qlist_head, cpu_shrink_qlist);
> /* Round-robin FIFO array of batches. */
> static struct qlist_head global_quarantine[QUARANTINE_BATCHES];
> static int quarantine_head;
> @@ -311,12 +312,14 @@ static void qlist_move_cache(struct qlist_head *from,
> static void per_cpu_remove_cache(void *arg)
> {
> struct kmem_cache *cache = arg;
> - struct qlist_head to_free = QLIST_INIT;
> + struct qlist_head *to_free;
> struct qlist_head *q;
>
> + to_free = this_cpu_ptr(&cpu_shrink_qlist);
> q = this_cpu_ptr(&cpu_quarantine);
> - qlist_move_cache(q, &to_free, cache);
> - qlist_free_all(&to_free, cache);
> + qlist_move_cache(q, to_free, cache);
> + if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> + qlist_free_all(to_free, cache);
> }
>
> /* Free all quarantined objects belonging to cache. */
> @@ -324,6 +327,7 @@ void kasan_quarantine_remove_cache(struct kmem_cache *cache)
> {
> unsigned long flags, i;
> struct qlist_head to_free = QLIST_INIT;
> + int cpu;
>
> /*
> * Must be careful to not miss any objects that are being moved from
> @@ -334,6 +338,11 @@ void kasan_quarantine_remove_cache(struct kmem_cache *cache)
> */
> on_each_cpu(per_cpu_remove_cache, cache, 1);
>
> + if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
> + for_each_possible_cpu(cpu)
> + qlist_free_all(per_cpu_ptr(&cpu_shrink_qlist, cpu), cache);
> + }

Hi Zqiang,

This code is not protected by any kind of mutex, right? If so, I think
it can lead to subtle memory corruptions, double-frees and leaks when
several tasks move to/free from cpu_shrink_qlist list.


> raw_spin_lock_irqsave(&quarantine_lock, flags);
> for (i = 0; i < QUARANTINE_BATCHES; i++) {
> if (qlist_empty(&global_quarantine[i]))