netlink path manager: Wrong interface id for created/established

[teto]

Here is an output of my wireshark fork to decode the mptcp netlink messages:

In the first created/established event (for the meta I guess), the interface id is always 0 (same for local/remote ids). When subflow events are created then the values are correct. Any reason not to set the correct values straightaway ? In my path manager it means I discard the MPTCP_EVENT_CREATED / MPTCP_EVENT_ESTABLISHED and wait for MPTCP_EVENT_SUB_ESTABLISHED instead but that sounds suboptimal.

[cpaasch]

The reported interface-ID comes from sk->sk_bound_dev_if, meaning the subflow did an SO_BINDTODEV before the connect(). Thus, for the initial subflow that entirely depends on whether or not the app used that socket-option. That's why you see 0.

MPTCP_EVENT_SUB_ESTABLISHED is only posted for additional subflows, thus these are getting the bindtodev.

@matttbe - should mptcp_nl_put_subsk rather get the if-idx from the current interface?

[teto]

MPTCP_EVENT_SUB_ESTABLISHED is only posted for additional subflows, thus these are getting the bindtodev.

thanks. I am fine with that, just a bit surprised, my reasoning being it's best not to submit the interface id if it's wrong. I ponder about creating a wiki page about the netlink protocol.

MPTCP_EVENT_SUB_ESTABLISHED is only posted for additional subflows, thus these are getting the bindtodev.

From my pcaps, it seems the event is also triggered for the master subflow.

[cpaasch]

thanks. I am fine with that, just a bit surprised, my reasoning being it's best not to submit the interface id if it's wrong.

Yes - probably, when the subflow is not bound to the interface it is ok to give 0, as the subflow could move if the routing changes.

I ponder about creating a wiki page about the netlink protocol.

A wiki-page about the netlink protocol would be awesome! :)

From my pcaps, it seems the event is also triggered for the master subflow.

Hmmm... Looking at the code, it does not seem to me to be the case.

[teto]

Hmmm... Looking at the code, it does not seem to me to be the case.

Indeed: I made a new capture/check the code to be sure and that isn't the case. Which means I have to fetch the interface idx myself which is a bit annoying.
As a I see it, either we don't send MPTCP_ATTR_IF_IDX for master or we send the correct interface if this can be retreived easily (otherwise the path manager should have the information in store anyway).

[cpaasch]

@matttbe - what do you think? I agree with @teto that the interface is not consistent here.

[matttbe]

Hi!

Sorry for the delay!

I ponder about creating a wiki page about the netlink protocol.

A wiki-page about the netlink protocol would be awesome! :)

👍

should mptcp_nl_put_subsk rather get the if-idx from the current interface?

@cpaasch : I am sorry but what do you mean? Just to be sure here you are talking about the master sf not having an if_idx, right? (I guess you don't want to do a lookup in the routing table for non established SF, right? :) )
Is the issue that @teto, you have done a setsockopt(fd, SOL_IP, IP_TRANSPARENT, ...) and we don't manage the "transparent" case?

diff --git a/net/mptcp/mptcp_netlink.c b/net/mptcp/mptcp_netlink.c
index 85964e473ab7..28e3b6c72fc8 100644
--- a/net/mptcp/mptcp_netlink.c
+++ b/net/mptcp/mptcp_netlink.c
@@ -182,7 +182,8 @@ mptcp_nl_put_subsk(struct sk_buff *msg, struct sock *sk)
        if (nla_put_u8(msg, MPTCP_ATTR_BACKUP, backup))
                goto nla_put_failure;
 
-       if (nla_put_s32(msg, MPTCP_ATTR_IF_IDX, sk->sk_bound_dev_if))
+       if (nla_put_s32(msg, MPTCP_ATTR_IF_IDX, isk->transparent ?
+                       isk->rx_dst_ifindex : sk->sk_bound_dev_if))
                goto nla_put_failure;
 
        sk_err = sk->sk_err ? : tcp_sk(sk)->mptcp->sk_err;

Does this patch help or do we need to retrieve the info from somewhere else for mpcb->master_sk?

I agree with @teto that the interface is not consistent here.

For history reasons, that's likely possible that this interface is not consistent indeed: from the beginning, this path manager is using struct mptcp_loc[46] structures to store data about the local addresses. Later, in 9e76311, a new field has been added in these structures: if_idx. It is not essential to support it in this Netlink PM but because it was easy and not to have an unused field, it has been decided to first add the possibility, from the userspace, to force an interface ID. I guess most people doesn't even need this but it is there just in case we want to fully mimic the current Fullmesh PM. At the same time, we added the possibility to retrieve this info if set. That's then likely possible that something is missing there for the master subflow as we don't use this info :-)

[teto]

I just ran an iperf3 session, which doesn't seem to contain any IP_TRANSPARENT call: https://github.com/esnet/iperf/search?q=IP_TRANSPARENT&unscoped_q=IP_TRANSPARENT .
I don't really mind having the interface id but the PM should not send a deceiving interface id.

[cpaasch]

wrt to deceiving interface id - the id you are getting is 0, right?

It is confusing indeed, and I think that the nl-pm should not give an if-index when so_bindtodev is 0. It should simply omit that field IMO.

[teto]

yes it's 0.

[matttbe]

@cpaasch is it not possible to get the interface id when so_bindtodev is 0? Is it because it can changed? Except if so_bindtodev is set?

[cpaasch]

When so_bindtodev is 0, the outgoing interface purely depends on the routing. And, as you say - that can change. So, if the user-space daemon receives the src/dst ip-addresses without the if-idx it is enough to determine what the outgoing interface is, simply by doing a route-lookup.