On Mon, Sep 9, 2024 at 10:37 PM Geliang Tang <geliang@xxxxxxxxxx> wrote:
>
> From: Geliang Tang <tanggeliang@xxxxxxxxxx>
>
> It's necessary to traverse all subflows on the conn_list of an MPTCP
> socket and then call kfunc to modify the fields of each subflow. In
> kernel space, mptcp_for_each_subflow() helper is used for this:
>
> mptcp_for_each_subflow(msk, subflow)
> kfunc(subflow);
>
> But in the MPTCP BPF program, this has not yet been implemented. As
> Martin suggested recently, this conn_list walking + modify-by-kfunc
> usage fits the bpf_iter use case. So this patch adds a new bpf_iter
> type named "mptcp_subflow" to do this and implements its helpers
> bpf_iter_mptcp_subflow_new()/_next()/_destroy().
>
> Then bpf_for_each() for mptcp_subflow can be used in BPF program like
> this:
>
> bpf_rcu_read_lock();
> bpf_for_each(mptcp_subflow, subflow, msk)
> kfunc(subflow);
> bpf_rcu_read_unlock();
>
> Suggested-by: Martin KaFai Lau <martin.lau@xxxxxxxxxx>
> Signed-off-by: Geliang Tang <tanggeliang@xxxxxxxxxx>
> ---
> net/mptcp/bpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 43 insertions(+), 4 deletions(-)
>
Not sure why, but only this patch made it to the BPF mailing list? Did
you cc bpf@vger on all patches?
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index 6414824402e6..350672e24a3d 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -201,9 +201,48 @@ static const struct btf_kfunc_id_set bpf_mptcp_fmodret_set = {
> .set = &bpf_mptcp_fmodret_ids,
> };
>
> -__diag_push();
> -__diag_ignore_all("-Wmissing-prototypes",
> - "kfuncs which will be used in BPF programs");
> +struct bpf_iter_mptcp_subflow {
> + __u64 __opaque[2];
> +} __attribute__((aligned(8)));
> +
> +struct bpf_iter_mptcp_subflow_kern {
> + struct mptcp_sock *msk;
> + struct list_head *pos;
> +} __attribute__((aligned(8)));
> +
> +__bpf_kfunc_start_defs();
> +
> +__bpf_kfunc int bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
> + struct mptcp_sock *msk)
> +{
> + struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
> +
> + if (!msk)
> + return -EINVAL;
you still need to initialize at least kit->msk to NULL to prevent next
implementation below doing the wrong thing
keep in mind, iterator constructor returning error doesn't prevent BPF
program from still calling next() and destroy(), so implementation has
to set iterator state such that next can return NULL if iterator was
never successfully initialized
pw-bot: cr
> +
> + kit->msk = msk;
> + kit->pos = &msk->conn_list;
> + return 0;
> +}
> +
> +__bpf_kfunc struct mptcp_subflow_context *
> +bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it)
> +{
> + struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
> + struct mptcp_subflow_context *subflow;
> + struct mptcp_sock *msk = kit->msk;
> +
you should check if (!msk) early here, before accessing kit->pos->next below
> + subflow = list_entry((kit->pos)->next, struct mptcp_subflow_context, node);
nit: why () around kit->pos?
> + if (!msk || list_entry_is_head(subflow, &msk->conn_list, node))
as I mentioned, !msk check seems too late. Maybe list_entry_is_head()
is a bit too late as well?
> + return NULL;
> +
> + kit->pos = &subflow->node;
> + return subflow;
> +}
> +
> +__bpf_kfunc void bpf_iter_mptcp_subflow_destroy(struct bpf_iter_mptcp_subflow *it)
> +{
> +}
>
> __bpf_kfunc struct mptcp_subflow_context *
> bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos)
> @@ -218,7 +257,7 @@ __bpf_kfunc bool bpf_mptcp_subflow_queues_empty(struct sock *sk)
> return tcp_rtx_queue_empty(sk);
> }
>
> -__diag_pop();
> +__bpf_kfunc_end_defs();
>
> BTF_KFUNCS_START(bpf_mptcp_sched_kfunc_ids)
> BTF_ID_FLAGS(func, mptcp_subflow_set_scheduled)
> --
> 2.43.0
>
>
