Re: [PATCH bpf-next v2 1/2] bpf: Keep target extended until its last freplace link detaches

[Leon Hwang <leon.hwang@linux.dev>]

On 24/9/26 10:37, chenyuan_fl@163.com wrote:
> From: Yuan Chen <chenyuan@kylinos.cn>
> 
> The is_extended / prog_array_member_cnt protocol introduced by commit
> d6083f040d5d ("bpf: Prevent tailcall infinite loop caused by freplace")
> keeps a prog extended by a freplace program out of prog_array maps, and
> vice versa: once a tail call re-enters an extended subprogram, its
> tail_call_cnt resets on every execution and the loop never terminates.
> 
> But is_extended is a plain boolean, while one target prog can carry
> several freplace links at the same time, one on its entry and one on a
> global subprogram.  __bpf_trampoline_unlink_prog() cleared is_extended
> whenever *any* freplace link detached, so detaching one of two links
> re-armed the unbounded loop through the remaining one.

A prog may have multiple global subprogs. And each of the subprogs can
be attached with freplace prog. When all the subprogs are attached with
freplace progs then detach one of the freplace prog, the *is_extended*
becomes *false*, which relaxes the restriction between tailcall and
freplace introduced by the commit d6083f040d5d ("bpf: Prevent tailcall
infinite loop caused by freplace").

> 
> Replace the is_extended boolean with a count of the freplace links
> attached to each target prog, so the target stays extended until its
> last link detaches.  Also rename bpf_freplace_check_tgt_prog() to
> bpf_freplace_link_tgt_prog(), as the helper has never been a pure
> check: it reserves the target prog on success.
> 
> Fixes: d6083f040d5d ("bpf: Prevent tailcall infinite loop caused by freplace")
> Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
> 

[...]

> @@ -933,7 +933,7 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_node *node,
>  		/* Cannot attach extension if fentry/fexit are in use. */
>  		if (cnt)
>  			return -EBUSY;
> -		err = bpf_freplace_check_tgt_prog(tgt_prog);
> +		err = bpf_freplace_link_tgt_prog(tgt_prog);
>  		if (err)
>  			return err;
>  		tr->extension_prog = node->link->prog;

                return bpf_arch_text_poke(tr->func.addr, BPF_MOD_NOP,
                                          BPF_MOD_JUMP, NULL,
                                          node->link->prog->bpf_func);


I think there are existing issues here: if bpf_arch_text_poke() returns
error, the tr->extension_prog and aux->freplace_link_cnt should be
rollbacked.

Thanks,
Leon

> @@ -979,7 +979,7 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_node *node,
>  					 tr->extension_prog->bpf_func, NULL);
>  		tr->extension_prog = NULL;
>  		guard(mutex)(&tgt_prog->aux->ext_mutex);
> -		tgt_prog->aux->is_extended = false;
> +		tgt_prog->aux->freplace_link_cnt--;
>  		return err;
>  	}
>  	bpf_trampoline_remove_prog(tr, node);