Re: [PATCH 3/6] selftests/bpf: libarena: Disable IRQs during allocation

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unhandled lock failure path in `buddy_alloc` and `buddy_alloc_from_new_chunk` leaks newly allocated memory chunks.
--

--- Patch [3]: [PATCH 3/6] selftests/bpf: libarena: Disable IRQs during allocation ---
commit 33cf3ed979bbaa281367dbd5d393dc12a91a1af9
Author: Emil Tsalapatis <[email protected]>

selftests/bpf: libarena: Disable IRQs during allocation

This commit updates the buddy allocator in libarena to use IRQ-saving
spinlocks to prevent ABBA deadlocks. These deadlocks were observed with
sched_ext schedulers when an IRQ interrupts an allocation and subsequently
attempts to take runqueue locks.

> diff --git a/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c b/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c
> index c674ee5cfcc1d..2490ab1396de0 100644
> --- a/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c
> +++ b/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c

[ ... ]

> @@ -730,9 +727,10 @@ static u64 buddy_alloc_from_existing_chunks(struct buddy __arena *buddy, int ord
>   */
>  static u64 buddy_alloc_from_new_chunk(struct buddy __arena *buddy, struct buddy_chunk __arena *chunk, int order)
>  {
> +	unsigned long flags;
>  	u64 address;
>  
> -	if (buddy_lock(buddy))
> +	if (buddy_lock(buddy, flags))
>  		return (u64)NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does this leak the newly
allocated arena chunk if the lock acquisition fails?

When buddy_alloc() needs more memory, it allocates a chunk and passes it
here:

buddy_alloc()
  chunk = buddy_chunk_get(buddy);
  if (chunk)
    address = buddy_alloc_from_new_chunk(buddy, chunk, order);

If buddy_lock() fails in buddy_alloc_from_new_chunk() (for example, returning
an error like -ETIMEDOUT under heavy contention), it returns NULL without
linking the new chunk to buddy->first_chunk. 

The NULL return then propagates up to buddy_alloc(), which discards the chunk
pointer completely without freeing it. Since BPF MCS spinlocks can fail under
heavy task load, could this predictably exhaust arena memory?

Should this error path call bpf_arena_free_pages() on the orphaned chunk
before returning?

>  
>  
> @@ -745,7 +743,7 @@ static u64 buddy_alloc_from_new_chunk(struct buddy __arena *buddy, struct buddy_
>  
>  	address = buddy_chunk_alloc(buddy->first_chunk, order);
>  
> -	buddy_unlock(buddy);
> +	buddy_unlock(buddy, flags);
>  
>  	return (u64)address;
>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3