Re: [PATCH] bpf: fix: Race condition in bpf_trampoline_unlink_cgroup_shim

[Martin KaFai Lau <martin.lau@linux.dev>]

On 2/5/26 11:13 PM, xulang wrote:
> Based on Martin KaFai Lau's suggestions, I have created a simple patch.

Thanks for the patch.

> 
> The root cause of this bug is that when `bpf_link_put` reduces the
> refcount of `shim_link->link.link` to zero, the resource is considered
> released but may still be referenced via `tr->progs_hlist` in
> `cgroup_shim_find`. The actual cleanup of `tr->progs_hlist` in
> `bpf_shim_tramp_link_release` is deferred. During this window, another
> process can cause a use-after-free via `bpf_trampoline_link_cgroup_shim`.
> 
> To fix this:
> 1. Add an atomic non-zero check in `bpf_trampoline_link_cgroup_shim`.
>     Only increment the refcount if it is not already zero.

This makes sense.

> 2. Guard the freeing of `shim_link` with `tr->mutex` to prevent release
>     while the mutex is held.

I am not sure about this one (details below).

> 
> Testing:
> I used a non-rigorous method to verify the fix by adding a delay in
> `bpf_link_put` to make the bug easier to trigger:
> 
> void bpf_link_put(struct bpf_link *link)
> {
>          if (!atomic64_dec_and_test(&link->refcnt))
>                  return;
> +       msleep(100);
>          INIT_WORK(&link->work, bpf_link_put_deferred);
>          schedule_work(&link->work);
> }
> 
> Before the patch, running a PoC easily reproduced the crash (often within
> dozens of iterations) with a call trace similar to KaiyanM's report.
> After the patch, the bug no longer occurs even after millions of
> iterations.
> 
> Signed-off-by: xulang <xulang@uniontech.com>
> ---
>   kernel/bpf/trampoline.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 976d89011b15..c16a53cca5e0 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -702,15 +702,23 @@ static void bpf_shim_tramp_link_release(struct bpf_link *link)
>   		return;
>   
>   	WARN_ON_ONCE(bpf_trampoline_unlink_prog(&shim_link->link, shim_link->trampoline, NULL));

bpf_trampoline_unlink_prog() will hold (/wait) for tr->mutex before 
unlinking from tr. The link's refcnt is already 0 here.


> -	bpf_trampoline_put(shim_link->trampoline);
>   }
>   
>   static void bpf_shim_tramp_link_dealloc(struct bpf_link *link)
>   {
>   	struct bpf_shim_tramp_link *shim_link =
>   		container_of(link, struct bpf_shim_tramp_link, link.link);
> +	struct bpf_trampoline *tr = shim_link->trampoline;
>   
> +	if (!tr) {
> +		kfree(shim_link);
> +		return;
> +	}
> +
> +	mutex_lock(&tr->mutex);

The link_release is done before the link_dealloc. Why it needs to hold 
(/wait) for the tr->mutex again?

>   	kfree(shim_link);
> +	mutex_unlock(&tr->mutex);
> +	bpf_trampoline_put(tr);
>   }
>   
>   static const struct bpf_link_ops bpf_shim_tramp_link_lops = {
> @@ -800,10 +808,8 @@ int bpf_trampoline_link_cgroup_shim(struct bpf_prog *prog,
>   	mutex_lock(&tr->mutex);
>   
>   	shim_link = cgroup_shim_find(tr, bpf_func);
> -	if (shim_link) {
> +	if (shim_link && atomic64_inc_not_zero(&shim_link->link.link.refcnt)) {

Use bpf_link_inc_not_zero().

pw-bot: cr

>   		/* Reusing existing shim attached by the other program. */
> -		bpf_link_inc(&shim_link->link.link);
> -
>   		mutex_unlock(&tr->mutex);
>   		bpf_trampoline_put(tr); /* bpf_trampoline_get above */
>   		return 0;