Re: [PATCH mptcp-next v4 0/5] Squash to "Add mptcp_subflow bpf_iter support"

[Mat Martineau <martineau@kernel.org>]

On Mon, 24 Feb 2025, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> v4:
> - drop sock_owned_by_user_nocheck and spin_is_locked. According to
>   comments from Mat and Martin, in this set mptcp_subflow
>   bpf_iter only used from a cg sockopt bpf prog, no need to add these
>   check at this moment.

Hi Geliang -

Sorry to continue the churn on this. I looked at Martin's comment in 
https://lore.kernel.org/mptcp/fdf0ddbe-e007-4a5f-bbdf-9a144e8fbe35@linux.dev/ 
where he says:

"For the cg get/setsockopt hook here, the lock should have already been 
held earlier in the kernel."

For the getsockopt case, there is a file layer lock held that ensures the 
'struct socket' remains valid while this hook is run, but no lock is 
acquired that prevents changes to msk->conn_list. It's not safe to iterate 
over the conn_list without protection from lock_sock() / unlock_sock().

As Matthieu noted in 
https://lore.kernel.org/mptcp/3b5af48e-4155-4b98-b67b-b75d9fb6285e@kernel.org/, 
when used in a bpf scheduler the msk lock will be held. But the test code 
called via getsockopt is a case where it's not protected, and a change to 
the linked list during iteration could lead to undefined behavior.

I'll talk about it with Matthieu in the meeting this week too.

- Mat


>
> v3:
> - patch 3, continue to use sock_owned_by_user_nocheck() and spin_is_locked()
> checks instead of using msk_owned_by_me().
> - patch 5, drop declaration of bpf_mptcp_subflow_tcp_sock. It's no longer
> used.
> - patch 5, update the comment for mptcp_subflow_tcp_sock(), which is a BPF
> helper, not a kfunc.
>
> The commit log of "bpf: Register mptcp common kfunc set" doesn't match the
> code, please update it as:
>
> '''
> bpf: Register mptcp common kfunc set
>
> MPTCP helper mptcp_subflow_ctx() is used to convert struct sock to
> struct mptcp_subflow_context. It will be used in MPTCP BPF programs.
>
> This patch defines corresponding wrapper of this helper, and put it
> into the newly defined mptcp common kfunc set and register this set
> with the flag BPF_PROG_TYPE_CGROUP_SOCKOPT to let it accessible to
> the 'cgroup/getsockopt' type of BPF programs.
> '''
>
> v2:
> - Drop bpf_skc_to_mptcp_sock
> - Check the owner before assigning the msk as Mat suggested.
> - Use bpf_core_cast() in mptcp_subflow bpf_iter subtest instead of
> using bpf_skc_to_mptcp_sock().
>
> Address Martin's suggestions for "Add mptcp_subflow bpf_iter support" v2.
>
> Geliang Tang (5):
>  Revert "bpf: Extend bpf_skc_to_mptcp_sock to MPTCP sock"
>  Revert "bpf: Allow use of skc_to_mptcp_sock in cg_sockopt"
>  Squash to "bpf: Add mptcp_subflow bpf_iter"
>  Revert "bpf: Acquire and release mptcp socket"
>  Squash to "selftests/bpf: Add mptcp_subflow bpf_iter subtest"
>
> include/net/mptcp.h                           |  4 +-
> kernel/bpf/cgroup.c                           |  2 -
> net/core/filter.c                             |  2 +-
> net/mptcp/bpf.c                               | 41 ++++---------------
> .../testing/selftests/bpf/bpf_experimental.h  |  2 +-
> tools/testing/selftests/bpf/progs/mptcp_bpf.h |  5 ---
> .../selftests/bpf/progs/mptcp_bpf_iters.c     | 10 ++---
> 7 files changed, 15 insertions(+), 51 deletions(-)
>
> -- 
> 2.43.0
>
>
>