Re: [PATCH mptcp-next v17 6/8] mptcp: add last_snd write access

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Thu, 28 Apr 2022, Geliang Tang wrote:

> This patch exports the member last_snd of struct mptcp_sock in
> bpf_mptcp_helpers.h, and adds BPF write access to it.
>

I still think we should stay away from giving the BPF functions write 
access to the msk unless it's really necessary.

In this case, both of the example schedulers write identical values to 
msk->last_snd and data->sock. Will they ever be different? Seems better to 
keep it simple for the BPF code and have the get_subflow wrappers handle 
assigning to msk->last_snd if a BPF function was called, and let the 
built-in scheduler assign last_snd itself.

- Mat

> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/bpf.c                                 | 15 ++++++++++++---
> tools/testing/selftests/bpf/bpf_mptcp_helpers.h |  1 +
> 2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index a085c72fe695..300493f9a2b6 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -18,8 +18,8 @@
> #ifdef CONFIG_BPF_JIT
> extern struct bpf_struct_ops bpf_mptcp_sched_ops;
> extern struct btf *btf_vmlinux;
> -static const struct btf_type *mptcp_sched_type __read_mostly;
> -static u32 mptcp_sched_id;
> +static const struct btf_type *mptcp_sock_type, *mptcp_sched_type __read_mostly;
> +static u32 mptcp_sock_id, mptcp_sched_id;
>
> static u32 optional_ops[] = {
> 	offsetof(struct mptcp_sched_ops, init),
> @@ -47,12 +47,15 @@ static int bpf_mptcp_sched_btf_struct_access(struct bpf_verifier_log *log,
> 		return btf_struct_access(log, btf, t, off, size, atype,
> 					 next_btf_id, flag);
>
> -	if (t != mptcp_sched_type) {
> +	if (t != mptcp_sock_type && t != mptcp_sched_type) {
> 		bpf_log(log, "only access to mptcp_sched is supported\n");
> 		return -EACCES;
> 	}
>
> 	switch (off) {
> +	case offsetof(struct mptcp_sock, last_snd):
> +		end = offsetofend(struct mptcp_sock, last_snd);
> +		break;
> 	case offsetof(struct mptcp_sched_data, sock):
> 		end = offsetofend(struct mptcp_sched_data, sock);
> 		break;
> @@ -145,6 +148,12 @@ static int bpf_mptcp_sched_init(struct btf *btf)
> {
> 	s32 type_id;
>
> +	type_id = btf_find_by_name_kind(btf, "mptcp_sock", BTF_KIND_STRUCT);
> +	if (type_id < 0)
> +		return -EINVAL;
> +	mptcp_sock_id = type_id;
> +	mptcp_sock_type = btf_type_by_id(btf, mptcp_sock_id);
> +
> 	type_id = btf_find_by_name_kind(btf, "mptcp_sched_data",
> 					BTF_KIND_STRUCT);
> 	if (type_id < 0)
> diff --git a/tools/testing/selftests/bpf/bpf_mptcp_helpers.h b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
> index a0b83fbe8133..79a16636400b 100644
> --- a/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
> @@ -27,6 +27,7 @@ struct mptcp_sched_ops {
> struct mptcp_sock {
> 	struct inet_connection_sock	sk;
>
> +	struct sock	*last_snd;
> 	__u32		token;
> 	struct sock	*first;
> 	struct mptcp_sched_ops *sched;
> -- 
> 2.34.1
>
>
>

--
Mat Martineau
Intel