Re: [PATCH mptcp-next v7 02/11] mptcp: userspace pm set_flags id support

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 06/01/2025 09:16, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Similar to in-kernel PM, this patch adds address ID support to set_flags()
> interface of userspace PM, allowing it to work with either an address or
> an address ID.
> 
> When an address ID is used, mptcp_userspace_pm_lookup_addr_by_id() helper
> is used to look up the address entry in the local address list instead of
> using mptcp_userspace_pm_lookup_addr().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm_userspace.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 40fd2f788196..60db72060e6e 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -577,6 +577,7 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
>  	struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
>  	struct mptcp_pm_addr_entry *entry;
>  	struct mptcp_sock *msk;
> +	u8 lookup_by_id = 0;
>  	int ret = -EINVAL;
>  	struct sock *sk;
>  	u8 bkup = 0;
> @@ -592,10 +593,13 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
>  		goto set_flags_err;
>  
>  	if (loc.addr.family == AF_UNSPEC) {
> -		NL_SET_ERR_MSG_ATTR(info->extack, attr,
> -				    "invalid local address family");
> -		ret = -EINVAL;
> -		goto set_flags_err;
> +		lookup_by_id = 1;
> +		if (!loc.addr.id) {
> +			NL_SET_ERR_MSG_ATTR(info->extack, attr,
> +					    "missing address ID");
> +			ret = -EOPNOTSUPP;
> +			goto set_flags_err;
> +		}
>  	}
>  
>  	if (attr_rem) {
> @@ -615,17 +619,22 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
>  		bkup = 1;
>  
>  	spin_lock_bh(&msk->pm.lock);
> -	entry = mptcp_userspace_pm_lookup_addr(msk, &loc.addr);
> -	if (entry) {
> -		if (bkup)
> -			entry->flags |= MPTCP_PM_ADDR_FLAG_BACKUP;
> -		else
> -			entry->flags &= ~MPTCP_PM_ADDR_FLAG_BACKUP;
> +	entry = lookup_by_id ? mptcp_userspace_pm_lookup_addr_by_id(msk, loc.addr.id) :
> +			       mptcp_userspace_pm_lookup_addr(msk, &loc.addr);
> +	if (!entry) {
> +		spin_unlock_bh(&msk->pm.lock);
> +		ret = -EINVAL;
> +		goto set_flags_err;

Mmh, you are changing the behaviour here by making it mandatory to have
an entry, and I don't think you can: if I'm not mistaken, with the
userspace PM, it is possible not to find any entries here, e.g. if a
subflow using this address has not been added or the address has not
been announced (I guess the initial address is not there then).

Also, with the userspace PM, it is possible to have an entry with an
ADDR ID == 0.


Do you think this patch is worth it? Setting by ID for the in-kernel PM
makes sense: unique ID for the netns, easier to type the ID than the
full address. While for the userspace PM, it will be managed by a daemon
that will have to track addresses anyway.


>  	}
> +
> +	if (bkup)
> +		entry->flags |= MPTCP_PM_ADDR_FLAG_BACKUP;
> +	else
> +		entry->flags &= ~MPTCP_PM_ADDR_FLAG_BACKUP;
>  	spin_unlock_bh(&msk->pm.lock);
>  
>  	lock_sock(sk);
> -	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &loc.addr, &rem.addr, bkup);
> +	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &entry->addr, &rem.addr, bkup);
>  	release_sock(sk);
>  
>  set_flags_err:

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