Re: [PATCH bpf-next v4 2/2] selftests/bpf: Add mptcp subflow subtest

[Geliang Tang <geliang@kernel.org>]

Hi Martin, Matt,

On Wed, 2024-08-14 at 15:37 -0700, Martin KaFai Lau wrote:
> On 8/14/24 3:04 AM, Matthieu Baerts wrote:
> > Hi Martin,
> > 
> > Thank you for your reply!
> > 
> > On 14/08/2024 03:12, Martin KaFai Lau wrote:
> > > On 8/5/24 2:52 AM, Matthieu Baerts (NGI0) wrote:
> > > > +static int endpoint_init(char *flags)
> > > > +{
> > > > +    SYS(fail, "ip -net %s link add veth1 type veth peer name
> > > > veth2",
> > > > NS_TEST);
> > > > +    SYS(fail, "ip -net %s addr add %s/24 dev veth1", NS_TEST,
> > > > ADDR_1);
> > > > +    SYS(fail, "ip -net %s link set dev veth1 up", NS_TEST);
> > > > +    SYS(fail, "ip -net %s addr add %s/24 dev veth2", NS_TEST,
> > > > ADDR_2);
> > > > +    SYS(fail, "ip -net %s link set dev veth2 up", NS_TEST);
> > > > +    if (SYS_NOFAIL("ip -net %s mptcp endpoint add %s %s",
> > > > NS_TEST,
> > > > ADDR_2, flags)) {
> > > > +        printf("'ip mptcp' not supported, skip this test.\n");
> > > > +        test__skip();
> > > 
> > > It is always a skip now in bpf CI:
> > > 
> > > #171/3   mptcp/subflow:SKIP
> > > 
> > > This test is a useful addition for the bpf CI selftest.
> > > 
> > > It can't catch regression if it is always a skip in bpf CI
> > > though.
> > 
> > Indeed, for the moment, this test is skipped in bpf CI.
> > 
> > The MPTCP CI checks the MPTCP BPF selftests that are on top of net
> > and
> > net-next at least once a day. It is always running with the last
> > stable
> > version of iproute2, so this test is not skipped:
> > 
> >     #169/3   mptcp/subflow:OK
> > 
> > https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10384566794/job/28751869426#step:7:11080
> > 
> > > iproute2 needs to be updated (cc: Daniel Xu and Manu, the
> > > outdated
> > > iproute2 is something that came up multiple times).
> > > 
> > > Not sure when the iproute2 can be updated. In the mean time, your
> > > v3 is
> > > pretty close to getting pm_nl_ctl compiled. Is there other
> > > blocker on this?
> > 
> > I will try to find some time to check the modifications I suggested
> > in
> > the v3, but I don't know how long it will take to have them ready,
> > as
> > they might require some adaptations of the CI side as well, I need
> > to
> > check. On the other hand, I understood adding a duplicated version
> > of
> > the mptcp.h UAPI header is not an option either.
> > 
> > So not to block this (already old) series, I thought it would help
> > to
> > first focus on this version using 'ip mptcp', while I'm looking at
> > the
> > selftests modifications. If these modifications are successful, I
> > can
> > always resend the patch 2/3 from the v3 later, and using
> > 'pm_nl_ctl'
> > instead of 'ip mptcp', to be able to work with IPRoute2 5.5.
> > 
> > Do you think that could work like that?
> 
> If there is CI started covering it, staying with the 'ip mptcp' is
> fine.
> 
> The bpf CI has to start testing it asap also. The iproute2 package
> will need to 
> be updated on the bpf CI side. I think this has to be done
> regardless.
> 
> It will be useful to avoid the uapi header dup on its own. The last
> one you have 
> seems pretty close.
> 
> > 
> > > > +        goto fail;
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +fail:
> > > > +    return -1;
> > > > +}
> > > > +
> > > > +static int _ss_search(char *src, char *dst, char *port, char
> > > > *keyword)
> > > > +{
> > > > +    return SYS_NOFAIL("ip netns exec %s ss -enita src %s dst
> > > > %s %s %d
> > > > > grep -q '%s'",
> > > > +              NS_TEST, src, dst, port, PORT_1, keyword);
> > > > +}
> > > > +
> > > > +static int ss_search(char *src, char *keyword)
> > > > +{
> > > > +    return _ss_search(src, ADDR_1, "dport", keyword);
> > > > +}
> > > > +
> > > > +static void run_subflow(char *new)
> > > > +{
> > > > +    int server_fd, client_fd, err;
> > > > +    char cc[TCP_CA_NAME_MAX];
> > > > +    socklen_t len = sizeof(cc);
> > > > +
> > > > +    server_fd = start_mptcp_server(AF_INET, ADDR_1, PORT_1,
> > > > 0);
> > > > +    if (!ASSERT_GE(server_fd, 0, "start_mptcp_server"))
> > > > +        return;
> > > > +
> > > > +    client_fd = connect_to_fd(server_fd, 0);
> > > > +    if (!ASSERT_GE(client_fd, 0, "connect to fd"))
> > > > +        goto fail;
> > > > +
> > > > +    err = getsockopt(server_fd, SOL_TCP, TCP_CONGESTION, cc,
> > > > &len);
> > > > +    if (!ASSERT_OK(err, "getsockopt(srv_fd, TCP_CONGESTION)"))
> > > > +        goto fail;
> > > > +
> > > > +    send_byte(client_fd);
> > > > +
> > > > +    ASSERT_OK(ss_search(ADDR_1, "fwmark:0x1"), "ss_search
> > > > fwmark:0x1");
> > > > +    ASSERT_OK(ss_search(ADDR_2, "fwmark:0x2"), "ss_search
> > > > fwmark:0x2");
> > > > +    ASSERT_OK(ss_search(ADDR_1, new), "ss_search new cc");
> > > > +    ASSERT_OK(ss_search(ADDR_2, cc), "ss_search default cc");
> > > 
> > > Is there a getsockopt way instead of ss + grep?
> > 
> > No there isn't: from the userspace, the app communicates with the
> > MPTCP
> > socket, which can have multiple paths (subflows, a TCP socket). To
> > keep
> > the compatibility with TCP, [gs]etsockopt() will look at/modify the
> > whole MPTCP connection. For example, in some cases, a setsockopt()
> > will
> > propagate the option to all the subflows. Depending on the option,
> > the
> > modification might only apply to the first subflow, or to the
> > user-facing socket.
> > 
> > For advanced users who want to have different options set to the
> > different subflows of an MPTCP connection, they can use BPF: that's
> > what
> > is being validated here. In other words, doing a 'getsockopt()'
> > from the
> > userspace program here will not show all the different marks and
> > TCP CC
> > that can be set per subflow with BPF. We can see that in the test:
> > a
> > getsockopt() is done on the MPTCP socket to retrieve the default
> > TCP CC
> > ('cc' which is certainly 'cubic'), but we expect to find another
> > one
> > ('new' which is 'reno'), set by the BPF program from patch 1/2. I
> > guess
> > we could use bpf to do a getsockopt() per subflow, but that's seems
> > a
> > bit cheated to have the BPF test program setting something and
> > checking
> > if it is set. Here, it is an external way. Because it is done from
> > a
> 
> I think the result is valid by having a bpf prog to inspect the value
> of a sock. 
> Inspecting socket is an existing use case. There are many existing
> bpf tests 
> covering this inspection use case to ensure the result is legit. A
> separate 
> cgroup/getsockopt program should help here (more on this below).
> 
> > dedicated netns, it sounds OK to do that, no?
> 
> Thanks for the explanation. I was hoping there is a way to get to the
> underlying 
> subflow fd. It seems impossible.
> 
> In the netns does help here. It is not only about the ss iterating a
> lot of 
> connections or not. My preference is not depending on external
> tool/shell-ing if 
> possible, e.g. to avoid the package update discussion like the
> iproute2 here. 
> The uapi from the testing kernel is always up-to-date. ss is another
> binary but 
> arguably in the same iproute2 package. There is now another extra
> "grep" and 
> pipe here. We had been bitten by different shell behaviors and some
> arch has 
> different shells ...etc.
> 
> I think it is ok to take this set as is if you (and Gelang?) are ok
> to followup 
> a "cgroup/getsockopt" way to inspect the subflow as the very next
> patch to the 
> mptcp selftest. It seems inspecting subflow will be a common test
> going forward 
> for mptcp, so it will be beneficial to have a "cgroup/getsockopt" way
> to inspect 
> the subflow directly.
> 
> Take a look at a recent example [0]. The mptcp test is under a cgroup
> already 
> and has the cgroup setup. An extra "cgroup/getsockopt" prog should be
> enough. 
> That prog can walk the msk->conn_list and use bpf_rdonly_cast (or the

I encountered some difficulties while walking the msk->conn_list in
BPF.

I added mptcp_for_iach_stubflow() and other helpers related to
list_dentry into progs/mptcp_bpf.h:

static inline int list_is_head(const struct list_head *list, const
struct list_head *head)
{
        return list == head;
}

#define list_entry(ptr, type, member) \
        container_of(ptr, type, member)

#define list_first_entry(ptr, type, member) \
        list_entry((ptr)->next, type, member)

#define list_next_entry(pos, member) \
        list_entry((pos)->member.next, typeof(*(pos)), member)

#define list_entry_is_head(pos, head, member) \
        list_is_head(&pos->member, (head))

#define list_for_each_entry(pos, head, member) \
        for (pos = list_first_entry(head, typeof(*pos), member); \
             !list_entry_is_head(pos, head, member); \
             pos = list_next_entry(pos, member))

#define mptcp_for_each_subflow(__msk, __subflow) \
        list_for_each_entry(__subflow, &((__msk)->conn_list), node)

Then used them in progs/mptcp_subflow.c like this:

SEC("cgroup/getsockopt")
int _getsockopt(struct bpf_sockopt *ctx)
{
        struct mptcp_sock *msk = bpf_core_cast(ctx->sk, struct
mptcp_sock);
        struct mptcp_subflow_context *subflow;
        __u32 token = 0;

        if (!msk || ctx->level != SOL_TCP ||
			ctx->optname != TCP_CONGESTION)
                return 1;

        subflow = list_first_entry(&msk->conn_list,
			struct mptcp_subflow_context, node);
        token = subflow->token;
        bpf_trace_printk(fmt, sizeof(fmt), msk, token); 

        return 1;
}

And got some access errors:

; token = subflow->token; @ mptcp_subflow.c:92
13: (61) r4 = *(u32 *)(r1 +524)
access beyond struct list_head at off 524 size 4

How can I resolve these errors? Shouldn't I walk the msk->conn_list
like this? Or use bpf_iter instead? Please give me some advice?

Thanks,
-Geliang

> bpf_core_cast macro in libbpf) to cast a pointer to tcp_sock for
> readonly. It 
> will allow to inspect all the fields in a tcp_sock.
> 
> Something needs to a fix in patch 2(replied separately), so a re-spin
> is needed.
> 
> pw-bot: cr
> 
> [0]:
> https://lore.kernel.org/all/20240808150558.1035626-3-alan.maguire@oracle.com/
> 
> 
>