Userspace PM can't specify if_idx on MPTCP_PM_CMD_SUBFLOW_CREATE

[majek]

Pre-requisites

• A similar idea has not been reported before.
• mptcp.dev website does not cover my case.
• The wiki doesn't cover my case.

Description

I'm playing with pm_nl_ctl to create subflows, like:

pm_nl_ctl csf lip 192.168.2.143 lid 120 rip 1.2.3.4 rport 12000 token $[0x33757f81]

And, first of course the CLI doesn't allow specifying ifindex of the path. But this is fine, I can patch it:

linux/tools/testing/selftests/net/mptcp$ git diff .
diff --git a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
index 994a556f46c1..f200d25d7189 100644
--- a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
+++ b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
@@ -448,7 +448,7 @@ int csf(int fd, int pm_family, int argc, char *argv[])
                  NLMSG_ALIGN(sizeof(struct genlmsghdr)) +
                  1024];
        u_int32_t flags = MPTCP_PM_ADDR_FLAG_SUBFLOW;
-       const char *params[5];
+       const char *params[6];
        struct nlmsghdr *nh;
        struct rtattr *addr;
        struct rtattr *rta;
@@ -460,7 +460,7 @@ int csf(int fd, int pm_family, int argc, char *argv[])
        int off = 0;
        int arg;
 
-       memset(params, 0, 5 * sizeof(const char *));
+       memset(params, 0, 6 * sizeof(const char *));
 
        memset(data, 0, sizeof(data));
        nh = (void *)data;
@@ -499,6 +499,11 @@ int csf(int fd, int pm_family, int argc, char *argv[])
                                error(1, 0, " missing token");
 
                        params[4] = argv[arg];
+               } else if (!strcmp(argv[arg], "dev")) {
+                       if (++arg >= argc)
+                               error(1, 0, " missing dev");
+
+                       params[5] = argv[arg];
                } else
                        error(1, 0, "unknown param %s", argv[arg]);
        }
@@ -541,6 +546,7 @@ int csf(int fd, int pm_family, int argc, char *argv[])
                        rta->rta_len = RTA_LENGTH(2);
                        memcpy(RTA_DATA(rta), &port, 2);
                        off += NLMSG_ALIGN(rta->rta_len);
+
                }
 
                if (arg == 0) {
@@ -551,6 +557,13 @@ int csf(int fd, int pm_family, int argc, char *argv[])
                        rta->rta_len = RTA_LENGTH(1);
                        memcpy(RTA_DATA(rta), &id, 1);
                        off += NLMSG_ALIGN(rta->rta_len);
+
+                       int32_t ifindex = if_nametoindex(params[5]);
+                       rta = (void *)(data + off);
+                       rta->rta_type = MPTCP_PM_ADDR_ATTR_IF_IDX;
+                       rta->rta_len = RTA_LENGTH(4);
+                       memcpy(RTA_DATA(rta), &ifindex, 4);
+                       off += NLMSG_ALIGN(rta->rta_len);
                }
 
                /* addr flags */

However, this still doesn't work. I don't think the MPTCP_PM_ADDR_ATTR_IF_IDX is taken into account when creating the flow...

The ip mptcp monitor seems to confirm.

The subflow created by pm_type=0 reports ifindex on subflow:

[SF_ESTABLISHED] token=bce365d4 remid=0 locid=13 saddr4=192.168.1.149 daddr4=X sport=51711 dport=12000 backup=0 ifindex=2

However, the subflow created by pm_type=1 and the patched pm_nl_ctl csf tool misses the ifindex in logline:

[SF_ESTABLISHED] token=8a924490 remid=0 locid=15 saddr4=192.168.2.63 daddr4=x sport=40489 dport=12000 backup=0

Solution

Make sure MPTCP_PM_ADDR_ATTR_IF_IDX is taken into account when dealing with MPTCP_PM_CMD_SUBFLOW_CREATE in kernel.

Considered alternatives

n/a

Additional context

No response

[matttbe]

Hi @majek,

Thank you for having reported this issue!

That's strange, the userspace PM should take if_idx into account. In mptcp_pm_nl_subflow_create_doit(), the "entry" is filled in, that's where MPTCP_PM_ADDR_ATTR_IF_IDX is parsed:

mptcp_net-next/net/mptcp/pm_userspace.c

Line 385 in e537fe4

err = mptcp_pm_parse_entry(laddr, info, true, &entry);

The info is passed to __mptcp_subflow_connect():

mptcp_net-next/net/mptcp/pm_userspace.c

Lines 416 to 422 in e537fe4

	local.addr = entry.addr;
	local.flags = entry.flags;
	local.ifindex = entry.ifindex;
	lock_sock(sk);
	err = __mptcp_subflow_connect(sk, &local, &addr_r);
	release_sock(sk);

Which will pass the info to the bind():

mptcp_net-next/net/mptcp/subflow.c

Lines 1646 to 1647 in e537fe4

	ssk->sk_bound_dev_if = local->ifindex;
	err = kernel_bind(sf, (struct sockaddr *)&addr, addrlen);

That's on the dev version. Which kernel are you using?

I didn't check the userspace code (pm_nl_ctl.c), but just to be sure: is the info correctly passed? (Are you not setting the info in a for-loop which is a bit "strange" if I remember well.

[majek]

I'm on kernel 6.8.

is the info correctly passed

I have absolutely no idea. Let me try to debug the netlink. Here's what I send over netlink from pm_nl_ctl when I do csf:

sendto(3, 
[{nlmsg_len=108, nlmsg_type=mptcp_pm, nlmsg_flags=NLM_F_REQUEST|NLM_F_ACK, nlmsg_seq=0, nlmsg_pid=0}, 
{cmd=0xa, version=1}, 
[[{nla_len=44, nla_type=NLA_F_NESTED|0x1},
"\x08\x00\x03\x00\xc0\xa8\x01\x95\x06\x00\x01\x00\x02\x00\x00\x00\x05\x00\x02\x00\x0d\x00\x00\x00\x08\x00\x07\x00\x02\x00\x00\x00\x08\x00\x06\x00\x02\x00\x00\x00"],
 [{nla_len=36, nla_type=NLA_F_NESTED|0x6},
"\x08\x00\x03\x00\x68\x1c\x98\x9d\x06\x00\x01\x00\x02\x00\x00\x00\x06\x00\x05\x00\xe0\x2e\x00\x00\x08\x00\x06\x00\x02\x00\x00\x00"],
[{nla_len=8, nla_type=0x4}, 
"\xa1\x9e\x90\x60"]]], 108, 0, {sa_family=AF_NETLINK, nl_pid=0, nl_groups=00000000}, 12) = 108

MPTCP_PM_CMD_SUBFLOW_CREATE = 0xa
MPTCP_PM_ATTR_ADDR = 0x1
MPTCP_PM_ATTR_ADDR_REMOTE = 0x6

{cmd=MPTCP_PM_CMD_SUBFLOW_CREATE, version=1},
[
 [{nla_len=44, nla_type=NLA_F_NESTED|MPTCP_PM_ATTR_ADDR},
               "\x08\x00\x03\x00\xc0\xa8\x01\x95   MPTCP_PM_ADDR_ATTR_ADDR4=3
                \x06\x00\x01\x00\x02\x00\x00\x00   MPTCP_PM_ADDR_ATTR_FAMILY=1
                \x05\x00\x02\x00\x0d\x00\x00\x00   MPTCP_PM_ADDR_ATTR_ID=2
                \x08\x00\x07\x00\x02\x00\x00\x00   MPTCP_PM_ADDR_ATTR_IF_IDX=7
                \x08\x00\x06\x00\x02\x00\x00\x00"  MPTCP_PM_ADDR_ATTR_FLAGS=6
               ],
 [{nla_len=36, nla_type=NLA_F_NESTED|MPTCP_PM_ATTR_ADDR_REMOTE},
               "\x08\x00\x03\x00\x68\x1c\x98\x9d   MPTCP_PM_ADDR_ATTR_ADDR4=3
                \x06\x00\x01\x00\x02\x00\x00\x00   MPTCP_PM_ADDR_ATTR_FAMILY=1
                \x06\x00\x05\x00\xe0\x2e\x00\x00   MPTCP_PM_ADDR_ATTR_PORT=5
                \x08\x00\x06\x00\x02\x00\x00\x00"  MPTCP_PM_ADDR_ATTR_FLAGS=6
               ],
 [{nla_len=8, nla_type=0x4}, "\xa1\x9e\x90\x60"]]

I guess the if_idx should be on local address. And I guess it looks fine?

[majek]

For the record, this is a change between 6.12 and master that changes the ifindex handling
torvalds/linux@b83fbca
(I'm on 6.8)

[majek]

Ok, I have it. AFAIU in kernels before and including v6.12 the ifindex of a path is taken from the "annouced list".
https://elixir.bootlin.com/linux/v6.8.12/source/net/mptcp/pm_userspace.c#L109

That is: the passed ifindex o MPTCP_PM_CMD_SUBFLOW_CREATE is ignored. This means, that like in my case, when I don't announce anything , the ifindex is assumed zero, which gets all wrong.

However, when I do pm_nl_ctl ann before csf, it works, so this is the correct incantation:

pm_nl_ctl ann 192.168.1.149 id 15 token 2023613974 dev wlp0s20f3
pm_nl_ctl csf lip 192.168.1.149 lid 15 rip XXX rport 12000 token 2023613974 dev wlp0s20f3

However, of course this means stupid add-addr with my local IP on the wire!

host > host : Flags [.], ack 6, win 502, options [nop,nop,TS val x ecr x,mptcp 16 add-addr v1 id 15 192.168.1.149 hmac x], length 0

Which is totally braindead.

Anyway, I think this is all I needed now. I guess the final question is - is this fixed in master, and can I just do pm_nl_ctl csf with "dev" without needing the ann before.

[majek]

Ok, off 6.12 it looks fine! Ie: my patch that adds MPTCP_PM_ADDR_ATTR_IF_IDX to pm_nl_ctl does indeed work and I can now see the proper iface-bound flows:

tcp   ESTAB 0      0           192.168.1.149%wlp0s20f3:56713  x:443
tcp   ESTAB 0      0      192.168.2.63%enx00e04c380ccb:33957 x:443