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

[Leon Hwang]

On 23/9/26 17:06, chenyuan_fl@xxxxxxx wrote:
> From: Yuan Chen <chenyuan@xxxxxxxxxx>
> 
> 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.
> 
> 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")

You should cc me when posting the next revision.

You can get the cc list by ./scripts/get_maintainer.pl /path/to/file.patch.

> Signed-off-by: Yuan Chen <chenyuan@xxxxxxxxxx>
> ---
>  include/linux/bpf.h     |  4 ++--
>  kernel/bpf/arraymap.c   |  2 +-
>  kernel/bpf/trampoline.c | 10 ++++++----
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 

[...]

>  
> @@ -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;
> @@ -979,7 +979,9 @@ 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;
> +		if (WARN_ON_ONCE(!tgt_prog->aux->freplace_link_cnt))

Is this WARN_ON_ONCE() necessary?

Thanks,
Leon

> +			return err;
> +		tgt_prog->aux->freplace_link_cnt--;
>  		return err;
>  	}
>  	bpf_trampoline_remove_prog(tr, node);