Re: [PATCH mptcp-next v8 08/12] mptcp: pm: initialize and release mptcp_pm_ops

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 04/03/2025 12:40, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Add a struct mptcp_pm_ops pointer "ops" in struct mptcp_pm_data, and two
> functions mptcp_pm_initialize() and mptcp_pm_release(), to set and release
> this pointer. mptcp_pm_initialize() is invoked in mptcp_pm_data_reset(),
> while mptcp_pm_release() is invoked in mptcp_pm_destroy().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c       | 57 +++++++++++++++++++++++++++-----------------
>  net/mptcp/protocol.h |  1 +
>  2 files changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 5018ed3c575f..e4d84aad3795 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -970,16 +970,45 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  	spin_unlock_bh(&msk->pm.lock);
>  }
>  
> +static void mptcp_pm_initialize(struct mptcp_sock *msk,
> +				struct mptcp_pm_ops *pm)

To avoid confusions, probably best to use "pm_ops" for "mptcp_pm_ops"
structures, and keep "pm" for "mptcp_pm_data" ones.

> +{
> +	if (!pm || !bpf_try_module_get(pm, pm->owner)) {
> +		pr_warn_once("pm %s fails, fallback to default pm",
> +			     pm->name);
> +		pm = &mptcp_pm_kernel;
> +	}
> +
> +	msk->pm.ops = pm;
> +	if (msk->pm.ops->init)

Note: this should be kept like that if 'init' is no longer mandatory
(see my comment on patch 7 -- but if it was mandatory because of what is
done in mptcp_pm_validate(), no need to check if it is set then)

> +		msk->pm.ops->init(msk);
> +
> +	pr_debug("pm %s initialized\n", pm->name);
> +}
> +
> +static void mptcp_pm_release(struct mptcp_sock *msk)
> +{
> +	struct mptcp_pm_ops *pm = msk->pm.ops;

Same here: pm_ops?

> +
> +	if (!pm)
When can we have this case? If we can have this case, that means each
time we will want to use "pm->ops", we will need to check if it is not
NULL. That feels wrong. The PM should be released only once.

We could add a WARN_ON_ONCE() here, but still, that feels wrong: why
adding a check only there? And if it is not needed, why adding it?

> +		return;
> +
> +	msk->pm.ops = NULL;
> +	if (pm->release)
> +		pm->release(msk);
> +
> +	bpf_module_put(pm, pm->owner);
Probably good to add this after:

  pr_debug("pm %s released\n", pm->name);

(...)

> @@ -991,25 +1020,9 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
>  	pm->rm_list_rx.nr = 0;
>  	WRITE_ONCE(pm->pm_type, pm_type);
>  
> -	if (pm_type == MPTCP_PM_TYPE_KERNEL) {
> -		bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
> -
> -		/* pm->work_pending must be only be set to 'true' when
> -		 * pm->pm_type is set to MPTCP_PM_TYPE_KERNEL
> -		 */
> -		WRITE_ONCE(pm->work_pending,
> -			   (!!mptcp_pm_get_local_addr_max(msk) &&
> -			    subflows_allowed) ||
> -			   !!mptcp_pm_get_add_addr_signal_max(msk));
> -		WRITE_ONCE(pm->accept_addr,
> -			   !!mptcp_pm_get_add_addr_accept_max(msk) &&
> -			   subflows_allowed);
> -		WRITE_ONCE(pm->accept_subflow, subflows_allowed);
> -	} else {
> -		WRITE_ONCE(pm->work_pending, 0);
> -		WRITE_ONCE(pm->accept_addr, 0);
> -		WRITE_ONCE(pm->accept_subflow, 0);
This should be kept here, for all PM, then calling pm->ops->init(), see
my comment in patch 7.


Also, not directly related to this patch, it feels like we should add a
"struct_group(reset, ...)" in struct mptcp_pm_data to simplify the
reset, and make sure we don't miss any. This should be done in a
dedicated patch, and can be done later.
Note that mptcp_pm_data_reset() is called when the msk is created, and
after a disconnect, to be able to re-use the socket again after. We
should make sure everything has been reset, that's not something we
heavily test. In fact, maybe we already missed the reset of some fields
that could cause some issues? Can you check that please?

> -	}
> +	rcu_read_lock();
> +	mptcp_pm_initialize(msk, mptcp_pm_find(path_manager));
> +	rcu_read_unlock();
>  
>  	WRITE_ONCE(pm->addr_signal, 0);
>  	WRITE_ONCE(pm->remote_deny_join_id0, false);
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.