[PATCH mptcp-next 0/2] mptcp: fix net.mptcp.scheduler behavior

[PATCH mptcp-next 0/2] mptcp: fix net.mptcp.scheduler behavior

[Gregory Detal]

The sysctl is accepting any input. This patchset ensures only
existing scheduler can be set.

Moreover, this adds an helper sysctl to list the available scheduler
similarly to tcp_available_congestion_control.

The first patch of the series is a fix for mptcp-net.

Signed-off-by: Gregory Detal <gregory.detal@gmail.com>
---
Gregory Detal (2):
      mptcp: only allow set existing scheduler for net.mptcp.scheduler
      mptcp: add net.mptcp.available_schedulers

 include/net/mptcp.h  |  3 +++
 net/mptcp/ctrl.c     | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 net/mptcp/protocol.h |  1 +
 net/mptcp/sched.c    | 22 +++++++++++++++++
 4 files changed, 91 insertions(+), 2 deletions(-)
---
base-commit: dd741144c1d1e3bd1fed5eb0ded1934c9230b4ef
change-id: 20240430-sysctl_scheduler-13367d5bf81a

Best regards,
-- 
Gregory Detal <gregory.detal@gmail.com>

[PATCH mptcp-next 1/2] mptcp: only allow set existing scheduler for net.mptcp.scheduler

[Gregory Detal]

The current behavior is to accept any strings as inputs, this results in
an inconsistent result where an unexisting scheduler can be set:

 # sysctl -w net.mptcp.scheduler=notdefault
 net.mptcp.scheduler = notdefault

This patch changes this behavior by checking for existing scheduler
before accepting the input.

Fixes: e3b2870b6d22 ("mptcp: add a new sysctl scheduler")
Signed-off-by: Gregory Detal <gregory.detal@gmail.com>
---
 net/mptcp/ctrl.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index 8d661156ab8c..0e69bd0ea302 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -96,6 +96,44 @@ static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet)
 }
 
 #ifdef CONFIG_SYSCTL
+
+static int mptcp_set_scheduler(const struct net *net, const char *name)
+{
+	struct mptcp_pernet *pernet = mptcp_get_pernet(net);
+	struct mptcp_sched_ops *sched;
+	int ret = 0;
+
+	rcu_read_lock();
+	sched = mptcp_sched_find(name);
+	if (sched)
+		strscpy(pernet->scheduler, name, MPTCP_SCHED_NAME_MAX);
+	else
+		ret = -ENOENT;
+	rcu_read_unlock();
+
+	return ret;
+}
+
+static int proc_scheduler(struct ctl_table *ctl, int write,
+			  void *buffer, size_t *lenp, loff_t *ppos)
+{
+	const struct net *net = current->nsproxy->net_ns;
+	char val[MPTCP_SCHED_NAME_MAX];
+	struct ctl_table tbl = {
+		.data = val,
+		.maxlen = MPTCP_SCHED_NAME_MAX,
+	};
+	int ret;
+
+	strscpy(val, mptcp_get_scheduler(net), MPTCP_SCHED_NAME_MAX);
+
+	ret = proc_dostring(&tbl, write, buffer, lenp, ppos);
+	if (write && ret == 0)
+		ret = mptcp_set_scheduler(net, val);
+
+	return ret;
+}
+
 static struct ctl_table mptcp_sysctl_table[] = {
 	{
 		.procname = "enabled",
@@ -148,7 +186,7 @@ static struct ctl_table mptcp_sysctl_table[] = {
 		.procname = "scheduler",
 		.maxlen	= MPTCP_SCHED_NAME_MAX,
 		.mode = 0644,
-		.proc_handler = proc_dostring,
+		.proc_handler = proc_scheduler,
 	},
 	{
 		.procname = "close_timeout",

-- 
2.43.0

Re: [PATCH mptcp-next 1/2] mptcp: only allow set existing scheduler for net.mptcp.scheduler

[Matthieu Baerts]

On 30/04/2024 14:06, Gregory Detal wrote:
> The current behavior is to accept any strings as inputs, this results in
> an inconsistent result where an unexisting scheduler can be set:
> 
>  # sysctl -w net.mptcp.scheduler=notdefault
>  net.mptcp.scheduler = notdefault
> 
> This patch changes this behavior by checking for existing scheduler
> before accepting the input.
> 
> Fixes: e3b2870b6d22 ("mptcp: add a new sysctl scheduler")
> Signed-off-by: Gregory Detal <gregory.detal@gmail.com>
> ---
>  net/mptcp/ctrl.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> index 8d661156ab8c..0e69bd0ea302 100644
> --- a/net/mptcp/ctrl.c
> +++ b/net/mptcp/ctrl.c
> @@ -96,6 +96,44 @@ static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet)
>  }
>  
>  #ifdef CONFIG_SYSCTL
> +

We can remove this extra new line here: I can do that when applying the
patches if I don't forget.

> +static int mptcp_set_scheduler(const struct net *net, const char *name)
(...)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

[PATCH mptcp-next 2/2] mptcp: add net.mptcp.available_schedulers

[Gregory Detal]

The sysctl lists the available schedulers that can be set using
net.mptcp.scheduler similarly to net.ipv4.tcp_available_congestion_control.

Signed-off-by: Gregory Detal <gregory.detal@gmail.com>
---
 include/net/mptcp.h  |  3 +++
 net/mptcp/ctrl.c     | 27 ++++++++++++++++++++++++++-
 net/mptcp/protocol.h |  1 +
 net/mptcp/sched.c    | 22 ++++++++++++++++++++++
 4 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index fb996124b3d5..0bc4ab03f487 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -97,6 +97,9 @@ struct mptcp_out_options {
 };
 
 #define MPTCP_SCHED_NAME_MAX	16
+#define MPTCP_SCHED_MAX		128
+#define MPTCP_SCHED_BUF_MAX	(MPTCP_SCHED_NAME_MAX * MPTCP_SCHED_MAX)
+
 #define MPTCP_SUBFLOWS_MAX	8
 
 struct mptcp_sched_data {
diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index 0e69bd0ea302..808df66ec53b 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -134,6 +134,24 @@ static int proc_scheduler(struct ctl_table *ctl, int write,
 	return ret;
 }
 
+static int proc_available_schedulers(struct ctl_table *ctl,
+				     int write, void *buffer,
+				     size_t *lenp, loff_t *ppos)
+{
+	struct ctl_table tbl = { .maxlen = MPTCP_SCHED_BUF_MAX, };
+	int ret;
+
+	tbl.data = kmalloc(tbl.maxlen, GFP_USER);
+	if (!tbl.data)
+		return -ENOMEM;
+
+	mptcp_get_available_schedulers(tbl.data, MPTCP_SCHED_BUF_MAX);
+	ret = proc_dostring(&tbl, write, buffer, lenp, ppos);
+	kfree(tbl.data);
+
+	return ret;
+}
+
 static struct ctl_table mptcp_sysctl_table[] = {
 	{
 		.procname = "enabled",
@@ -188,6 +206,12 @@ static struct ctl_table mptcp_sysctl_table[] = {
 		.mode = 0644,
 		.proc_handler = proc_scheduler,
 	},
+	{
+		.procname = "available_schedulers",
+		.maxlen	= MPTCP_SCHED_BUF_MAX,
+		.mode = 0644,
+		.proc_handler = proc_available_schedulers,
+	},
 	{
 		.procname = "close_timeout",
 		.maxlen = sizeof(unsigned int),
@@ -216,7 +240,8 @@ static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet *pernet)
 	table[4].data = &pernet->stale_loss_cnt;
 	table[5].data = &pernet->pm_type;
 	table[6].data = &pernet->scheduler;
-	table[7].data = &pernet->close_timeout;
+	/* table[7] is for available_schedulers which is read-only info */
+	table[8].data = &pernet->close_timeout;
 
 	hdr = register_net_sysctl_sz(net, MPTCP_SYSCTL_PATH, table,
 				     ARRAY_SIZE(mptcp_sysctl_table));
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index ffd00fb45433..8750254eb3bb 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -683,6 +683,7 @@ unsigned int mptcp_stale_loss_cnt(const struct net *net);
 unsigned int mptcp_close_timeout(const struct sock *sk);
 int mptcp_get_pm_type(const struct net *net);
 const char *mptcp_get_scheduler(const struct net *net);
+void mptcp_get_available_schedulers(char *buf, size_t maxlen);
 void __mptcp_subflow_fully_established(struct mptcp_sock *msk,
 				       struct mptcp_subflow_context *subflow,
 				       const struct mptcp_options_received *mp_opt);
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index a7e1c10b1984..8def10abd60e 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -51,6 +51,28 @@ struct mptcp_sched_ops *mptcp_sched_find(const char *name)
 	return ret;
 }
 
+/* Build string with list of available scheduler values.
+ * Similar to tcp_get_available_congestion_control()
+ */
+void mptcp_get_available_schedulers(char *buf, size_t maxlen)
+{
+	struct mptcp_sched_ops *sched;
+	size_t offs = 0;
+
+	rcu_read_lock();
+	spin_lock(&mptcp_sched_list_lock);
+	list_for_each_entry_rcu(sched, &mptcp_sched_list, list) {
+		offs += snprintf(buf + offs, maxlen - offs,
+				 "%s%s",
+				 offs == 0 ? "" : " ", sched->name);
+
+		if (WARN_ON_ONCE(offs >= maxlen))
+			break;
+	}
+	spin_unlock(&mptcp_sched_list_lock);
+	rcu_read_unlock();
+}
+
 int mptcp_register_scheduler(struct mptcp_sched_ops *sched)
 {
 	if (!sched->get_subflow)

-- 
2.43.0

Re: [PATCH mptcp-next 0/2] mptcp: fix net.mptcp.scheduler behavior

[MPTCP CI]

Hi Gregory,

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: Unstable: 1 failed test(s): selftest_mptcp_join 🔴
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8894625435

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/de7bcee71937
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=849264


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)

Re: [PATCH mptcp-next 0/2] mptcp: fix net.mptcp.scheduler behavior

[Matthieu Baerts]

Hi Gregory,

On 30/04/2024 14:59, MPTCP CI wrote:
> Hi Gregory,
> 
> 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: Unstable: 1 failed test(s): selftest_mptcp_join 🔴

FYI, the issue doesn't seem to be due to your modification.

This test is known to be unstable:

  https://github.com/multipath-tcp/mptcp_net-next/issues/324

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next 0/2] mptcp: fix net.mptcp.scheduler behavior

[Matthieu Baerts]

Hi Gregory,

On 30/04/2024 14:06, Gregory Detal wrote:
> The sysctl is accepting any input. This patchset ensures only
> existing scheduler can be set.
> 
> Moreover, this adds an helper sysctl to list the available scheduler
> similarly to tcp_available_congestion_control.
> 
> The first patch of the series is a fix for mptcp-net.

Thank you for this fix, it makes sense! And thanks for the new sysctl
entry, it will be useful!

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

@Mat / @Geliang: because I did a review "internally" with Gregory, do
not hesitate to have a quick look to have an external point of view :)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next 0/2] mptcp: fix net.mptcp.scheduler behavior

[Geliang Tang]

Hi Gregory,

On Tue, Apr 30, 2024 at 04:00:17PM +0200, Matthieu Baerts wrote:
> Hi Gregory,
> 
> On 30/04/2024 14:06, Gregory Detal wrote:
> > The sysctl is accepting any input. This patchset ensures only
> > existing scheduler can be set.
> > 
> > Moreover, this adds an helper sysctl to list the available scheduler
> > similarly to tcp_available_congestion_control.
> > 
> > The first patch of the series is a fix for mptcp-net.
> 
> Thank you for this fix, it makes sense! And thanks for the new sysctl
> entry, it will be useful!
> 
> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Thanks for these patches, all tests passed.

Tested-by: Geliang Tang <geliang@kernel.org>

-Geliang

> 
> @Mat / @Geliang: because I did a review "internally" with Gregory, do
> not hesitate to have a quick look to have an external point of view :)
> 
> Cheers,
> Matt
> -- 
> Sponsored by the NGI0 Core fund.
> 

Re: [PATCH mptcp-next 0/2] mptcp: fix net.mptcp.scheduler behavior

[Mat Martineau]

On Tue, 30 Apr 2024, Gregory Detal wrote:

> The sysctl is accepting any input. This patchset ensures only
> existing scheduler can be set.
>
> Moreover, this adds an helper sysctl to list the available scheduler
> similarly to tcp_available_congestion_control.
>
> The first patch of the series is a fix for mptcp-net.
>
> Signed-off-by: Gregory Detal <gregory.detal@gmail.com>

Series looks good to me as well (and Matthieu can remove that newline if 
he'd like). Thanks for the patches!

Reviewed-by: Mat Martineau <martineau@kernel.org>

> ---
> Gregory Detal (2):
>      mptcp: only allow set existing scheduler for net.mptcp.scheduler
>      mptcp: add net.mptcp.available_schedulers
>
> include/net/mptcp.h  |  3 +++
> net/mptcp/ctrl.c     | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> net/mptcp/protocol.h |  1 +
> net/mptcp/sched.c    | 22 +++++++++++++++++
> 4 files changed, 91 insertions(+), 2 deletions(-)
> ---
> base-commit: dd741144c1d1e3bd1fed5eb0ded1934c9230b4ef
> change-id: 20240430-sysctl_scheduler-13367d5bf81a
>
> Best regards,
> -- 
> Gregory Detal <gregory.detal@gmail.com>
>
>
>

Re: [PATCH mptcp-next 0/2] mptcp: fix net.mptcp.scheduler behavior

[Matthieu Baerts]

Hi Gregory, Geliang, Mat,

On 30/04/2024 14:06, Gregory Detal wrote:
> The sysctl is accepting any input. This patchset ensures only
> existing scheduler can be set.
> 
> Moreover, this adds an helper sysctl to list the available scheduler
> similarly to tcp_available_congestion_control.
> 
> The first patch of the series is a fix for mptcp-net.
Thank you for the patches, the tests, and the review:

New patches for t/upstream-net and t/upstream:
- bc3eac499491: mptcp: only allow set existing scheduler for
net.mptcp.scheduler
- Results: 9df9126ccb3d..193bf8fbc31b (export-net)
- Results: c1f52bb3cea5..e83515296801 (export)

New patches for t/upstream:
- 660afb67d62f: mptcp: add net.mptcp.available_schedulers
- Results: e83515296801..022e39266b3c (export)

Tests are now in progress:

- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/56030f9d3812071365435354c0eb5ffb3504e58a/checks
- export-net:
https://github.com/multipath-tcp/mptcp_net-next/commit/a6e18d675691b704ed8e61862efa61464ffbeeb7/checks

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.