Re: [PATCH bpf-next v4 1/4] bpf: Prevent updating extended prog to prog_array map

[Eduard Zingerman <eddyz87@gmail.com>]

On Sun, 2024-09-29 at 21:27 +0800, Leon Hwang wrote:

[...]

> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 79660e3fca4c1..4a4de4f014be9 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -947,16 +947,29 @@ static void *prog_fd_array_get_ptr(struct bpf_map *map,
>  				   struct file *map_file, int fd)
>  {
>  	struct bpf_prog *prog = bpf_prog_get(fd);
> +	bool is_extended;
>  
>  	if (IS_ERR(prog))
>  		return prog;
>  
> -	if (!bpf_prog_map_compatible(map, prog)) {
> -		bpf_prog_put(prog);
> -		return ERR_PTR(-EINVAL);
> -	}
> +	if (!bpf_prog_map_compatible(map, prog))
> +		goto out_put_prog;
> +
> +	mutex_lock(&prog->aux->ext_mutex);
> +	is_extended = prog->aux->is_extended;
> +	mutex_unlock(&prog->aux->ext_mutex);
> +	if (is_extended)
> +		/* Extended prog can not be tail callee. It's to prevent a
> +		 * potential infinite loop like:
> +		 * tail callee prog entry -> tail callee prog subprog ->
> +		 * freplace prog entry --tailcall-> tail callee prog entry.
> +		 */
> +		goto out_put_prog;

Nit: I think return value should be -EBUSY in this case.

>  
>  	return prog;
> +out_put_prog:
> +	bpf_prog_put(prog);
> +	return ERR_PTR(-EINVAL);
>  }
>

[...]

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index a8f1808a1ca54..db17c52fa35db 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3212,14 +3212,23 @@ 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);
> -
> -	WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link,
> -						tr_link->trampoline));
> +	struct bpf_prog *tgt_prog = tr_link->tgt_prog;
> +
> +	if (link->prog->type == BPF_PROG_TYPE_EXT) {
> +		mutex_lock(&tgt_prog->aux->ext_mutex);
> +		WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link,
> +							tr_link->trampoline));
> +		tgt_prog->aux->is_extended = false;

In case if unlink fails is_extended should not be reset.

> +		mutex_unlock(&tgt_prog->aux->ext_mutex);
> +	} else {
> +		WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link,
> +							tr_link->trampoline));
> +	}
>  
>  	bpf_trampoline_put(tr_link->trampoline);
>  
>  	/* tgt_prog is NULL if target is a kernel function */
> -	if (tr_link->tgt_prog)
> +	if (tgt_prog)
>  		bpf_prog_put(tr_link->tgt_prog);
>  }

[...]