| Message ID | cover.1740368110.git.tanggeliang@kylinos.cn (mailing list archive) |
|---|---|
| Headers | show |
| Series | Squash to "Add mptcp_subflow bpf_iter support" | expand |
On Mon, 2025-02-24 at 11:37 +0800, 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. 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. ''' Thanks, -Geliang > > 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(-) >
Hi Geliang,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/13490473539
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/9fc290ccc06f
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=936873
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
Hi Geliang, Mat, On 24/02/2025 04:37, 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. The v4 looks OK to me, but I prefer to wait for Mat's ACK, if that's OK, because he reviewed the original series. Cheers, Matt
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 > > >
Hi Geliang, Mat, On 24/02/2025 04:37, 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. Thank you for the patches and the reviews! Now in our tree: New patches for t/upstream: - fe0c24467a00: "squashed" patch 1/5 in "bpf: Extend bpf_skc_to_mptcp_sock to MPTCP sock" - 83bb4052ab8a: "squashed" patch 2/5 in "bpf: Allow use of skc_to_mptcp_sock in cg_sockopt" - dd8797c750fe: "squashed" patch 3/5 in "bpf: Add mptcp_subflow bpf_iter" - a5e732a840e2: "squashed" my patch in "bpf: Add mptcp_subflow bpf_iter" - 23094de26cb5: "squashed" (with conflicts) patch 4/5 in "bpf: Acquire and release mptcp socket" - 5a1d91403efd: "squashed" patch 5/5 in "selftests/bpf: Add mptcp_subflow bpf_iter subtest" - 29cc864a2cc6: tg:msg: update after the recent squash-to patch - da6054a670da: conflict in t/mptcp-add-sched_data-helpers-2 - 401780491d45: conflict in t/mptcp-add-bpf_mptcp_sched_ops - Results: e1315f2b7271..bf7e42dbc2fa (export) Tests are now in progress: - export: https://github.com/multipath-tcp/mptcp_net-next/commit/0447c3073f646734cffeec271b1a6b03a53103ef/checks Cheers, Matt
Hi Matt, On Mon, 2025-02-24 at 11:37 +0800, 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. > > 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. > ''' Thanks for applying this set. Please update this commit log too. -Geliang > > 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(-) >
Hi Geliang, On 06/03/2025 02:50, Geliang Tang wrote: > Hi Matt, > > On Mon, 2025-02-24 at 11:37 +0800, 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. >> >> 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. >> ''' > > Thanks for applying this set. Please update this commit log too. Done. The modification will be visible soon: New patches for t/upstream: - 061566ebfca8: tg:msg: adapt 'bpf: Register mptcp common kfunc set' - Results: 6c4b1c674680..ccf37aa90567 (export) Cheers, Matt
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. 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(-)