Re: [PATCH bpf-next/net 1/5] bpf: Register mptcp common kfunc set

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

On 11/8/24 7:52 AM, Matthieu Baerts (NGI0) wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> MPTCP helper mptcp_sk() is used to convert struct sock to mptcp_sock.
> Helpers mptcp_subflow_ctx() and mptcp_subflow_tcp_sock() are used to
> convert between struct mptcp_subflow_context and sock. They all will
> be used in MPTCP BPF programs too.
> 
> This patch defines corresponding wrappers of them, and put the
> wrappers into mptcp common kfunc set and register the set with the
> flag BPF_PROG_TYPE_UNSPEC to let them accessible to all types of BPF
> programs.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> Reviewed-by: Mat Martineau <martineau@kernel.org>
> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>   net/mptcp/bpf.c | 40 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index 8a16672b94e2384f5263e1432296cbca1236bb30..6f96a5927fd371f8ea92cbf96c875edef9272b98 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -29,8 +29,46 @@ static const struct btf_kfunc_id_set bpf_mptcp_fmodret_set = {
>   	.set   = &bpf_mptcp_fmodret_ids,
>   };
>   
> +__bpf_kfunc_start_defs();
> +
> +__bpf_kfunc static struct mptcp_sock *bpf_mptcp_sk(struct sock *sk)
> +{
> +	return mptcp_sk(sk);
> +}
> +
> +__bpf_kfunc static struct mptcp_subflow_context *
> +bpf_mptcp_subflow_ctx(const struct sock *sk)
> +{
> +	return mptcp_subflow_ctx(sk);

This returns "struct mptcp_subflow_context *" without checking the sk is a mptcp 
subflow or not...

> +}
> +
> +__bpf_kfunc static struct sock *
> +bpf_mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
> +{
> +	return mptcp_subflow_tcp_sock(subflow);

...and then the "struct mptcp_subflow_context *" can be used by this kfunc here. 
Is it really safe?

> +}
> +
> +__bpf_kfunc_end_defs();
> +
> +BTF_KFUNCS_START(bpf_mptcp_common_kfunc_ids)
> +BTF_ID_FLAGS(func, bpf_mptcp_sk)
> +BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx)
> +BTF_ID_FLAGS(func, bpf_mptcp_subflow_tcp_sock)

All of them has no KF_TRUSTED_ARGS or KF_RCU, so the returned ptr is supposed to 
be read-only? Why are they needed and why bpf_rdonly_cast (aka the bpf_core_cast 
in libbpf) cannot be used?

pw-bot: cr

> +BTF_KFUNCS_END(bpf_mptcp_common_kfunc_ids)
> +
> +static const struct btf_kfunc_id_set bpf_mptcp_common_kfunc_set = {
> +	.owner	= THIS_MODULE,
> +	.set	= &bpf_mptcp_common_kfunc_ids,
> +};
> +
>   static int __init bpf_mptcp_kfunc_init(void)
>   {
> -	return register_btf_fmodret_id_set(&bpf_mptcp_fmodret_set);
> +	int ret;
> +
> +	ret = register_btf_fmodret_id_set(&bpf_mptcp_fmodret_set);
> +	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_UNSPEC,
> +					       &bpf_mptcp_common_kfunc_set);
> +
> +	return ret;
>   }
>   late_initcall(bpf_mptcp_kfunc_init);
>