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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Reviewed-by: Mat Martineau <martineau@xxxxxxxxxx>
> > Reviewed-by: Matthieu Baerts (NGI0) <matttbe@xxxxxxxxxx>
> > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@xxxxxxxxxx>
> > ---
> > 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);
> >
