Re: [PATCH] mm/vmalloc: Bail out if in softirq context

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm/vmalloc: ABBA deadlock when calling vmalloc() with BH or IRQs disabled
- [Medium] mm/vmalloc: __get_vm_area_node() irq bailout triggers warnings and retry
--

commit 9fea18279139941aa247a1a577dd5e101116871a
Author: Uladzislau Rezki (Sony) <urezki@gmail.com>

mm/vmalloc: Bail out if in softirq context

This commit restores a softirq check in __get_vm_area_node() using
in_serving_softirq() so that vmalloc() bails out when invoked from softirq
context while still permitting callers with disabled bottom halves. It also
replaces BUG_ON(in_nmi() || in_hardirq()) with a WARN_ON_ONCE() check that
returns NULL.

> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 44e0eb32708f1..5a949525b9db8 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -3271,7 +3271,13 @@ struct vm_struct *__get_vm_area_node(unsigned long size,
>  	struct vm_struct *area;
>  	unsigned long requested_size = size;
>  
> -	BUG_ON(in_nmi() || in_hardirq());
> +	/* IRQ re-entrancy -> irq_exit() -> process softirq. */
> +	if (in_serving_softirq())
> +		return NULL;
> +
> +	if (WARN_ON_ONCE(in_nmi() || in_hardirq()))
> +		return NULL;

[Severity: High]
Can allowing vmalloc() in task context with bottom halves or interrupts
disabled still cause a multi-CPU ABBA deadlock on vmalloc's internal locks?

In mm/vmalloc.c, alloc_vmap_area() acquires vn->pool_lock,
free_vmap_area_lock, and vn->busy.lock (and __pte_alloc_kernel() in
mm/memory.c acquires init_mm.page_table_lock) with plain spin_lock(), leaving
bottom halves and hardware interrupts enabled:

mm/vmalloc.c:alloc_vmap_area() {
    ...
	spin_lock(&vn->busy.lock);
	insert_vmap_area(va, &vn->busy.root, &vn->busy.head);
	spin_unlock(&vn->busy.lock);
    ...
}

Because __get_vm_area_node() only checks in_serving_softirq(), in_nmi(), and
in_hardirq(), a task-context caller holding a lock via spin_lock_bh() (which
sets SOFTIRQ_DISABLE_OFFSET rather than SOFTIRQ_OFFSET) or
spin_lock_irqsave() (where irqs_disabled() is true while in_hardirq() is
false) will proceed into alloc_vmap_area().

Using the bridge FDB path referenced by commit 04aa71da5f35 ("mm/vmalloc: do
not trigger BUG() on BH disabled context"):

net/bridge/br_fdb.c:br_fdb_add_local() {
    ...
	spin_lock_bh(&br->hash_lock);
	ret = fdb_add_local(br, source, addr, vid);
	spin_unlock_bh(&br->hash_lock);
    ...
}

net/bridge/br_fdb.c:br_fdb_update() {
    ...
	} else {
		spin_lock(&br->hash_lock);
		fdb = fdb_create(br, source, addr, vid, flags);
    ...
}

If CPU 0 is in task context inside alloc_vmap_area() holding
spin_lock(&vn->busy.lock) or spin_lock(&free_vmap_area_lock), and CPU 1 is in
task context inside br_fdb_add_local() holding spin_lock_bh(&br->hash_lock):

1. A hardware interrupt arrives on CPU 0, and on irq_exit() NET_RX_SOFTIRQ
   runs br_fdb_update(), which spins on spin_lock(&br->hash_lock) before ever
   reaching fdb_create() or __get_vm_area_node().
2. CPU 1 proceeds through
   fdb_add_local() -> fdb_create() -> rhashtable_lookup_insert_fast() ->
   rhashtable_insert_rehash() -> bucket_table_alloc(GFP_ATOMIC | __GFP_NOWARN)
   -> kvmalloc() -> vmalloc() -> __get_vm_area_node().
3. Because in_serving_softirq() is false on CPU 1, it enters
   alloc_vmap_area() and spins on free_vmap_area_lock or vn->busy.lock held
   by CPU 0.

Similarly, a task-context caller holding a lock with spin_lock_irqsave() can
enter alloc_vmap_area() with irqs_disabled() and invert lock ordering against
any hardirq handler acquiring that same lock (and sleep with interrupts
disabled on CONFIG_PREEMPT_RT).

Should __get_vm_area_node() also bail out when in_softirq() or
irqs_disabled() is true?

[Severity: Medium]
When __get_vm_area_node() returns NULL due to in_serving_softirq(), in_nmi(),
or in_hardirq(), how does __vmalloc_node_range_noprof() handle the NULL
return?

Looking at __vmalloc_node_range_noprof() in mm/vmalloc.c:

mm/vmalloc.c:__vmalloc_node_range_noprof() {
    ...
	area = __get_vm_area_node(real_size, align, shift, VM_ALLOC |
				  VM_UNINITIALIZED | vm_flags, start, end, node,
				  gfp_mask, caller);
	if (!area) {
		bool nofail = gfp_mask & __GFP_NOFAIL;
		warn_alloc(gfp_mask, NULL,
			"vmalloc error: size %lu, align 0x%lx, vm_struct allocation failed%s",
			size, align, (nofail) ? ". Retrying." : "");
		if (nofail) {
			schedule_timeout_uninterruptible(1);
			goto again;
		}
		goto fail;
	}
    ...
}

Since commit c6307674ed82 ("mm: kvmalloc: add non-blocking support for
vmalloc"), __kvmalloc_node_noprof() in mm/slub.c falls back to
__vmalloc_node_range_noprof() for non-blocking allocations (such as
GFP_ATOMIC or GFP_NOWAIT) when size > PAGE_SIZE and __do_kmalloc_node()
fails, while kmalloc_gfp_adjust() only adds __GFP_NOWARN to the kmalloc
attempt:

mm/slub.c:__kvmalloc_node_noprof() {
    ...
	return __vmalloc_node_range_noprof(size, align, VMALLOC_START, VMALLOC_END,
			flags, PAGE_KERNEL, allow_block ? VM_ALLOW_HUGE_VMAP:0,
			node, __builtin_return_address(0));
}

Doesn't bailing out in __get_vm_area_node() lead to a few issues in callers:

1. In softirq, hardirq, or NMI context, when !area is reached in
   __vmalloc_node_range_noprof(), it unconditionally calls warn_alloc()
   whenever __GFP_NOWARN was not passed by the caller (and if shift >
   PAGE_SHIFT, goto fail resets shift = PAGE_SHIFT and jumps to goto again,
   repeating the call and warning a second time).
2. If a caller in hardirq or NMI context invokes
   kvmalloc(size, GFP_ATOMIC | __GFP_NOWARN) (for example,
   rhashtable_insert_rehash() -> bucket_table_alloc() in lib/rhashtable.c,
   reachable from irq-safe rhashtable users and BPF_MAP_TYPE_RHASHTABLE
   updates in rhtab_map_update_elem() in kernel/bpf/hashtab.c) and the
   kmalloc attempt fails for size > PAGE_SIZE, __kvmalloc_node_noprof() falls
   back to __vmalloc_node_range_noprof() -> __get_vm_area_node() and triggers
   WARN_ON_ONCE(in_nmi() || in_hardirq()) even though the caller passed
   GFP_ATOMIC | __GFP_NOWARN and handles NULL.
3. In __vmalloc_node_range_noprof(), nofail is not cleared when
   !gfpflags_allow_blocking(gfp_mask) (unlike in __vmalloc_area_node()), so
   if __GFP_NOFAIL is set when __get_vm_area_node() returns NULL in
   interrupt context, it will call schedule_timeout_uninterruptible(1) in
   atomic context and loop indefinitely at goto again.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026102503.30860-1-urezki@gmail.com?part=1