Re: [PATCH mptcp-next v8 03/12] mptcp: sysctl: map pm_type to path_manager

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 04/03/2025 12:40, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new proc_handler "proc_pm_type" for "pm_type" to
> map old path manager sysctl "pm_type" to the newly added "path_manager".
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/ctrl.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> index d64e6b4f6d1d..d425fcbd036a 100644
> --- a/net/mptcp/ctrl.c
> +++ b/net/mptcp/ctrl.c
> @@ -217,6 +217,32 @@ static int proc_path_manager(const struct ctl_table *ctl, int write,
>  	return ret;
>  }
>  
> +static int proc_pm_type(const struct ctl_table *ctl, int write,
> +			void *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	struct mptcp_pernet *pernet = container_of(ctl->data,
> +						   struct mptcp_pernet,
> +						   pm_type);
> +	u8 pm_type = READ_ONCE(*(u8 *)ctl->data);
> +	const struct ctl_table tbl = {
> +		.maxlen = sizeof(pm_type),
> +		.data = &pm_type,
> +	};
> +	int ret;
> +
> +	ret = proc_dou8vec_minmax(&tbl, write, buffer, lenp, ppos);


I missed that in my previous review. Do you need tbl here? Can you not
use "ctl" here instead? If you need tbl, you will need to move the
"extra[12]" fields there I suppose, otherwise the limits will not be
checked, no?

In the proc_handler, a new ctl_table structure is needed when it is
required to have a way to prevent the writing of the data after having
called proc_do(...). Here, that's not the case: if the new value is
between 0 and mptcp_pm_type_max, we will always write ctl->data.

In other words, I think you can remove tlb, use ctl instead, and remove
the 'WRITE_ONCE(*(u8 *)ctl->data, pm_type)' here below.

> +	if (write && ret == 0) {
> +		char *path_manager = "kernel";
> +
> +		if (pm_type == MPTCP_PM_TYPE_USERSPACE)

pm_type should be read here in the 'if' statement, after having
potentially be modified in proc_dou8vec_minmax(). Or use "buffer" directly.

> +			path_manager = "userspace";
> +		mptcp_set_path_manager(pernet->path_manager, path_manager);
> +		WRITE_ONCE(*(u8 *)ctl->data, pm_type);
> +	}
> +
> +	return ret;
> +}
> +
>  static struct ctl_table mptcp_sysctl_table[] = {
>  	{
>  		.procname = "enabled",
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.