sspi: using wrong dev when send local address to kernel?

[andywu106]

Describe the bug When a new mptcp conntion create, SSPI will send each address associate with all the network interfaces other than which the new connection was created to kernel，but it is always using the same network interface index in sspi_send_addr()，and it is not the interface which address belongs to. eg. I have two nic, ens33 and ens37, when get a new mptcp connection at 192.168.174.128(ens37), addresses in ens33 is send to kernel by SSPI, but these addresses are associated with ens37.

Is this a BUG?

2: ens33: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000 link/ether 00:0c:29:bb:2e:ca brd ff:ff:ff:ff:ff:ff inet 192.168.234.147/24 brd 192.168.234.255 scope global noprefixroute dynamic ens33 valid_lft 1715sec preferred_lft 1715sec inet 192.168.234.140/24 brd 192.168.234.255 scope global secondary dynamic ens33 valid_lft 1290sec preferred_lft 1290sec inet6 fe80::63d2:1cb6:e7c6:ae30/64 scope link noprefixroute valid_lft forever preferred_lft forever 3: ens37: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000 link/ether 00:0c:29:bb:2e:d4 brd ff:ff:ff:ff:ff:ff inet 192.168.174.128/24 brd 192.168.174.255 scope global noprefixroute ens37 valid_lft forever preferred_lft forever inet 192.168.174.130/24 brd 192.168.174.255 scope global secondary dynamic ens37 valid_lft 1480sec preferred_lft 1480sec inet6 fe80::2ace:2b3c:1129:67b4/64 scope link noprefixroute valid_lft forever preferred_lft forever

[root@bogon mptcp]# ./pm_nl_ctl dump id 1 flags dev ens37 192.168.234.147 id 2 flags dev ens37 192.168.234.140 id 3 flags dev ens37 fe80::63d2:1cb6:e7c6:ae30

To Reproduce Steps to reproduce the behavior:

1. run mptcpd
2. create mptcp connection
3. ./pm_nl_ctl dump

[ossama-othman]

Thanks for the detailed description. This does seem like a bug. I'll try to reproduce it, and fix it accordingly.

[andywu106]

Thanks, I have some other questions:

1. sspi will send all local addresses associated with a network interface, so if a nic have more than one address, it will create more than one subflow on that nic, not sigle subflow per interface as sspi is intend to do?
2. now sspi doesn't set any flags on local address, so it won't signal to remote side, or create subflow?
3. sspi_send_addr() set address_id to 0, this will cause return EINVAL in mptcpd_pm_add_addr(), and address is not sent to kernel really?

[andywu106]

4. every new mptcp connection(MPTCP_EVENT_CREATED) event will trigger sspi send local address to kernel? this leads to a lot of repetitive work.

[ossama-othman]

1. sspi will send all local addresses associated with a network interface, so if a nic have more than one address, it will create more than one subflow on that nic, not sigle subflow per interface as sspi is intend to do?

The sspi plugin should remove the subflow if it detects an existing subflow on a given NIC. In sspi_new_subflow():

        if (l_queue_find(info->tokens,
                         sspi_token_match,
                         L_UINT_TO_PTR(token)) != NULL) {
                l_warn("Subflow already exists on network "
                       "interface (%d). "
                       "Closing new subflow.",
                        info->index);

                mptcpd_pm_remove_subflow(pm,
                                         token,
                                         laddr,
                                         raddr);

                return;
        }

In the scenario you described this is not very efficient due to the additional netlink messages that could potentially have been prevented earlier in the sspi_new_connection() event handler. At the time the sspi plugin was written it was believed that such a scenario was not common, or at the very least IP addresses on a given NIC would not change often, meaning this plugin behavior shouldn't be a performance bottleneck. Regardless,

Please keep in mind that mptcpd, along with its plugins, is meant to be a reference implementation of a user space path manager. :)

2. now sspi doesn't set any flags on local address, so it won't signal to remote side, or create subflow?

This is a known bug that is slated to be fixed for the next (0.8) mptcpd release. A similar problem exists in the addr_adv plugin. I'll create an issue in GitHub to make it easier to track progress.

3. sspi_send_addr() set address_id to 0, this will cause return EINVAL in mptcpd_pm_add_addr(), and address is not sent to kernel really?

Thanks for pointing this out. The EINVAL caused by the zero address ID is a vestige of the original implementation for the multpath-tcp.org kernel. That kernel requires the address ID for the add_addr command, but the upstream kernel does not. I'll move the zero address_id check that triggers the EINVAL to the multipath-tcp.org implementation in src/netlink_pm_mptcp_org.c.

There was also some confusion in the mptcpd_pm_add_addr() and mptcd_pm_remove_addr() functions where overlap in support for the multipath-tcp.org and upstream kernel was implemented, but really there should be separate implementations since the multipath-tcp.org kernel PM netlink API's add_addr command is client-oriented (per-connection) whereas the upstream kernel PM netlink API's add_addr command is server-oriented. Path management is done in the kernel for the latter. We are currently planning to remove the overlap, and split add_addr and remove_addr functionality into client- and server-oriented functions. Doing so would also remove unused parameters from the functions, depending on the use case. No GitHub issue exists for this yet, but it is listed as a note in the mptcpd 0.8 project.

[ossama-othman]

1. every new mptcp connection(MPTCP_EVENT_CREATED) event will trigger sspi send local address to kernel? this leads to a lot of repetitive work.

That's a good point. It would probably be good to track whether or not an add_addr for given IP address is necessary prior to issuing that command.

[andywu106]

Hi ossama，thank you for your patient explanation:)