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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9b373a23-c093-42d8-b4ae-99f2e62e7681@linux.dev>
Date: Thu, 23 Jan 2025 17:26:41 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Geliang Tang <geliang@...nel.org>
Cc: "Matthieu Baerts (NGI0)" <matttbe@...nel.org>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Mat Martineau <martineau@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
 Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
 Andrii Nakryiko <andrii@...nel.org>, Eduard Zingerman <eddyz87@...il.com>,
 Song Liu <song@...nel.org>, Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>,
 Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>,
 Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, bpf@...r.kernel.org,
 linux-kselftest@...r.kernel.org, mptcp@...ts.linux.dev
Subject: Re: [PATCH bpf-next/net v2 5/7] bpf: Acquire and release mptcp socket

On 12/19/24 7:46 AM, Matthieu Baerts (NGI0) wrote:
> From: Geliang Tang <tanggeliang@...inos.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@...inos.cn>
> Reviewed-by: Mat Martineau <martineau@...nel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@...nel.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 = {
>