Re: [PATCH bpf-next/net v2 5/7] bpf: Acquire and release mptcp socket

[Martin KaFai Lau <martin.lau@linux.dev>]

On 12/19/24 7:46 AM, Matthieu Baerts (NGI0) wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The KF_TRUSTED_ARGS flag is used for bpf_iter_mptcp_subflow_new, it
> indicates that the all pointer arguments are valid. It's necessary to
> add a KF_ACQUIRE helper to get valid "msk".

This feels wrong. It forces an unnecessary acquire to get around the verifier. 
bpf_sockopt->sk should be in "trusted". From looking at patch 7, the issue 
should be the return value of bpf_skc_to_mptcp_sock().

> 
> This patch adds bpf_mptcp_sock_acquire() and bpf_mptcp_sock_release()
> helpers for this. Increase sk->sk_refcnt in _acquire() and decrease it
> in _release(). Register them with KF_ACQUIRE flag and KF_RELEASE flag.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> Reviewed-by: Mat Martineau <martineau@kernel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>   net/mptcp/bpf.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index e39f0e4fb683c1aa31ee075281daee218dac5878..d50bd1ea7f6d0ff1abff32deef9a98b98ee8f42c 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -97,6 +97,23 @@ bpf_iter_mptcp_subflow_destroy(struct bpf_iter_mptcp_subflow *it)
>   {
>   }
>   
> +__bpf_kfunc static struct
> +mptcp_sock *bpf_mptcp_sock_acquire(struct mptcp_sock *msk)
> +{
> +	struct sock *sk = (struct sock *)msk;
> +
> +	if (sk && refcount_inc_not_zero(&sk->sk_refcnt))
> +		return msk;
> +	return NULL;
> +}
> +
> +__bpf_kfunc static void bpf_mptcp_sock_release(struct mptcp_sock *msk)
> +{
> +	struct sock *sk = (struct sock *)msk;
> +
> +	WARN_ON_ONCE(!sk || !refcount_dec_not_one(&sk->sk_refcnt));
> +}
> +
>   __bpf_kfunc_end_defs();
>   
>   BTF_KFUNCS_START(bpf_mptcp_common_kfunc_ids)
> @@ -104,6 +121,8 @@ BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx, 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, bpf_mptcp_sock_acquire, KF_ACQUIRE | KF_RET_NULL)

It should need a KF_TRUSTED_ARGS here but then it will hit the same problem 
described in the commit message.

Instead of changing the verifier to get this work, one option is to use the 
"struct sock *sk" instead of "struct mptcp-sock *msk" as the argument in the 
bpf_iter_mptcp_subflow_new, and do the bpf_mptcp_sock_from_sock check in the 
bpf_iter_mptcp_subflow_new.

> +BTF_ID_FLAGS(func, bpf_mptcp_sock_release, KF_RELEASE)
>   BTF_KFUNCS_END(bpf_mptcp_common_kfunc_ids)
>   
>   static const struct btf_kfunc_id_set bpf_mptcp_common_kfunc_set = {
>