Re: [PATCH mptcp-next v2 23/36] mptcp: drop struct mptcp_pm_local

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Thu, 2024-11-07 at 11:00 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 07/11/2024 08:29, Geliang Tang wrote:
> > Hi Matt,
> > 
> > On Mon, 2024-11-04 at 20:08 +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_pm_nl_subflow_create_doit() that
> > > > assigns struct
> > > > mptcp_pm_addr_entry "entry" to the local struct mptcp_pm_local
> > > > variable
> > > > "local" is not allowed in BPF if we use the same code to
> > > > implement
> > > > the
> > > > subflow_create() interface of a BFP path manager:
> > > > 
> > > > 	struct mptcp_pm_local local;
> > > > 
> > > > 	local.addr = entry.addr;
> > > > 	local.flags = entry.flags;
> > > > 	local.ifindex = entry.ifindex;
> > > 
> > > Here as well, I'm not sure to understand the issue. Is it because
> > > you
> > > cannot allocate this structure locally with BPF? No alternatives?
> > 
> > Here "struct mptcp_pm_local local" is a "scalar", when passing it
> > to
> > __mptcp_subflow_connect(), this error occurs:
> > 
> > # ; err = __mptcp_subflow_connect(sk, &local, remote); @
> > mptcp_bpf_userspace_pm.c:242
> > # 53: (bf) r1 = r6                      ;
> > R1_w=trusted_ptr_mptcp_sock()
> > R6=trusted_ptr_mptcp_sock()
> > # 54: (bf) r3 = r8                      ;
> > R3_w=trusted_ptr_mptcp_addr_info() R8=trusted_ptr_mptcp_addr_info()
> > # 55: (85) call __mptcp_subflow_connect#34404
> > # max struct nesting depth exceeded
> > # arg#1 pointer type STRUCT mptcp_pm_local must point to scalar, or
> > struct with scalar
> > 
> > Also, assigning an address to an address like this is not allowed
> > in
> > BPF:
> > 
> > 	local.addr = entry.addr;
> 
> OK, thank you, that's a bit clearer (even if it is not clear what BPF
> code was causing that). For this kind of issue, would it work to add
> a
> BPF-specific kfunc, eventually with additional annotations or
> arguments,
> or taking a "struct mptcp_pm_addr_entry" in argument, and "simply"
> calling __mptcp_subflow_connect() with a "struct mptcp_pm_local"?

It works indeed. But I prefer the version in v3, "mptcp: use
mptcp_pm_local in pm_netlink only". It makes more sense to move
mptcp_pm_local in pm_netlink.c.

WDYT?

Thanks,
-Geliang

> 
> So something specific to BPF, without impacting the rest?
> 
> Cheers,
> Matt