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

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Mon, 2024-11-04 at 19:48 +0100, Matthieu Baerts wrote:
> 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?

Same as in patch 23, passing "new_entry" to
mptcp_userspace_pm_append_new_local_addr() will get an error:

"pointer type STRUCT mptcp_pm_addr_entry must point to scalar, or
struct with scalar".

And assigning an address to an address like this is not allowed in
BPF:

	new_entry.addr = *skc;

> 
> > 	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 };

Updated in v3.

> 
> (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?

Updated in v3.

Thanks,
-Geliang

> 
> > +	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