On Wed, 1 Jun 2022, Geliang Tang wrote:
> This patch adds a new helper mptcp_subflow_set_scheduled() to set the
> scheduled flag of struct mptcp_subflow_context using WRITE_ONCE().
> Register this helper in bpf_mptcp_sched_kfunc_init() to make sure it can
> be accessed from the BPF context.
>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/bpf.c | 16 ++++++++++++++++
> net/mptcp/protocol.h | 2 ++
> net/mptcp/sched.c | 7 +++++++
> tools/testing/selftests/bpf/bpf_tcp_helpers.h | 3 +++
Geliang -
This commit changes both bpf_tcp_helpers.h and MPTCP core code. There are
different upstreaming requirements for bpf_tcp_helpers.h since the BPF
maintainers own that file.
Could you split the changes to bpf_tcp_helpers.h in this commit, and in
the others already in the export branch, in to a separate "MPTCP BPF
helper" commit before "selftests/bpf: add bpf_first scheduler"? I think
that will make it easier to upstream commits that don't need to go through
the BPF maintainers.
> 4 files changed, 28 insertions(+)
>
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index 0529e70d53b1..e86dff4272d5 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -161,6 +161,22 @@ struct bpf_struct_ops bpf_mptcp_sched_ops = {
> .init = bpf_mptcp_sched_init,
> .name = "mptcp_sched_ops",
> };
> +
> +BTF_SET_START(bpf_mptcp_sched_kfunc_ids)
> +BTF_ID(func, mptcp_subflow_set_scheduled)
> +BTF_SET_END(bpf_mptcp_sched_kfunc_ids)
> +
> +static const struct btf_kfunc_id_set bpf_mptcp_sched_kfunc_set = {
> + .owner = THIS_MODULE,
> + .check_set = &bpf_mptcp_sched_kfunc_ids,
> +};
> +
> +static int __init bpf_mptcp_sched_kfunc_init(void)
> +{
> + return register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS,
> + &bpf_mptcp_sched_kfunc_set);
> +}
> +late_initcall(bpf_mptcp_sched_kfunc_init);
> #endif /* CONFIG_BPF_JIT */
>
> struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk)
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 48c5261b7b15..d406b5afbee4 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -629,6 +629,8 @@ void mptcp_unregister_scheduler(struct mptcp_sched_ops *sched);
> int mptcp_init_sched(struct mptcp_sock *msk,
> struct mptcp_sched_ops *sched);
> void mptcp_release_sched(struct mptcp_sock *msk);
> +void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
> + bool scheduled);
> struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk);
> struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk);
> struct sock *mptcp_sched_get_send(struct mptcp_sock *msk);
> diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
> index 6815654fc6f4..8858e1fc8b74 100644
> --- a/net/mptcp/sched.c
> +++ b/net/mptcp/sched.c
> @@ -88,6 +88,12 @@ void mptcp_release_sched(struct mptcp_sock *msk)
> bpf_module_put(sched, sched->owner);
> }
>
> +void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
> + bool scheduled)
> +{
> + WRITE_ONCE(subflow->scheduled, scheduled);
> +}
As far as I can tell, it is safe to call a function like this from BPF
code. I modified the bpf_first test to try to pass a bad subflow pointer
in a few different ways, and the BPF verifier caught it.
I'm curious, have you found any documentation for how the verifier checks
args for kfunc calls? I'd like to understand that better and will keep
searching.
Thanks,
Mat
> +
> static int mptcp_sched_data_init(struct mptcp_sock *msk, bool reinject,
> struct mptcp_sched_data *data)
> {
> @@ -101,6 +107,7 @@ static int mptcp_sched_data_init(struct mptcp_sock *msk, bool reinject,
> pr_warn_once("too many subflows");
> break;
> }
> + mptcp_subflow_set_scheduled(subflow, false);
> data->contexts[i++] = subflow;
> }
>
> diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> index 5f6a7fa2269b..5e301e830a0a 100644
> --- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> @@ -261,4 +261,7 @@ struct mptcp_sock {
> char ca_name[TCP_CA_NAME_MAX];
> } __attribute__((preserve_access_index));
>
> +extern void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
> + bool scheduled) __ksym;
> +
> #endif
> --
> 2.34.1
>
>
>
--
Mat Martineau
Intel