Hi Mat & Matt,
On Tue, 2024-01-30 at 18:28 -0800, Mat Martineau wrote:
> On Thu, 18 Jan 2024, Geliang Tang wrote:
>
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> >
> > Just like MPTCP_PM_ADDR_FLAG_SIGNAL flag is checked in userspace PM
> > announce mptcp_pm_nl_announce_doit(), MPTCP_PM_ADDR_FLAG_SUBFLOW
> > flag
> > should be checked in mptcp_pm_nl_subflow_create_doit() too.
> >
> > If MPTCP_PM_ADDR_FLAG_SUBFLOW flag is not set, there's no flags
> > field
> > in the output of dump_addr. This looks a bit strange:
> >
> > id 10 flags 10.0.3.2
> >
> > This patch uses mptcp_pm_parse_entry() instead of
> > mptcp_pm_parse_addr()
> > to get the flags of the entry. Add MPTCP_PM_ADDR_FLAG_SUBFLOW flag
> > check
> > in mptcp_pm_nl_subflow_create_doit().
> >
>
> Hi Geliang -
>
> Have you tested this change with mptcpd?
What do you think to test 'mptcpd' in our CI too? If you agree, I'll
try to do this, and add a new ticket about it in Github.
>
> Is the addr flag (SUBFLOW or SIGNAL) relevant for userspace PM
> addresses?
> It doesn't seem like a useful distinction: the userspace PM is always
> creating a new subflow in this context, since address signaling is
> handled
> by mptcp_pm_nl_announce_doit().
>
> Two options I see:
> * update the userspace tools to cleanly handle missing flags for
> userspace connections
> or
> * have mptcp_pm_nl_subflow_create_doit() always set
> MPTCP_PM_ADDR_FLAG_SUBFLOW for the local addr even if it's missing
> from
> the netlink attribute (and maybe don't allow
> MPTCP_PM_ADDR_FLAG_SIGNAL?)
Thanks for your suggestions. Option 2 has been updated in v11.
-Geliang
>
> - Mat
>
>
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> > net/mptcp/pm_userspace.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > index f1582f40f70e..ca0d6e1dfade 100644
> > --- a/net/mptcp/pm_userspace.c
> > +++ b/net/mptcp/pm_userspace.c
> > @@ -361,11 +361,18 @@ int mptcp_pm_nl_subflow_create_doit(struct
> > sk_buff *skb, struct genl_info *info)
> > goto create_err;
> > }
> >
> > - err = mptcp_pm_parse_addr(laddr, info, &addr_l);
> > + err = mptcp_pm_parse_entry(laddr, info, true, &local);
> > if (err < 0) {
> > NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error
> > parsing local addr");
> > goto create_err;
> > }
> > + addr_l = local.addr;
> > +
> > + if (!(local.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW)) {
> > + GENL_SET_ERR_MSG(info, "invalid addr flags");
> > + err = -EINVAL;
> > + goto create_err;
> > + }
> >
> > err = mptcp_pm_parse_addr(raddr, info, &addr_r);
> > if (err < 0) {
> > @@ -379,7 +386,6 @@ int mptcp_pm_nl_subflow_create_doit(struct
> > sk_buff *skb, struct genl_info *info)
> > goto create_err;
> > }
> >
> > - local.addr = addr_l;
> > err = mptcp_userspace_pm_append_new_local_addr(msk,
> > &local,
> >
> > !mptcp_pm_has_addr_attr_id(laddr, info));
> > if (err < 0) {
> > --
> > 2.40.1
> >
> >
> >