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

[Geliang Tang <geliang@kernel.org>]

Hi Martin,

Thanks for the review.

On Mon, Nov 11, 2024 at 04:25:52PM -0800, Martin KaFai Lau wrote:
> 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...

I checked it in patch 5 in the bpf program. I'll move this check into
bpf_mptcp_subflow_ctx() in v2.

> 
> > +}
> > +
> > +__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?

I'll add null-check for subflow here too in v2.

> 
> > +}
> > +
> > +__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

KF_TRUSTED_ARGS should be added for bpf_mptcp_sk() indeed, but not for
bpf_mptcp_subflow_ctx() or bpf_mptcp_subflow_tcp_sock(), since the parameters
'subflow' or 'sk' of the latter two functions are not pointers which are
passed as tracepoint or struct_ops callback arguments, but pointers returned
from kfuncs. I'll add KF_RET_NULL flag for all of them.

> supposed to be read-only? Why are they needed and why bpf_rdonly_cast (aka
> the bpf_core_cast in libbpf) cannot be used?

The returned ptrs will pass to kfuncs. If bpf_rdonly_cast() is used here, a
"untrusted_ptr_" error occurs:

# ; msk = bpf_core_cast(sk, struct mptcp_sock); @ mptcp_bpf_iters.c:29
# 9: (18) r2 = 0x3f95                   ; R2_w=16277
# 11: (85) call bpf_rdonly_cast#53914   ; R0_w=untrusted_ptr_mptcp_sock()
# ; if (!msk || msk->pm.server_side || !msk->pm.subflows) @ mptcp_bpf_iters.c:30
# 12: (15) if r0 == 0x0 goto pc+50      ; R0_w=untrusted_ptr_mptcp_sock()
# 13: (71) r1 = *(u8 *)(r0 +2785)       ; R0_w=untrusted_ptr_mptcp_sock() R1_w=scalar(smin=smin32=0,smax=umax=smax32=umax32=255,var_off=(0x0; 0xff))
# 14: (56) if w1 != 0x0 goto pc+48      ; R1_w=0
# 15: (71) r1 = *(u8 *)(r0 +2794)       ; R0_w=untrusted_ptr_mptcp_sock() R1_w=scalar(smin=smin32=0,smax=umax=smax32=umax32=255,var_off=(0x0; 0xff))
# 16: (16) if w1 == 0x0 goto pc+46      ; R1_w=scalar(smin=umin=smin32=umin32=1,smax=umax=smax32=umax32=255,var_off=(0x0; 0xff))
# ; msk = bpf_mptcp_sock_acquire(msk); @ mptcp_bpf_iters.c:33
# 17: (bf) r1 = r0                      ; R0_w=untrusted_ptr_mptcp_sock() R1_w=untrusted_ptr_mptcp_sock()
# 18: (85) call bpf_mptcp_sock_acquire#24762
# arg#0 is untrusted_ptr_ expected ptr_ or socket
# processed 18 insns (limit 1000000) max_states_per_insn 0 total_states 1 peak_states 1 mark_read 1
# -- END PROG LOAD LOG --

And I don't know how to fix it. So I added a new kfunc wrapper
bpf_mptcp_sk() instead.

Please give me some advice, thanks.

-Geliang

> 
> 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);
> >