kfree_nolock() deliberately skips slab free hooks that should not be called in unknown context. However, there is one more place that needs the same thing: memcg_alloc_abort_single(). Instead of open coding slab_free_hook(), let it handle SLAB_FREE_NOLOCK. This is required to correctly handle allocation failures due to memcg charging. kfree_nolock() now respects init_on_free which has been ignored. Besides that, no functional change intended. Fixes: af92793e52c3 ("slab: Introduce kmalloc_nolock() and kfree_nolock().") Signed-off-by: Harry Yoo (Meta) --- mm/slub.c | 76 +++++++++++++++++++++++++++++++-------------------------------- 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/mm/slub.c b/mm/slub.c index 761dc2affea2..7c0adc104988 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -2684,11 +2684,14 @@ struct rcu_delayed_free { * - memcg_slab_post_alloc_hook() * - alloc_tagging_slab_alloc_hook() * + * slab_free_hook() assumes that memcg_slab_free_hook() and + * alloc_tagging_slab_free_hook() are already invoked. + * * Free hooks that must handle missing corresponding alloc hooks: * - kmemleak_free_recursive() * - kfence_free() * - * Free hooks that have no alloc hook counterpart, and thus safe to call: + * Free hooks that have no alloc hook counterpart: * - debug_check_no_locks_freed() * - debug_check_no_obj_freed() * - __kcsan_check_access() @@ -2698,28 +2701,39 @@ bool slab_free_hook(struct kmem_cache *s, void *x, unsigned int free_flags) { bool init = free_flags & SLAB_FREE_INIT; bool after_rcu_delay = free_flags & SLAB_FREE_AFTER_RCU_DELAY; + bool allow_spin = free_flags_allow_spinning(free_flags); /* Are the object contents still accessible? */ bool still_accessible = (s->flags & SLAB_TYPESAFE_BY_RCU) && !after_rcu_delay; - kmemleak_free_recursive(x, s->flags); kmsan_slab_free(s, x); - debug_check_no_locks_freed(x, s->object_size); + /* Hooks that acquire locks or kmalloc_nolock() skips alloc hook counterpart */ + if (likely(allow_spin)) { + kmemleak_free_recursive(x, s->flags); + debug_check_no_locks_freed(x, s->object_size); - if (!(s->flags & SLAB_DEBUG_OBJECTS)) - debug_check_no_obj_freed(x, s->object_size); + if (!(s->flags & SLAB_DEBUG_OBJECTS)) + debug_check_no_obj_freed(x, s->object_size); - /* Use KCSAN to help debug racy use-after-free. */ - if (!still_accessible) - __kcsan_check_access(x, s->object_size, - KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT); + /* Use KCSAN to help debug racy use-after-free. */ + if (!still_accessible) + __kcsan_check_access(x, s->object_size, + KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT); - if (kfence_free(x)) - return false; + if (kfence_free(x)) + return false; + } /* * Give KASAN a chance to notice an invalid free operation before we * modify the object. + * + * If KASAN finds a bug it will do kasan_report_invalid_free() which + * will call raw_spin_lock_irqsave() which is technically unsafe from + * NMI, but take chance and report kernel bug. The sequence of + * kasan_report_invalid_free() -> raw_spin_lock_irqsave() -> NMI + * -> kfree_nolock() -> kasan_report_invalid_free() on the same CPU + * is double buggy and deserves to deadlock. */ if (kasan_slab_pre_free(s, x)) return false; @@ -2728,6 +2742,7 @@ bool slab_free_hook(struct kmem_cache *s, void *x, unsigned int free_flags) if (still_accessible) { struct rcu_delayed_free *delayed_free; + VM_WARN_ON_ONCE(!allow_spin); delayed_free = kmalloc_obj(*delayed_free, GFP_NOWAIT); if (delayed_free) { /* @@ -2775,8 +2790,11 @@ bool slab_free_hook(struct kmem_cache *s, void *x, unsigned int free_flags) set_orig_size(s, x, orig_size); } - /* KASAN might put x into memory quarantine, delaying its reuse. */ - return !kasan_slab_free(s, x, init, still_accessible, false); + /* + * KASAN might put x into memory quarantine, delaying its reuse. + * Skip quarantine when spinning is not allowed as it uses a spinlock. + */ + return !kasan_slab_free(s, x, init, still_accessible, !allow_spin); } static __fastpath_inline @@ -6935,6 +6953,7 @@ void kfree_nolock(const void *object) struct slab *slab; struct kmem_cache *s; void *x = (void *)object; + unsigned int free_flags = SLAB_FREE_NOLOCK; if (unlikely(ZERO_OR_NULL_PTR(object))) return; @@ -6949,33 +6968,12 @@ void kfree_nolock(const void *object) memcg_slab_free_hook(s, slab, &x, 1); alloc_tagging_slab_free_hook(s, slab, &x, 1); - /* - * Unlike slab_free() do NOT call the following: - * kmemleak_free_recursive(x, s->flags); - * debug_check_no_locks_freed(x, s->object_size); - * debug_check_no_obj_freed(x, s->object_size); - * __kcsan_check_access(x, s->object_size, ..); - * kfence_free(x); - * since they take spinlocks or not safe from any context. - */ - kmsan_slab_free(s, x); - /* - * If KASAN finds a kernel bug it will do kasan_report_invalid_free() - * which will call raw_spin_lock_irqsave() which is technically - * unsafe from NMI, but take chance and report kernel bug. - * The sequence of - * kasan_report_invalid_free() -> raw_spin_lock_irqsave() -> NMI - * -> kfree_nolock() -> kasan_report_invalid_free() on the same CPU - * is double buggy and deserves to deadlock. - */ - if (kasan_slab_pre_free(s, x)) + + if (slab_want_init_on_free(s)) + free_flags |= SLAB_FREE_INIT; + + if (unlikely(!slab_free_hook(s, x, free_flags))) return; - /* - * memcg, kasan_slab_pre_free are done for 'x'. - * The only thing left is kasan_poison without quarantine, - * since kasan quarantine takes locks and not supported from NMI. - */ - kasan_slab_free(s, x, false, false, /* skip quarantine */true); if (likely(can_free_to_pcs(slab)) && likely(free_to_pcs(s, x, false))) return; -- 2.54.0