Re: [PATCH mptcp-next v3 5/8] mptcp: userspace pm set_flags id support

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

Thanks for the review!

On Wed, 2025-01-08 at 19:51 +0100, Matthieu Baerts wrote:
> On 08/01/2025 19:47, Matthieu Baerts wrote:
> > Hi Geliang,
> > 
> > On 08/01/2025 05:21, 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().
> > 
> > Mmh, I'm still not sure about that. As I was saying in [1], 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).

The previous version (in [1]) did have this issue and userspace_pm.sh
tests would fail because of it, but this new version has fixed it.

 mptcp_pm_nl_mp_prio_send_ack(msk,
                              entry ? &entry->addr : &local->addr,
                              remote, bkup);

When the entry is not found, we continue to pass local->addr to ensure
the same behavior as before.

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

I think it's still useful to extend this functionality while the
original behavior is not affected, at least it doesn't hurt.

We cannot assume that userspace PM is always managed by a daemon. We
have exported its interfaces to BPF. We allow users to customize path
managers. That means we also allow users to use their own userspace PM
in any way.

Another consideration is that we need to maintain the consistency
between in-kernel PM and userspace PM. For ease of maintenance, we need
to make these two PMs use the same code as much as possible, and only
abstract their differences through PM interfaces such as get_addr,
dump_addr, set_flags, etc. At present, the biggest difference between
the two is that they use different linked lists (pernet-
>local_addr_list vs. msk->pm.userspace_pm_local_addr_list) to store
address entries, so we only need to put the code for operating the
linked lists into the interfaces of each PM. This is also the goal of
adjusting the pm interfaces in this series.

> > 
> > Or in other words, do you have a use-case for this? To me, it looks
> > like
> > "yes, you can only set the ID, but it might not always work". Then
> > maybe
> > better to always set the full address, no?

If you're worried that this functionality isn't covered by tests, I've
added a test that covers it in BPF path manager selftests:

        err = userspace_pm_set_flags(token, addr, "backup");
        if (!ASSERT_OK(err, "userspace_pm_set_flags backup"))
                goto close_accept;

	...

        err = userspace_pm_set_flags_by_id(token, 100, "nobackup");
        if (!ASSERT_OK(err, "userspace_pm_set_flags_by_id nobackup"))
                goto close_accept;

> > 
> > [1]
> > https://lore.kernel.org/mptcp/d01d0e8a-5606-4152-aabe-32e4402adeeb@kernel.org/
> 
> Note: if we drop this patch (I think it is better), maybe patch 8/8
> is
> not worth it: not to have a "common" section with plenty of 'if
> (token)', no? Or do you really need them for the BPF PM?

Here we are only adjusting set_flags interface of in-kernel PM and
userspace PM, which has nothing to do with the BPF PM implementation.

It seems that moving the code in mptcp_pm_nl_set_flags_doit() to
mptcp_pm_set_flags() can remove these 'if (token)':

int mptcp_pm_nl_set_flags_doit(struct sk_buff *skb, struct genl_info
*info)
{
        return mptcp_pm_set_flags(info);
}

static int mptcp_pm_set_flags(struct genl_info *info)
{
        struct mptcp_pm_addr_entry loc = { .addr = { .family =
AF_UNSPEC }, };
        struct mptcp_addr_info rem = { .family = AF_UNSPEC, };
        struct nlattr *attr_loc, *attr_rem;
        int ret;

        if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
                return -EINVAL;

        attr_loc = info->attrs[MPTCP_PM_ATTR_ADDR];
        ret = mptcp_pm_parse_entry(attr_loc, info, false, &loc);
        if (ret < 0)
                return ret;

        if (info->attrs[MPTCP_PM_ATTR_TOKEN]) {
                if (GENL_REQ_ATTR_CHECK(info,
MPTCP_PM_ATTR_ADDR_REMOTE))
                        return -EINVAL;

                attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
                ret = mptcp_pm_parse_addr(attr_rem, info, &rem);
                if (ret < 0)
                        return ret;

                if (rem.family == AF_UNSPEC) {
                        NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
                                            "invalid remote address
family");
                        return -EINVAL;
                }

                return mptcp_userspace_pm_set_flags(&loc, &rem, info);
        }

        if (loc.addr.family == AF_UNSPEC) {
                if (!loc.addr.id) {
                        NL_SET_ERR_MSG_ATTR(info->extack, attr_loc,
                                            "missing address ID");
                        return -EOPNOTSUPP;
		}
        }

        return mptcp_pm_nl_set_flags(&loc, info);
}

WDYT?

-Geliang

> 
> Cheers,
> Matt