Re: [PATCH mptcp-next v1 1/4] bpf: Add mptcp path manager struct_ops

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 21/03/2025 02:49, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch implements a new struct bpf_struct_ops for MPTCP BPF path
> manager: bpf_mptcp_pm_ops. Register and unregister the bpf path manager
> in .reg and .unreg.
> 
> Add write access for some fields of struct mptcp_sock and struct
> mptcp_pm_addr_entry in .btf_struct_access.
> 
> This MPTCP BPF path manager implementation is similar to BPF TCP CC. And
> net/ipv4/bpf_tcp_ca.c is a frame of reference for this patch.

(...)

> +static int bpf_mptcp_pm_btf_struct_access(struct bpf_verifier_log *log,
> +					  const struct bpf_reg_state *reg,
> +					  int off, int size)

I don't know how it works exactly, but with BPF, can we not force a
program to automatically take a lock (pm->lock) when trying to modify
any of the fields below?

Also, is there really a need for a BPF PM to modify any of these fields
directly?

Are most of them handled either by pm.c before calling a callback or are
specific to the in-kernel PM?

(...)

> +static struct mptcp_pm_ops __bpf_mptcp_pm_ops = {
> +	.get_local_id		= __bpf_mptcp_pm_get_local_id,
> +	.get_priority		= __bpf_mptcp_pm_get_priority,
> +	.established		= __bpf_mptcp_pm_established,
> +	.subflow_established	= __bpf_mptcp_pm_subflow_established,
> +	.allow_new_subflow      = __bpf_mptcp_pm_allow_new_subflow,
> +	.accept_new_subflow     = __bpf_mptcp_pm_accept_new_subflow,

There is a mix of spaces and tabs here above. Only use tabs?

> +	.add_addr_echo		= __bpf_mptcp_pm_add_addr_echo,
> +	.add_addr_received	= __bpf_mptcp_pm_add_addr_received,
> +	.rm_addr_received	= __bpf_mptcp_pm_rm_addr_received,
> +	.init			= __bpf_mptcp_pm_init,
> +	.release		= __bpf_mptcp_pm_release,
> +};

(...)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.