optional values for MPTCP_CMD_SUB_CREATE in netlink_pm_mptcp_org.c

[VenkateswaranJ]

Hi,

It looks like the comment https://github.com/intel/mptcpd/blob/master/src/netlink_pm_mptcp_org.c#L317-L321 are not correct. Local address, remote address and remote port are not optional for MPTCP_CMD_SUB_CREATE command. Only source port, backup flag and if_idx are optional. Please check the out of tree mptcp Netlink path manager implementation at kernel side https://github.com/multipath-tcp/mptcp/blob/mptcp_v0.95/net/mptcp/mptcp_netlink.c#L862-L872

Also, why we are not adding MPTCP_ATTR_FAMILY for subflow create command? I see the bug comment https://github.com/intel/mptcpd/blob/v0.7/src/netlink_pm_mptcp_org.c#L333 but I don't understand it.

Am I missing something here?

[ossama-othman]

The optional attribute comments and handling in netlink_pm_mptcp_org.c were based on the API documentation in <linux/mptcp.h>. There it states that only the MPTCP connection token, address family, local address ID and remote address ID are required. All other attributes are marked as optional (placed between square brackets). However, the implementation you pointed to does appear to indicate that the source and destination address, and destination port are required.

@matttbe: Are the attributes in question actually required in the multipath-tcp.org kernel? Did I misunderstand the API docs?

[ossama-othman]

Also, why we are not adding MPTCP_ATTR_FAMILY for subflow create command? I see the bug comment https://github.com/intel/mptcpd/blob/v0.7/src/netlink_pm_mptcp_org.c#L333 but I don't understand it.

Thanks for bringing this up. It's a long standing piece of code that really should be corrected. Mptcpd doesn't explicitly require plugin authors to pass in a separate address family argument since it is readily available in the sockaddr supplied by the user. I felt that it would be redundant to add a separate address family parameter. Unfortunately, the local/remote address parameters are currently treated as optional by mptcpd, which is why I left the @bug comment. Assuming that the remote address truly is required we can easily address this bug and the comment.

[ossama-othman]

Sorry. I inadvertently closed this issue. Reopened.

[matttbe]

Good catch!

@matttbe: Are the attributes in question actually required in the multipath-tcp.org kernel? Did I misunderstand the API docs?

@ossama-othman indeed the "doc" is not OK in mptcp.h: https://github.com/multipath-tcp/mptcp/blob/mptcp_v0.95/include/uapi/linux/mptcp.h#L90

 *   - MPTCP_CMD_SUB_CREATE: token, family, loc_id, rem_id, [saddr4 | saddr6,
 *                           daddr4 | daddr6, dport [, sport, backup, if_idx]]
 *       Create a new subflow.

We need the token, family, loc_id and rem_id but also the destination address and port, which makes sense when you want to create a new subflow :)
But the source address is not mandatory, loc_id can be used to find the address if not given.

So it should be:

 *   - MPTCP_CMD_SUB_CREATE: token, family, loc_id, rem_id, daddr4 | daddr6,
 *                           dport[, saddr4 | saddr6 sport, backup, if_idx]
 *       Create a new subflow.

I can fix that if nobody already has a patch.