Re: [PATCH bpf-next v1 4/7] selftests/bpf: libarena: Handle NMI reentrancy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: buddy_free() overwrites error with deferred free status
--

commit 6b8ade1832d5b23360c4e6c8d75625a18af4fe8e
Author: Emil Tsalapatis <emil@etsalapatis.com>

selftests/bpf: libarena: Handle NMI reentrancy

This commit adds per-CPU reentrancy protection to the libarena buddy cache
and defers frees issued from NMI context onto an intrusive list when the
local cache cannot be used.

> diff --git a/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c b/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c
> index ce05e3e58ebae..85d670b8762bb 100644
> --- a/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c
> +++ b/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c

[ ... ]

> @@ -1100,12 +1244,25 @@ __weak int buddy_free(struct buddy __arena *buddy, void __arena *addr)
>  	if (!ret)
>  		return 0;
>  
> +	if (bpf_in_nmi()) {
> +		buddy_deferred_free_start(buddy, addr);
> +		return 0;
> +	}
> +
>  	ret = buddy_lock(buddy, flags);
>  	if (ret)
>  		return ret;
>  
>  	ret = buddy_free_unlocked(buddy, (u64)addr);
>  
> +	/* Drain any frees stranded during an NMI. */
> +	if (unlikely(buddy_deferred_free_pending(buddy))) {
> +		ret = buddy_deferred_free_complete(buddy);
> +		if (ret)
> +			goto out_unlock;
> +	}
> +
> +out_unlock:
>  	buddy_unlock(buddy, flags);
>  
>  	return ret;

[Severity: Medium]
In buddy_free() in tools/testing/selftests/bpf/libarena/src/buddy.bpf.c, if
buddy_free_unlocked(buddy, (u64)addr) fails (for example, returning -EINVAL
on an unaligned or invalid address) while buddy_deferred_free_pending(buddy)
is true, calling buddy_deferred_free_complete(buddy) unconditionally
overwrites ret. If draining the deferred list succeeds, ret is set to 0 and
the error from buddy_free_unlocked() is lost.

Also, the goto out_unlock statement jumps directly to the immediately
adjacent out_unlock label.

Should buddy_free() preserve any error from buddy_free_unlocked() in the
same way buddy_free_bulk() does:

	if (unlikely(buddy_deferred_free_pending(buddy))) {
		err = buddy_deferred_free_complete(buddy);
		ret = ret ?: err;
	}

and drop the redundant out_unlock label?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020181900.6755-1-emil@etsalapatis.com?part=4