Re: [PATCH mptcp-next v3 2/3] Squash to "bpf: Add mptcp_subflow bpf_iter"

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 30/05/2025 10:03, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Define .filter as Martin suggested.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/bpf.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index 113aad086b37..e52821bae49c 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -305,9 +305,6 @@ __bpf_kfunc_end_defs();
>  BTF_KFUNCS_START(bpf_mptcp_common_kfunc_ids)
>  BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx, KF_RET_NULL)
>  BTF_ID_FLAGS(func, bpf_mptcp_subflow_tcp_sock, KF_RET_NULL)
> -BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_new, KF_ITER_NEW | KF_TRUSTED_ARGS)
> -BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_next, KF_ITER_NEXT | KF_RET_NULL)
> -BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_destroy, KF_ITER_DESTROY)
>  BTF_ID_FLAGS(func, mptcp_subflow_set_scheduled)
>  BTF_ID_FLAGS(func, mptcp_subflow_active)
>  BTF_ID_FLAGS(func, mptcp_set_timeout)
> @@ -338,6 +335,31 @@ static const struct btf_kfunc_id_set bpf_mptcp_common_kfunc_set = {
>  	.filter	= bpf_mptcp_common_kfunc_filter,
>  };
>  
> +/* .filter is called sometimes before prog->aux->st_ops is set,
> + * then prog->aux->st_ops is NULL. If return -EACCES in this case,
> + * an error occur:
> + * 	bug: bad parent state for iter next call
> + */
> +BTF_KFUNCS_START(bpf_mptcp_iter_kfunc_ids)
> +BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_new, KF_ITER_NEW | KF_TRUSTED_ARGS)
> +BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_next, KF_ITER_NEXT | KF_RET_NULL)
> +BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_destroy, KF_ITER_DESTROY)
> +BTF_KFUNCS_END(bpf_mptcp_iter_kfunc_ids)
> +
> +static int bpf_mptcp_iter_kfunc_filter(const struct bpf_prog *prog, u32 kfunc_id)
> +{
> +	if (btf_id_set8_contains(&bpf_mptcp_iter_kfunc_ids, kfunc_id) &&
> +	    prog->type != BPF_PROG_TYPE_STRUCT_OPS)

Correct me if I'm wrong, but I think this filter is not needed: it
simply restricts to (any) BPF_PROG_TYPE_STRUCT_OPS, which is already the
case without this filter because this kfunc set is only registered for
this program type, no?

When applying the patches, I removed this function, the corresponding
.filter, and the comment explaining the error with .filter (at the end,
this iterator can be used elsewhere, and it is cleaner like that).
Please tell me if that was not correct.

> +		return -EACCES;
> +	return 0;
> +}
> +
> +static const struct btf_kfunc_id_set bpf_mptcp_iter_kfunc_set = {
> +	.owner	= THIS_MODULE,
> +	.set	= &bpf_mptcp_iter_kfunc_ids,
> +	.filter	= bpf_mptcp_iter_kfunc_filter,
> +};
> +
>  static int __init bpf_mptcp_kfunc_init(void)
>  {
>  	int ret;
> @@ -345,6 +367,8 @@ static int __init bpf_mptcp_kfunc_init(void)
>  	ret = register_btf_fmodret_id_set(&bpf_mptcp_fmodret_set);
>  	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS,
>  					       &bpf_mptcp_common_kfunc_set);
> +	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS,
> +					       &bpf_mptcp_iter_kfunc_set);
>  #ifdef CONFIG_BPF_JIT
>  	ret = ret ?: register_bpf_struct_ops(&bpf_mptcp_sched_ops, mptcp_sched_ops);
>  #endif

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.