Re: [PATCH mptcp-next v4 5/9] mptcp: pm: add add_addr_received() interface

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 24/03/2025 09:19, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds an optional .add_addr_received interface for struct
> mptcp_pm_ops and invokes it in mptcp_pm_worker().
> 
> This interface is only implemented in the in-kernel PM as a wrapper
> of mptcp_pm_nl_add_addr_received().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  include/net/mptcp.h   |  1 +
>  net/mptcp/pm.c        | 18 +++++++++++-------
>  net/mptcp/pm_kernel.c | 24 +++++++++++-------------
>  net/mptcp/protocol.h  |  1 -
>  4 files changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 4ac936e4ce0d..5118d11d2ee9 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -127,6 +127,7 @@ struct mptcp_pm_ops {
>  	/* optional */
>  	void (*established)(struct mptcp_sock *msk);
>  	void (*subflow_established)(struct mptcp_sock *msk);
> +	void (*add_addr_received)(struct mptcp_sock *msk);
>  
>  	char			name[MPTCP_PM_NAME_MAX];
>  	struct module		*owner;
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 8efb47331f79..71589cd5dee7 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -607,10 +607,11 @@ void mptcp_pm_add_addr_received(const struct sock *ssk,
>  		   (addr->id > 0 && !READ_ONCE(pm->accept_addr))) {

How do you plan to remove the 'if (mptcp_pm_is_userspace(msk))' here above.

I guess this code is there to force the other peer to retransmit the
ADD_ADDR, hoping a userspace will be launched in between. Either we
remove this exception for the userspace PM (other events will not be
retransmitted: RM_ADDR, subflow closed, etc.), or we have another hook
but it feels wrong.

EDIT: I just saw your patch 7/9. Maybe we should avoid adding this
add_addr_echo hook, no? It is not clear what should be done here. I need
to think about that too.

An alternative is to send the ADD_ADDR echo from the worker, if
pm->ops->add_addr_received() returned true. If
pm->ops->add_addr_received is not implemented, then the ADD_ADDR echo is
scheduled from here. WDYT?

>  		mptcp_pm_announce_addr(msk, addr, true);
>  		mptcp_pm_add_addr_send_ack(msk);
> -	} else if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED)) {
> -		pm->remote = *addr;
> -	} else {
> -		ret = -EINVAL;
> +	} else if (pm->ops->add_addr_received) {
> +		if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED))
> +			pm->remote = *addr;
> +		else
> +			ret = -EINVAL;
>  	}
>  
>  	if (ret)
> @@ -948,6 +949,12 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  		return;
>  
>  	pr_debug("msk=%p status=%x\n", msk, pm->status);
> +	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_RECEIVED)) {
> +		spin_lock_bh(&pm->lock);
> +		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED);
> +		spin_unlock_bh(&pm->lock);
> +		pm->ops->add_addr_received(msk);
> +	}
>  	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_SEND_ACK)) {
>  		spin_lock_bh(&pm->lock);
>  		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_SEND_ACK);
> @@ -972,9 +979,6 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  		spin_unlock_bh(&pm->lock);
>  		pm->ops->subflow_established(msk);
>  	}
> -	spin_lock_bh(&pm->lock);
> -	__mptcp_pm_kernel_worker(msk);
> -	spin_unlock_bh(&pm->lock);
>  }
>  
>  static void mptcp_pm_ops_init(struct mptcp_sock *msk,
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 2e181224bccb..4f4791620072 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -461,12 +461,13 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  		 msk->pm.add_addr_accepted, add_addr_accept_max,
>  		 msk->pm.remote.family);
>  
> +	spin_lock_bh(&msk->pm.lock);
>  	remote = msk->pm.remote;
>  	mptcp_pm_announce_addr(msk, &remote, true);
>  	mptcp_pm_addr_send_ack(msk);
>  
>  	if (lookup_subflow_by_daddr(&msk->conn_list, &remote))
> -		return;
> +		goto out;
>  
>  	/* pick id 0 port, if none is provided the remote address */
>  	if (!remote.port)
> @@ -477,7 +478,7 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  	 */
>  	nr = fill_local_addresses_vec(msk, &remote, locals);
>  	if (nr == 0)
> -		return;
> +		goto out;
>  
>  	spin_unlock_bh(&msk->pm.lock);
>  	for (i = 0; i < nr; i++)
> @@ -493,6 +494,8 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  		    msk->pm.subflows >= subflows_max)
>  			WRITE_ONCE(msk->pm.accept_addr, false);
>  	}
> +out:
> +	spin_unlock_bh(&msk->pm.lock);
>  }
>  
>  void mptcp_pm_nl_rm_addr(struct mptcp_sock *msk, u8 rm_id)
> @@ -1342,17 +1345,6 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
>  	return true;
>  }
>  
> -/* Called under PM lock */
> -void __mptcp_pm_kernel_worker(struct mptcp_sock *msk)
> -{
> -	struct mptcp_pm_data *pm = &msk->pm;
> -
> -	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_RECEIVED)) {
> -		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED);
> -		mptcp_pm_nl_add_addr_received(msk);
> -	}
> -}
> -
>  static int __net_init pm_nl_init_net(struct net *net)
>  {
>  	struct pm_nl_pernet *pernet = pm_nl_get_pernet(net);
> @@ -1394,6 +1386,11 @@ static struct pernet_operations mptcp_pm_pernet_ops = {
>  	.size = sizeof(struct pm_nl_pernet),
>  };
>  
> +static void mptcp_pm_kernel_add_addr_received(struct mptcp_sock *msk)
> +{
> +	mptcp_pm_nl_add_addr_received(msk);

No need to add a new static function only calling another static
function with the same arguments.

Simply rename mptcp_pm_nl_add_addr_received() to
mptcp_pm_kernel_add_addr_received().

> +}
> +
>  static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
>  {
>  	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
> @@ -1419,6 +1416,7 @@ struct mptcp_pm_ops mptcp_pm_kernel = {
>  	.get_priority		= mptcp_pm_kernel_get_priority,
>  	.established		= mptcp_pm_kernel_established,
>  	.subflow_established	= mptcp_pm_kernel_subflow_established,
> +	.add_addr_received	= mptcp_pm_kernel_add_addr_received,
>  	.init			= mptcp_pm_kernel_init,
>  	.name			= "kernel",
>  	.owner			= THIS_MODULE,
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 64aa091cb685..7fa26c49fbed 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1164,7 +1164,6 @@ void __init mptcp_pm_kernel_register(void);
>  void __init mptcp_pm_userspace_register(void);
>  void __init mptcp_pm_nl_init(void);
>  void mptcp_pm_worker(struct mptcp_sock *msk);
> -void __mptcp_pm_kernel_worker(struct mptcp_sock *msk);
>  unsigned int mptcp_pm_get_add_addr_signal_max(const struct mptcp_sock *msk);
>  unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk);
>  unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk);

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