Re: [PATCH mptcp-next v2 21/36] mptcp: update address type of get_local_id

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 22/10/2024 11:14, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The following code in mptcp_userspace_pm_get_local_id() that assigns "skc"
> to "new_entry" is not allowed in BPF if we use the same code to implement
> the get_local_id() interface of a BFP path manager:

Can you give more details about the fact it is not allowed please?

Do you mean you cannot do the following code in BPF? Why?

> 	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
> 	new_entry.addr = *skc;
> 	new_entry.addr.id = 0;
> 	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
> 
> To solve the issue, this patch moves this assignment to "new_entry" forward
> to mptcp_pm_get_local_id(), and then passing "new_entry" as a parameter to
> both mptcp_pm_nl_get_local_id() and mptcp_userspace_pm_get_local_id().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c           | 10 ++++++++--
>  net/mptcp/pm_netlink.c   | 11 +++--------
>  net/mptcp/pm_userspace.c | 17 ++++++-----------
>  net/mptcp/protocol.h     |  4 ++--
>  4 files changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index b6e6859903ef..d28e844eba2d 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -404,6 +404,7 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
>  {
>  	struct mptcp_addr_info skc_local;
>  	struct mptcp_addr_info msk_local;
> +	struct mptcp_pm_addr_entry local;
>  
>  	if (WARN_ON_ONCE(!msk))
>  		return -1;
> @@ -416,9 +417,14 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
>  	if (mptcp_addresses_equal(&msk_local, &skc_local, false))
>  		return 0;
>  
> +	memset(&local, 0, sizeof(struct mptcp_pm_addr_entry));

Out of curiosity: do you need to call memset() here? Can you not init
"local" when you declare it?

  struct mptcp_pm_addr_entry local = { 0 };

(Or maybe that's not working with C99 with inner structures? I thought
it was, but not sure).

> +	local.addr = skc_local;

Do you still need skc_local? Can you not use 'local.addr' directly
instead above?

> +	local.addr.id = 0;

In pm_netlink.c, 'addr.port' is also reset. Should you not do that here
too? I guess that's fine for the userspace pm, no?

> +	local.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
> +
>  	if (mptcp_pm_is_userspace(msk))
> -		return mptcp_userspace_pm_get_local_id(msk, &skc_local);
> -	return mptcp_pm_nl_get_local_id(msk, &skc_local);
> +		return mptcp_userspace_pm_get_local_id(msk, &local);
> +	return mptcp_pm_nl_get_local_id(msk, &local);
>  }
>  
>  bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
(...)

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