Re: [PATCH bpf-next v3] bpf: log error code on trampoline unlink failure

[Leon Hwang]

On 28/7/26 14:26, xu.xin16@xxxxxxxxxx wrote:
> From: xu xin <xu.xin16@xxxxxxxxxx>
> 
> Replace silent WARN_ON_ONCE with WARN_ONCE that prints the actual error
> code from bpf_trampoline_unlink_prog(). This aids debugging of race
> conditions during link teardown, while keeping the warning rate limited
> to avoid log flooding.
> 
> This will be very helpful for speeding up trouble-shooting of some crash
> UAF due to bpf_trampoline_unlink_prog failures.
> 
> No change to unlink behavior.
> 
> Signed-off-by: xu xin <xu.xin16@xxxxxxxxxx>


Better to use Xu Xin as your formal name.

> ---
> Change log
> V2->v3: 
> Add up the case in bpf_trampoline_multi_detach. And remove the unneeded 'if (err)'
> 
> v1->v2:
> 1) clean the subject name by remove 'syscall' suggested by Leon Hwang
> https://lore.kernel.org/all/20a444b2-aeed-4af8-ba76-e994e2c14087@xxxxxxxxx/
> 2) Add up the missed case in kernel/bpf/trampoline.c:bpf_shim_tramp_link_release()
> 
>  kernel/bpf/syscall.c    |  8 +++++---
>  kernel/bpf/trampoline.c | 12 ++++++++----
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 6db306d23b47..52c57f8beb37 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3626,10 +3626,12 @@ static void bpf_tracing_link_release(struct bpf_link *link)
>  {
>  	struct bpf_tracing_link *tr_link =
>  		container_of(link, struct bpf_tracing_link, link.link);
> +	int err;
> 
> -	WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link.node,
> -						tr_link->trampoline,
> -						tr_link->tgt_prog));
> +	err = bpf_trampoline_unlink_prog(&tr_link->link.node,
> +					tr_link->trampoline,
> +					tr_link->tgt_prog);
> +	WARN_ONCE(err, "bpf_trampoline_unlink_prog returns error: %d\n", err);
> 
>  	bpf_trampoline_put(tr_link->trampoline);
> 
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 1a721fc4bef5..a34ddb97ff6b 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -997,12 +997,15 @@ static void bpf_shim_tramp_link_release(struct bpf_link *link)
>  {
>  	struct bpf_shim_tramp_link *shim_link =
>  		container_of(link, struct bpf_shim_tramp_link, link.link);
> +	int err;
> 
>  	/* paired with 'shim_link->trampoline = tr' in bpf_trampoline_link_cgroup_shim */
>  	if (!shim_link->trampoline)
>  		return;
> 
> -	WARN_ON_ONCE(bpf_trampoline_unlink_prog(&shim_link->link.node, shim_link->trampoline, NULL));
> +	err = bpf_trampoline_unlink_prog(&shim_link->link.node, shim_link->trampoline, NULL);
> +	WARN_ONCE(err, "bpf_trampoline_unlink_prog returns error: %d\n", err);
> +
>  	bpf_trampoline_put(shim_link->trampoline);
>  }
> 
> @@ -1712,15 +1715,16 @@ int bpf_trampoline_multi_detach(struct bpf_prog *prog, struct bpf_tracing_multi_
>  {
>  	struct bpf_tracing_multi_data *data = &link->data;
>  	struct bpf_tracing_multi_node *mnode;
> -	int i;
> +	int i, err;
> 
>  	trampoline_lock_all();
> 
>  	for_each_mnode(mnode, link) {
>  		data->entry = &mnode->entry;
>  		bpf_trampoline_multi_attach_init(mnode->trampoline);
> -		WARN_ON_ONCE(__bpf_trampoline_unlink_prog(&mnode->node, mnode->trampoline,
> -					NULL, &trampoline_multi_ops, data));
> +		err = __bpf_trampoline_unlink_prog(&mnode->node, mnode->trampoline,NULL,


NIT: a space is missing at the position between ',' and 'NULL'.

> +					&trampoline_multi_ops, data);

NIT: align '&' with previous line's '&'.

Other than that,

Acked-by: Leon Hwang <leon.hwang@xxxxxxxxx>

> +		WARN_ONCE(err, "__bpf_trampoline_unlink_prog returns error: %d\n", err);
>  	}
> 
>  	if (ftrace_hash_count(data->unreg))