mm/kfence: select random number before taking raw lock
Subsystems (2)
PATCH 1/1 mm/kfence: select random number before taking raw lock
The RNG uses vanilla spinlocks, not raw spinlocks, so kfence should pick its random numbers before taking its raw spinlocks. This also has the nice effect of doing less work inside the lock. It should fix a splat that Geert saw with CONFIG_PROVE_RAW_LOCK_NESTING:
dump_backtrace.part.0+0x98/0xc0
show_stack+0x14/0x28
dump_stack_lvl+0xac/0xec
dump_stack+0x14/0x2c
__lock_acquire+0x388/0x10a0
lock_acquire+0x190/0x2c0
_raw_spin_lock_irqsave+0x6c/0x94
crng_make_state+0x148/0x1e4
_get_random_bytes.part.0+0x4c/0xe8
get_random_u32+0x4c/0x140
__kfence_alloc+0x460/0x5c4
kmem_cache_alloc_trace+0x194/0x1dc
__kthread_create_on_node+0x5c/0x1a8
kthread_create_on_node+0x58/0x7c
printk_start_kthread.part.0+0x34/0xa8
printk_activate_kthreads+0x4c/0x54
do_one_initcall+0xec/0x278
kernel_init_freeable+0x11c/0x214
kernel_init+0x24/0x124
ret_from_fork+0x10/0x20--- mm/kfence/core.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
mm/kfence/core.c +5 -2
@@ -360,6 +360,9 @@ static void *kfence_guarded_alloc(struct kmem_cache *cache, size_t size, gfp_t gunsigned long flags;struct slab *slab;void *addr; +bool random_right_allocate =prandom_u32_max (2 ); +bool random_fault =CONFIG_KFENCE_STRESS_TEST_FAULTS && + !prandom_u32_max (CONFIG_KFENCE_STRESS_TEST_FAULTS );/* Try to obtain a free object. */ raw_spin_lock_irqsave (&kfence_freelist_lock, flags);
@@ -404,7 +407,7 @@ static void *kfence_guarded_alloc(struct kmem_cache *cache, size_t size, gfp_t g* is that the out-of-bounds accesses detected are deterministic for * such allocations. */ -if (prandom_u32_max (2 )) { +if (random_right_allocate) {/* Allocate on the "right" side, re-calculate address. */ meta->addr +=PAGE_SIZE - size; meta->addr =ALIGN_DOWN (meta->addr, cache->align);
@@ -444,7 +447,7 @@ static void *kfence_guarded_alloc(struct kmem_cache *cache, size_t size, gfp_t gif (cache->ctor) cache->ctor (addr); -if (CONFIG_KFENCE_STRESS_TEST_FAULTS && !prandom_u32_max (CONFIG_KFENCE_STRESS_TEST_FAULTS )) +if (random_fault)kfence_protect (meta->addr);/* Random "faults" by protecting the object. */ atomic_long_inc (&counters[KFENCE_COUNTER_ALLOCATED ]);
Signature · 1 line
2.35.1
Marco Elver
maintainerQuoted 61 lines · Jason A. Donenfeld
The RNG uses vanilla spinlocks, not raw spinlocks, so kfence should pick its random numbers before taking its raw spinlocks. This also has the nice effect of doing less work inside the lock. It should fix a splat that Geert saw with
CONFIG_PROVE_RAW_LOCK_NESTING:dump_backtrace.part.0+0x98/0xc0 show_stack+0x14/0x28 dump_stack_lvl+0xac/0xec dump_stack+0x14/0x2c __lock_acquire+0x388/0x10a0 lock_acquire+0x190/0x2c0 _raw_spin_lock_irqsave+0x6c/0x94 crng_make_state+0x148/0x1e4 _get_random_bytes.part.0+0x4c/0xe8 get_random_u32+0x4c/0x140 __kfence_alloc+0x460/0x5c4 kmem_cache_alloc_trace+0x194/0x1dc __kthread_create_on_node+0x5c/0x1a8 kthread_create_on_node+0x58/0x7c printk_start_kthread.part.0+0x34/0xa8 printk_activate_kthreads+0x4c/0x54 do_one_initcall+0xec/0x278 kernel_init_freeable+0x11c/0x214 kernel_init+0x24/0x124 ret_from_fork+0x10/0x20Cc John Ogness <[email protected]>Cc Alexander Potapenko <[email protected]>Cc Marco Elver <[email protected]>Cc Dmitry Vyukov <[email protected]>Reported-by Geert Uytterhoeven <[email protected]>Signed-off-by Jason A. Donenfeld <[email protected]>--- mm/kfence/core.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)mm/kfence/core.c +4
-1@@ -360,6 +360,9 @@ static void *kfence_guarded_alloc(struct kmem_cache *cache, size_t size, gfp_t gunsigned long flags;struct slab *slab;void *addr; +bool random_right_allocate =prandom_u32_max (2 ); +bool random_fault =CONFIG_KFENCE_STRESS_TEST_FAULTS && + !prandom_u32_max (CONFIG_KFENCE_STRESS_TEST_FAULTS );/* Try to obtain a free object. */ raw_spin_lock_irqsave (&kfence_freelist_lock, flags);@@ -404,7 +407,7 @@ static void *kfence_guarded_alloc(struct kmem_cache *cache, size_t size, gfp_t g* is that the out-of-bounds accesses detected are deterministic for * such allocations. */ -if (prandom_u32_max (2 )) { +if (random_right_allocate) {/* Allocate on the "right" side, re-calculate address. */ meta->addr +=PAGE_SIZE - size; meta->addr =ALIGN_DOWN (meta->addr, cache->align);
mm/kfence/core.c static void *kfence_guarded_alloc(struct kmem_cache *cache, size_t size, gfp_t g · line 450
Quoted 5 lines
mm/kfence/core.c +1
-1@@ -444,7 +447,7 @@ static void *kfence_guarded_alloc(struct kmem_cache *cache, size_t size, gfp_t gif (cache->ctor) cache->ctor (addr); -if (CONFIG_KFENCE_STRESS_TEST_FAULTS && !prandom_u32_max (CONFIG_KFENCE_STRESS_TEST_FAULTS )) +if (random_fault)
The compiler should elide this branch entirely if CONFIG_KFENCE_STRESS_TEST_FAULTS=0, but not sure it'll always do so now. My suggestion is to make both new bools consts, to help out the compiler a little.
Otherwise looks good, thanks for the quick fix!
Quoted 6 lines · Marco Elver
The compiler should elide this branch entirely if
CONFIG_KFENCE_STRESS_TEST_FAULTS=0, but not sure it'll always do so now. My suggestion is to make both new bools consts, to help out the compiler a little.Otherwise looks good, thanks for the quick fix!
Disassembling reveals it still does elide. (gcc 11.3) But I'll make them const just in case and send a v2.
Jason
PATCH v2 1/1 mm/kfence: select random number before taking raw lock
The RNG uses vanilla spinlocks, not raw spinlocks, so kfence should pick its random numbers before taking its raw spinlocks. This also has the nice effect of doing less work inside the lock. It should fix a splat that Geert saw with CONFIG_PROVE_RAW_LOCK_NESTING:
dump_backtrace.part.0+0x98/0xc0
show_stack+0x14/0x28
dump_stack_lvl+0xac/0xec
dump_stack+0x14/0x2c
__lock_acquire+0x388/0x10a0
lock_acquire+0x190/0x2c0
_raw_spin_lock_irqsave+0x6c/0x94
crng_make_state+0x148/0x1e4
_get_random_bytes.part.0+0x4c/0xe8
get_random_u32+0x4c/0x140
__kfence_alloc+0x460/0x5c4
kmem_cache_alloc_trace+0x194/0x1dc
__kthread_create_on_node+0x5c/0x1a8
kthread_create_on_node+0x58/0x7c
printk_start_kthread.part.0+0x34/0xa8
printk_activate_kthreads+0x4c/0x54
do_one_initcall+0xec/0x278
kernel_init_freeable+0x11c/0x214
kernel_init+0x24/0x124
ret_from_fork+0x10/0x20---
Changes v1->v2:
- Make the bools const to help compiler elide branch when possible, suggested by Marco.
mm/kfence/core.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
mm/kfence/core.c +5 -2
@@ -360,6 +360,9 @@ static void *kfence_guarded_alloc(struct kmem_cache *cache, size_t size, gfp_t gunsigned long flags;struct slab *slab;void *addr; +const bool random_right_allocate =prandom_u32_max (2 ); +const bool random_fault =CONFIG_KFENCE_STRESS_TEST_FAULTS && + !prandom_u32_max (CONFIG_KFENCE_STRESS_TEST_FAULTS );/* Try to obtain a free object. */ raw_spin_lock_irqsave (&kfence_freelist_lock, flags);
@@ -404,7 +407,7 @@ static void *kfence_guarded_alloc(struct kmem_cache *cache, size_t size, gfp_t g* is that the out-of-bounds accesses detected are deterministic for * such allocations. */ -if (prandom_u32_max (2 )) { +if (random_right_allocate) {/* Allocate on the "right" side, re-calculate address. */ meta->addr +=PAGE_SIZE - size; meta->addr =ALIGN_DOWN (meta->addr, cache->align);
@@ -444,7 +447,7 @@ static void *kfence_guarded_alloc(struct kmem_cache *cache, size_t size, gfp_t gif (cache->ctor) cache->ctor (addr); -if (CONFIG_KFENCE_STRESS_TEST_FAULTS && !prandom_u32_max (CONFIG_KFENCE_STRESS_TEST_FAULTS )) +if (random_fault)kfence_protect (meta->addr);/* Random "faults" by protecting the object. */ atomic_long_inc (&counters[KFENCE_COUNTER_ALLOCATED ]);
Signature · 1 line
2.35.1
Quoted 26 lines · Jason A. Donenfeld
The RNG uses vanilla spinlocks, not raw spinlocks, so kfence should pick its random numbers before taking its raw spinlocks. This also has the nice effect of doing less work inside the lock. It should fix a splat that Geert saw with
CONFIG_PROVE_RAW_LOCK_NESTING:dump_backtrace.part.0+0x98/0xc0 show_stack+0x14/0x28 dump_stack_lvl+0xac/0xec dump_stack+0x14/0x2c __lock_acquire+0x388/0x10a0 lock_acquire+0x190/0x2c0 _raw_spin_lock_irqsave+0x6c/0x94 crng_make_state+0x148/0x1e4 _get_random_bytes.part.0+0x4c/0xe8 get_random_u32+0x4c/0x140 __kfence_alloc+0x460/0x5c4 kmem_cache_alloc_trace+0x194/0x1dc __kthread_create_on_node+0x5c/0x1a8 kthread_create_on_node+0x58/0x7c printk_start_kthread.part.0+0x34/0xa8 printk_activate_kthreads+0x4c/0x54 do_one_initcall+0xec/0x278 kernel_init_freeable+0x11c/0x214 kernel_init+0x24/0x124 ret_from_fork+0x10/0x20
Quoted 7 lines
Cc John Ogness <[email protected]>Cc Alexander Potapenko <[email protected]>Cc Marco Elver <[email protected]>Cc Dmitry Vyukov <[email protected]>Reported-by Geert Uytterhoeven <[email protected]>Tested-by Geert Uytterhoeven <[email protected]>Signed-off-by Jason A. Donenfeld <[email protected]>
Thank you.
Quoted 43 lines
---
Changesv1->v2:
- Make the bools const to help compiler elide branch when possible, suggested by Marco.mm/kfence/core.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)mm/kfence/core.c +5
-2@@ -360,6 +360,9 @@ static void *kfence_guarded_alloc(struct kmem_cache *cache, size_t size, gfp_t gunsigned long flags;struct slab *slab;void *addr; +const bool random_right_allocate =prandom_u32_max (2 ); +const bool random_fault =CONFIG_KFENCE_STRESS_TEST_FAULTS && + !prandom_u32_max (CONFIG_KFENCE_STRESS_TEST_FAULTS );/* Try to obtain a free object. */ raw_spin_lock_irqsave (&kfence_freelist_lock, flags);@@ -404,7 +407,7 @@ static void *kfence_guarded_alloc(struct kmem_cache *cache, size_t size, gfp_t g* is that the out-of-bounds accesses detected are deterministic for * such allocations. */ -if (prandom_u32_max (2 )) { +if (random_right_allocate) {/* Allocate on the "right" side, re-calculate address. */ meta->addr +=PAGE_SIZE - size; meta->addr =ALIGN_DOWN (meta->addr, cache->align);@@ -444,7 +447,7 @@ static void *kfence_guarded_alloc(struct kmem_cache *cache, size_t size, gfp_t gif (cache->ctor) cache->ctor (addr); -if (CONFIG_KFENCE_STRESS_TEST_FAULTS && !prandom_u32_max (CONFIG_KFENCE_STRESS_TEST_FAULTS )) +if (random_fault)kfence_protect (meta->addr);/* Random "faults" by protecting the object. */ atomic_long_inc (&counters[KFENCE_COUNTER_ALLOCATED ]);Signature · 1 line
2.35.1
Quoted 26 lines · Jason A. Donenfeld
The RNG uses vanilla spinlocks, not raw spinlocks, so kfence should pick its random numbers before taking its raw spinlocks. This also has the nice effect of doing less work inside the lock. It should fix a splat that Geert saw with
CONFIG_PROVE_RAW_LOCK_NESTING:dump_backtrace.part.0+0x98/0xc0 show_stack+0x14/0x28 dump_stack_lvl+0xac/0xec dump_stack+0x14/0x2c __lock_acquire+0x388/0x10a0 lock_acquire+0x190/0x2c0 _raw_spin_lock_irqsave+0x6c/0x94 crng_make_state+0x148/0x1e4 _get_random_bytes.part.0+0x4c/0xe8 get_random_u32+0x4c/0x140 __kfence_alloc+0x460/0x5c4 kmem_cache_alloc_trace+0x194/0x1dc __kthread_create_on_node+0x5c/0x1a8 kthread_create_on_node+0x58/0x7c printk_start_kthread.part.0+0x34/0xa8 printk_activate_kthreads+0x4c/0x54 do_one_initcall+0xec/0x278 kernel_init_freeable+0x11c/0x214 kernel_init+0x24/0x124 ret_from_fork+0x10/0x20
Quoted 7 lines
Cc John Ogness <[email protected]>Cc Alexander Potapenko <[email protected]>Cc Marco Elver <[email protected]>Cc Dmitry Vyukov <[email protected]>Reported-by Geert Uytterhoeven <[email protected]>Tested-by Geert Uytterhoeven <[email protected]>Signed-off-by Jason A. Donenfeld <[email protected]>
Thanks a lot for fixing this. It is great to know that the printk kthreads were not the culprit here ;-)
Best Regards,
Petr
Hi Jason,
Quoted 26 lines · Jason A. Donenfeld
The RNG uses vanilla spinlocks, not raw spinlocks, so kfence should pick its random numbers before taking its raw spinlocks. This also has the nice effect of doing less work inside the lock. It should fix a splat that Geert saw with
CONFIG_PROVE_RAW_LOCK_NESTING:dump_backtrace.part.0+0x98/0xc0 show_stack+0x14/0x28 dump_stack_lvl+0xac/0xec dump_stack+0x14/0x2c __lock_acquire+0x388/0x10a0 lock_acquire+0x190/0x2c0 _raw_spin_lock_irqsave+0x6c/0x94 crng_make_state+0x148/0x1e4 _get_random_bytes.part.0+0x4c/0xe8 get_random_u32+0x4c/0x140 __kfence_alloc+0x460/0x5c4 kmem_cache_alloc_trace+0x194/0x1dc __kthread_create_on_node+0x5c/0x1a8 kthread_create_on_node+0x58/0x7c printk_start_kthread.part.0+0x34/0xa8 printk_activate_kthreads+0x4c/0x54 do_one_initcall+0xec/0x278 kernel_init_freeable+0x11c/0x214 kernel_init+0x24/0x124 ret_from_fork+0x10/0x20
Quoted 6 lines
Cc John Ogness <[email protected]>Cc Alexander Potapenko <[email protected]>Cc Marco Elver <[email protected]>Cc Dmitry Vyukov <[email protected]>Reported-by Geert Uytterhoeven <[email protected]>Signed-off-by Jason A. Donenfeld <[email protected]>
Thank you, the splat is gone.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds