| 1 | From: Geliang Tang <tanggeliang@kylinos.cn> | 1 | From: Geliang Tang <tanggeliang@kylinos.cn> |
|---|---|---|---|
| 2 | 2 | ||
| 3 | Address Martin's suggestions for v2. | 3 | v2: |
| 4 | - add CONFIG_LOCKDEP check in patch 2 to fix the build error reported | ||
| 5 | by CI. | ||
| 6 | |||
| 7 | Address Martin's comments in v1. | ||
| 4 | 8 | ||
| 5 | Geliang Tang (3): | 9 | Geliang Tang (3): |
| 10 | Squash to "bpf: Register mptcp common kfunc set" | ||
| 6 | Squash to "bpf: Add mptcp_subflow bpf_iter" | 11 | Squash to "bpf: Add mptcp_subflow bpf_iter" |
| 7 | Revert "bpf: Acquire and release mptcp socket" | ||
| 8 | Squash to "selftests/bpf: Add mptcp_subflow bpf_iter subtest" | 12 | Squash to "selftests/bpf: Add mptcp_subflow bpf_iter subtest" |
| 9 | 13 | ||
| 10 | net/mptcp/bpf.c | 34 +++++-------------- | 14 | net/mptcp/bpf.c | 22 ++++++++++++++----- |
| 11 | .../testing/selftests/bpf/bpf_experimental.h | 2 +- | 15 | .../selftests/bpf/progs/mptcp_bpf_iters.c | 7 +++--- |
| 12 | tools/testing/selftests/bpf/progs/mptcp_bpf.h | 3 -- | 16 | 2 files changed, 20 insertions(+), 9 deletions(-) |
| 13 | .../selftests/bpf/progs/mptcp_bpf_iters.c | 6 +--- | ||
| 14 | 4 files changed, 11 insertions(+), 34 deletions(-) | ||
| 15 | 17 | ||
| 16 | -- | 18 | -- |
| 17 | 2.43.0 | 19 | 2.45.2 | diff view generated by jsdifflib |
| 1 | From: Geliang Tang <tanggeliang@kylinos.cn> | 1 | From: Geliang Tang <tanggeliang@kylinos.cn> |
|---|---|---|---|
| 2 | 2 | ||
| 3 | Drop this patch as Martin suggested. | 3 | Address Martin's comments in v1: |
| 4 | |||
| 5 | - check IPPROTO_MPTCP in bpf_mptcp_sk. | ||
| 6 | - add null-checks for the wrappers. | ||
| 7 | - add more BPF flags for the wrappers. | ||
| 8 | - register this kfunc set to BPF_PROG_TYPE_CGROUP_SOCKOPT only, | ||
| 9 | not BPF_PROG_TYPE_UNSPEC. | ||
| 4 | 10 | ||
| 5 | Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> | 11 | Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> |
| 6 | --- | 12 | --- |
| 7 | net/mptcp/bpf.c | 19 ------------------- | 13 | net/mptcp/bpf.c | 17 +++++++++++++---- |
| 8 | 1 file changed, 19 deletions(-) | 14 | 1 file changed, 13 insertions(+), 4 deletions(-) |
| 9 | 15 | ||
| 10 | diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c | 16 | diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c |
| 11 | index XXXXXXX..XXXXXXX 100644 | 17 | index XXXXXXX..XXXXXXX 100644 |
| 12 | --- a/net/mptcp/bpf.c | 18 | --- a/net/mptcp/bpf.c |
| 13 | +++ b/net/mptcp/bpf.c | 19 | +++ b/net/mptcp/bpf.c |
| 14 | @@ -XXX,XX +XXX,XX @@ bpf_iter_mptcp_subflow_destroy(struct bpf_iter_mptcp_subflow *it) | 20 | @@ -XXX,XX +XXX,XX @@ __bpf_kfunc_start_defs(); |
| 21 | |||
| 22 | __bpf_kfunc static struct mptcp_sock *bpf_mptcp_sk(struct sock *sk) | ||
| 15 | { | 23 | { |
| 24 | + if (!sk || sk->sk_protocol != IPPROTO_MPTCP) | ||
| 25 | + return NULL; | ||
| 26 | + | ||
| 27 | return mptcp_sk(sk); | ||
| 16 | } | 28 | } |
| 17 | 29 | ||
| 18 | -__bpf_kfunc static struct | 30 | __bpf_kfunc static struct mptcp_subflow_context * |
| 19 | -mptcp_sock *bpf_mptcp_sock_acquire(struct mptcp_sock *msk) | 31 | bpf_mptcp_subflow_ctx(const struct sock *sk) |
| 20 | -{ | ||
| 21 | - struct sock *sk = (struct sock *)msk; | ||
| 22 | - | ||
| 23 | - if (sk && refcount_inc_not_zero(&sk->sk_refcnt)) | ||
| 24 | - return msk; | ||
| 25 | - return NULL; | ||
| 26 | -} | ||
| 27 | - | ||
| 28 | -__bpf_kfunc static void bpf_mptcp_sock_release(struct mptcp_sock *msk) | ||
| 29 | -{ | ||
| 30 | - struct sock *sk = (struct sock *)msk; | ||
| 31 | - | ||
| 32 | - WARN_ON_ONCE(!sk || !refcount_dec_not_one(&sk->sk_refcnt)); | ||
| 33 | -} | ||
| 34 | - | ||
| 35 | __bpf_kfunc struct mptcp_subflow_context * | ||
| 36 | bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos) | ||
| 37 | { | 32 | { |
| 38 | @@ -XXX,XX +XXX,XX @@ BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx, KF_RET_NULL) | 33 | + if (!sk) |
| 34 | + return NULL; | ||
| 35 | + | ||
| 36 | return mptcp_subflow_ctx(sk); | ||
| 37 | } | ||
| 38 | |||
| 39 | __bpf_kfunc static struct sock * | ||
| 40 | bpf_mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow) | ||
| 41 | { | ||
| 42 | + if (!subflow) | ||
| 43 | + return NULL; | ||
| 44 | + | ||
| 45 | return mptcp_subflow_tcp_sock(subflow); | ||
| 46 | } | ||
| 47 | |||
| 48 | @@ -XXX,XX +XXX,XX @@ __bpf_kfunc static bool bpf_mptcp_subflow_queues_empty(struct sock *sk) | ||
| 49 | __bpf_kfunc_end_defs(); | ||
| 50 | |||
| 51 | BTF_KFUNCS_START(bpf_mptcp_common_kfunc_ids) | ||
| 52 | -BTF_ID_FLAGS(func, bpf_mptcp_sk) | ||
| 53 | -BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx) | ||
| 54 | -BTF_ID_FLAGS(func, bpf_mptcp_subflow_tcp_sock) | ||
| 55 | +BTF_ID_FLAGS(func, bpf_mptcp_sk, KF_TRUSTED_ARGS | KF_RET_NULL) | ||
| 56 | +BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx, KF_RET_NULL) | ||
| 57 | +BTF_ID_FLAGS(func, bpf_mptcp_subflow_tcp_sock, KF_RET_NULL) | ||
| 39 | BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_new, KF_ITER_NEW | KF_TRUSTED_ARGS) | 58 | BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_new, KF_ITER_NEW | KF_TRUSTED_ARGS) |
| 40 | BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_next, KF_ITER_NEXT | KF_RET_NULL) | 59 | BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_next, KF_ITER_NEXT | KF_RET_NULL) |
| 41 | BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_destroy, KF_ITER_DESTROY) | 60 | BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_destroy, KF_ITER_DESTROY) |
| 42 | -BTF_ID_FLAGS(func, bpf_mptcp_sock_acquire, KF_ACQUIRE | KF_RET_NULL) | 61 | @@ -XXX,XX +XXX,XX @@ static int __init bpf_mptcp_kfunc_init(void) |
| 43 | -BTF_ID_FLAGS(func, bpf_mptcp_sock_release, KF_RELEASE) | 62 | int ret; |
| 44 | BTF_KFUNCS_END(bpf_mptcp_common_kfunc_ids) | 63 | |
| 45 | 64 | ret = register_btf_fmodret_id_set(&bpf_mptcp_fmodret_set); | |
| 46 | static const struct btf_kfunc_id_set bpf_mptcp_common_kfunc_set = { | 65 | - ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_UNSPEC, |
| 66 | + ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCKOPT, | ||
| 67 | &bpf_mptcp_common_kfunc_set); | ||
| 68 | ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, | ||
| 69 | &bpf_mptcp_sched_kfunc_set); | ||
| 47 | -- | 70 | -- |
| 48 | 2.43.0 | 71 | 2.45.2 | diff view generated by jsdifflib |
| 1 | From: Geliang Tang <tanggeliang@kylinos.cn> | 1 | From: Geliang Tang <tanggeliang@kylinos.cn> |
|---|---|---|---|
| 2 | 2 | ||
| 3 | Drop the NULL check as Martin suggested. | 3 | Address Martin's comments in v1: |
| 4 | 4 | ||
| 5 | Use the "struct sock *sk" instead of "struct mptcp-sock *msk" as the | 5 | - bpf_iter_mptcp_subflow_new returns -EINVAL when msk socket lock isn't |
| 6 | argument in the bpf_iter_mptcp_subflow_new as Martin suggested. | 6 | held. |
| 7 | |||
| 8 | Use msk_owned_by_me(). | ||
| 9 | 7 | ||
| 10 | Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> | 8 | Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> |
| 11 | --- | 9 | --- |
| 12 | net/mptcp/bpf.c | 15 +++++++++------ | 10 | net/mptcp/bpf.c | 5 ++++- |
| 13 | 1 file changed, 9 insertions(+), 6 deletions(-) | 11 | 1 file changed, 4 insertions(+), 1 deletion(-) |
| 14 | 12 | ||
| 15 | diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c | 13 | diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c |
| 16 | index XXXXXXX..XXXXXXX 100644 | 14 | index XXXXXXX..XXXXXXX 100644 |
| 17 | --- a/net/mptcp/bpf.c | 15 | --- a/net/mptcp/bpf.c |
| 18 | +++ b/net/mptcp/bpf.c | 16 | +++ b/net/mptcp/bpf.c |
| 19 | @@ -XXX,XX +XXX,XX @@ bpf_mptcp_subflow_ctx(const struct sock *sk) | 17 | @@ -XXX,XX +XXX,XX @@ bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it, |
| 20 | 18 | if (!msk) | |
| 21 | __bpf_kfunc static int | ||
| 22 | bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it, | ||
| 23 | - struct mptcp_sock *msk) | ||
| 24 | + struct sock *sk) | ||
| 25 | { | ||
| 26 | struct bpf_iter_mptcp_subflow_kern *kit = (void *)it; | ||
| 27 | - struct sock *sk = (struct sock *)msk; | ||
| 28 | + struct mptcp_sock *msk; | ||
| 29 | |||
| 30 | BUILD_BUG_ON(sizeof(struct bpf_iter_mptcp_subflow_kern) > | ||
| 31 | sizeof(struct bpf_iter_mptcp_subflow)); | ||
| 32 | BUILD_BUG_ON(__alignof__(struct bpf_iter_mptcp_subflow_kern) != | ||
| 33 | __alignof__(struct bpf_iter_mptcp_subflow)); | ||
| 34 | |||
| 35 | - kit->msk = msk; | ||
| 36 | - if (!msk) | ||
| 37 | + if (unlikely(!sk || !sk_fullsock(sk))) | ||
| 38 | return -EINVAL; | 19 | return -EINVAL; |
| 39 | 20 | ||
| 40 | - if (!sock_owned_by_user_nocheck(sk) && | 21 | - msk_owned_by_me(msk); |
| 41 | - !spin_is_locked(&sk->sk_lock.slock)) | 22 | +#ifdef CONFIG_LOCKDEP |
| 42 | + if (sk->sk_protocol != IPPROTO_MPTCP) | 23 | + if (!lockdep_sock_is_held((const struct sock *)msk)) |
| 43 | return -EINVAL; | 24 | + return -EINVAL; |
| 44 | 25 | +#endif | |
| 45 | + msk = mptcp_sk(sk); | 26 | |
| 46 | + kit->msk = msk; | ||
| 47 | + | ||
| 48 | + msk_owned_by_me(msk); | ||
| 49 | + | ||
| 50 | kit->pos = &msk->conn_list; | 27 | kit->pos = &msk->conn_list; |
| 51 | return 0; | 28 | return 0; |
| 52 | } | ||
| 53 | -- | 29 | -- |
| 54 | 2.43.0 | 30 | 2.45.2 | diff view generated by jsdifflib |
| 1 | From: Geliang Tang <tanggeliang@kylinos.cn> | 1 | From: Geliang Tang <tanggeliang@kylinos.cn> |
|---|---|---|---|
| 2 | 2 | ||
| 3 | Drop bpf_mptcp_sock_acquire/release. | 3 | IPPROTO_MPTCP is checked in bpf_mptcp_sk(), no need to check it in BPF |
| 4 | program. | ||
| 5 | |||
| 6 | bpf_mptcp_sk() and bpf_mptcp_subflow_ctx() may return NULL, need to | ||
| 7 | check the return values. | ||
| 4 | 8 | ||
| 5 | Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> | 9 | Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> |
| 6 | --- | 10 | --- |
| 7 | tools/testing/selftests/bpf/bpf_experimental.h | 2 +- | 11 | tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c | 7 +++---- |
| 8 | tools/testing/selftests/bpf/progs/mptcp_bpf.h | 3 --- | 12 | 1 file changed, 3 insertions(+), 4 deletions(-) |
| 9 | tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c | 6 +----- | ||
| 10 | 3 files changed, 2 insertions(+), 9 deletions(-) | ||
| 11 | 13 | ||
| 12 | diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h | ||
| 13 | index XXXXXXX..XXXXXXX 100644 | ||
| 14 | --- a/tools/testing/selftests/bpf/bpf_experimental.h | ||
| 15 | +++ b/tools/testing/selftests/bpf/bpf_experimental.h | ||
| 16 | @@ -XXX,XX +XXX,XX @@ extern void bpf_iter_css_destroy(struct bpf_iter_css *it) __weak __ksym; | ||
| 17 | |||
| 18 | struct bpf_iter_mptcp_subflow; | ||
| 19 | extern int bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it, | ||
| 20 | - struct mptcp_sock *msk) __weak __ksym; | ||
| 21 | + struct sock *sk) __weak __ksym; | ||
| 22 | extern struct mptcp_subflow_context * | ||
| 23 | bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it) __weak __ksym; | ||
| 24 | extern void | ||
| 25 | diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/testing/selftests/bpf/progs/mptcp_bpf.h | ||
| 26 | index XXXXXXX..XXXXXXX 100644 | ||
| 27 | --- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h | ||
| 28 | +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h | ||
| 29 | @@ -XXX,XX +XXX,XX @@ mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow) | ||
| 30 | } | ||
| 31 | |||
| 32 | /* ksym */ | ||
| 33 | -extern struct mptcp_sock *bpf_mptcp_sock_acquire(struct mptcp_sock *msk) __ksym; | ||
| 34 | -extern void bpf_mptcp_sock_release(struct mptcp_sock *msk) __ksym; | ||
| 35 | - | ||
| 36 | extern struct mptcp_subflow_context * | ||
| 37 | bpf_mptcp_subflow_ctx(const struct sock *sk) __ksym; | ||
| 38 | extern struct sock * | ||
| 39 | diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c | 14 | diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c |
| 40 | index XXXXXXX..XXXXXXX 100644 | 15 | index XXXXXXX..XXXXXXX 100644 |
| 41 | --- a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c | 16 | --- a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c |
| 42 | +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c | 17 | +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c |
| 43 | @@ -XXX,XX +XXX,XX @@ int iters_subflow(struct bpf_sockopt *ctx) | 18 | @@ -XXX,XX +XXX,XX @@ int iters_subflow(struct bpf_sockopt *ctx) |
| 44 | if (!msk || msk->pm.server_side || !msk->pm.subflows) | 19 | struct mptcp_sock *msk; |
| 20 | int local_ids = 0; | ||
| 21 | |||
| 22 | - if (!sk || sk->protocol != IPPROTO_MPTCP || | ||
| 23 | - ctx->level != SOL_TCP || ctx->optname != TCP_IS_MPTCP) | ||
| 24 | + if (ctx->level != SOL_TCP || ctx->optname != TCP_IS_MPTCP) | ||
| 45 | return 1; | 25 | return 1; |
| 46 | 26 | ||
| 47 | - msk = bpf_mptcp_sock_acquire(msk); | 27 | msk = bpf_mptcp_sk((struct sock *)sk); |
| 48 | - if (!msk) | 28 | - if (msk->pm.server_side || !msk->pm.subflows) |
| 49 | - return 1; | 29 | + if (!msk || msk->pm.server_side || !msk->pm.subflows) |
| 50 | - bpf_for_each(mptcp_subflow, subflow, msk) { | 30 | return 1; |
| 51 | + bpf_for_each(mptcp_subflow, subflow, (struct sock *)sk) { | 31 | |
| 52 | /* Here MPTCP-specific packet scheduler kfunc can be called: | 32 | msk = bpf_mptcp_sock_acquire(msk); |
| 53 | * this test is not doing anything really useful, only to | ||
| 54 | * verify the iteration works. | ||
| 55 | @@ -XXX,XX +XXX,XX @@ int iters_subflow(struct bpf_sockopt *ctx) | 33 | @@ -XXX,XX +XXX,XX @@ int iters_subflow(struct bpf_sockopt *ctx) |
| 34 | |||
| 35 | /* only to check the following kfunc works */ | ||
| 36 | subflow = bpf_mptcp_subflow_ctx(ssk); | ||
| 37 | - if (subflow->token != msk->token) | ||
| 38 | + if (!subflow || subflow->token != msk->token) | ||
| 39 | goto out; | ||
| 40 | |||
| 56 | ids = local_ids; | 41 | ids = local_ids; |
| 57 | |||
| 58 | out: | ||
| 59 | - bpf_mptcp_sock_release(msk); | ||
| 60 | return 1; | ||
| 61 | } | ||
| 62 | -- | 42 | -- |
| 63 | 2.43.0 | 43 | 2.45.2 | diff view generated by jsdifflib |