:p
atchew
Login
According to some bug reports we had in the past, setting the MPTCP endpoints might be confusing, and the wrong flags might be set. Adding missing info for the 'dev' parameter, and clarifying 'signal' and 'subflow' flags in the manual is a first step. But because people might surprisingly not (fully) read the manual, printing warnings to stderr if the 'signal' and 'subflow' flags are wrongly used is a second step to prevent frustration, and bug reports. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Matthieu Baerts (NGI0) (4): man: mptcp: document 'dev IFNAME' man: mptcp: clarify 'signal' and 'subflow' flags mptcp: warn if 'signal' and 'subflow' flags are set mptcp: warn if 'signal' or 'subflow' flag is not set ip/ipmptcp.c | 18 ++++++++++++++++++ man/man8/ip-mptcp.8 | 14 ++++++++++++-- 2 files changed, 30 insertions(+), 2 deletions(-) --- base-commit: f9601b10c21145f76c3d46c163bac39515ed2061 change-id: 20240605-mptcp-user-feedback-0676915250cb Best regards, -- Matthieu Baerts (NGI0) <matttbe@kernel.org>
It was missing, while it is a very important option. Indeed, without it, the kernel might not pick the right interface to send packets for additional subflows. Mention that in the man page. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- man/man8/ip-mptcp.8 | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/man/man8/ip-mptcp.8 b/man/man8/ip-mptcp.8 index XXXXXXX..XXXXXXX 100644 --- a/man/man8/ip-mptcp.8 +++ b/man/man8/ip-mptcp.8 @@ -XXX,XX +XXX,XX @@ established MPTCP sockets will be accepted on the specified port, regardless the original listener port accepting the first MPTCP subflow and/or this peer being actually on the client side. +.TP +.IR IFNAME +is the network interface name attached to the endpoint. It is important to +specify this device name linked to the address to make sure the system knows how +to route packets from the specified IP address to the correct network interface. +Without this, it might be required to add IP rules and routes to have the +expected behavior. + .TP .IR ID is a unique numeric identifier for the given endpoint -- 2.43.0
According to some bug reports on the MPTCP project, these options might be a bit confusing for some. Mentioning that the 'signal' flag is typically for a server, and the 'subflow' one is typically for a client should help the user knowing in which context which flag should be picked. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- man/man8/ip-mptcp.8 | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/man/man8/ip-mptcp.8 b/man/man8/ip-mptcp.8 index XXXXXXX..XXXXXXX 100644 --- a/man/man8/ip-mptcp.8 +++ b/man/man8/ip-mptcp.8 @@ -XXX,XX +XXX,XX @@ is a unique numeric identifier for the given endpoint .TP .BR signal The endpoint will be announced/signaled to each peer via an MPTCP ADD_ADDR -sub-option. Upon reception of an ADD_ADDR sub-option, the peer can try to +sub-option. Typically, a server would be responsible for this. Upon reception of +an ADD_ADDR sub-option, the other peer, typically the client side, can try to create additional subflows, see .BR ADD_ADDR_ACCEPTED_NR. @@ -XXX,XX +XXX,XX @@ create additional subflows, see .BR subflow If additional subflow creation is allowed by the MPTCP limits, the MPTCP path manager will try to create an additional subflow using this endpoint -as the source address after the MPTCP connection is established. +as the source address after the MPTCP connection is established. A client would +typically do this. .TP .BR backup -- 2.43.0
Setting both the 'signal' and 'subflow' flags is very likely a mistake from the user, and would bring unexpected behaviours as mentioned in some bug reports on the MPTCP project. It is then important to guide the users by printing a warning on stderr. Users can ignore this message if they have an atypical environment where the initiator of a connection might accept the creation of additional subflows from the other peer. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- ip/ipmptcp.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c index XXXXXXX..XXXXXXX 100644 --- a/ip/ipmptcp.c +++ b/ip/ipmptcp.c @@ -XXX,XX +XXX,XX @@ static int mptcp_parse_opt(int argc, char **argv, struct nlmsghdr *n, int cmd) if (adding && port && !(flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) invarg("flags must have signal when using port", "port"); + if (adding && (flags & MPTCP_PM_ADDR_FLAG_SIGNAL) && + (flags & MPTCP_PM_ADDR_FLAG_SUBFLOW)) + fprintf(stderr, + "Warning: Setting both the 'signal' and 'subflow' flags is usually a mistake:\n" + " 'signal' is used to announce additional addresses, typically for the\n" + " server side, while 'subflow' is used to create new paths, typically\n" + " for the client side. It is not typical to both initiate, and accept\n" + " new connections with the same peer. Continuing as requested.\n"); + if (setting && id_set && port) invarg("port can't be used with id", "port"); -- 2.43.0
Similar to the previous commit, not setting either the 'signal' or 'subflow' flag is usually a mistake from the user. This would bring unexpected behaviours as mentioned in some bug reports on the MPTCP project. It is then important to guide the users by printing a warning on stderr. Users can ignore this message if they have an atypical environment where it is required to reserve an ID for some obscure reasons. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- ip/ipmptcp.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c index XXXXXXX..XXXXXXX 100644 --- a/ip/ipmptcp.c +++ b/ip/ipmptcp.c @@ -XXX,XX +XXX,XX @@ static int mptcp_parse_opt(int argc, char **argv, struct nlmsghdr *n, int cmd) " for the client side. It is not typical to both initiate, and accept\n" " new connections with the same peer. Continuing as requested.\n"); + if (adding && !(flags & (MPTCP_PM_ADDR_FLAG_SIGNAL | + MPTCP_PM_ADDR_FLAG_SUBFLOW))) + fprintf(stderr, + "Warning: Not setting either the 'signal' or 'subflow' flag is usually a mistake:\n" + " 'signal' is used to announce additional addresses, typically for the\n" + " server side, while 'subflow' is used to create new paths, typically\n" + " for the client side. Not setting either of these flags will reserve\n" + " the endpoint ID, but that's it. Continuing as requested.\n"); + if (setting && id_set && port) invarg("port can't be used with id", "port"); -- 2.43.0
According to some bug reports we had in the past, setting the MPTCP endpoints might be confusing, and the wrong flags might be set. This series adds missing info for the 'dev' parameter in the manual, but also clarify a few other parameters. While at it, a better error message is reported when 'id 0' is used to 'add' or 'change' an endpoint. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Changes in v3: - Patch 4/7: Reword (Mat). - Link to v2: https://lore.kernel.org/r/20240723-mptcp-user-feedback-v2-0-59f18975401c@kernel.org Changes in v2: - Drop the patches adding the warnings (Mat). - Add patches 3 to 7. - Link to v1: https://lore.kernel.org/r/20240605-mptcp-user-feedback-v1-0-d2dc3b399643@kernel.org --- Matthieu Baerts (NGI0) (7): man: mptcp: document 'dev IFNAME' man: mptcp: clarify 'signal' and 'subflow' flags man: mptcp: 'port' has to be used with 'signal' man: mptcp: 'backup' flag also affects outgoing data man: mptcp: 'fullmesh' has to be used with 'subflow' man: mptcp: clarify the 'ID' parameter ip: mptcp: 'id 0' is only for 'del' ip/ipmptcp.c | 2 ++ man/man8/ip-mptcp.8 | 44 +++++++++++++++++++++++++++++++++++--------- 2 files changed, 37 insertions(+), 9 deletions(-) --- base-commit: 3e807112fdf3d7b89a8295379dd8474f08a38b4b change-id: 20240605-mptcp-user-feedback-0676915250cb Best regards, -- Matthieu Baerts (NGI0) <matttbe@kernel.org>
It was missing, while it is a very important option. Indeed, without it, the kernel might not pick the right interface to send packets for additional subflows. Mention that in the man page. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- man/man8/ip-mptcp.8 | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/man/man8/ip-mptcp.8 b/man/man8/ip-mptcp.8 index XXXXXXX..XXXXXXX 100644 --- a/man/man8/ip-mptcp.8 +++ b/man/man8/ip-mptcp.8 @@ -XXX,XX +XXX,XX @@ established MPTCP sockets will be accepted on the specified port, regardless the original listener port accepting the first MPTCP subflow and/or this peer being actually on the client side. +.TP +.IR IFNAME +is the network interface name attached to the endpoint. It is important to +specify this device name linked to the address to make sure the system knows how +to route packets from the specified IP address to the correct network interface. +Without this, it might be required to add IP rules and routes to have the +expected behavior. + .TP .IR ID is a unique numeric identifier for the given endpoint -- 2.45.2
According to some bug reports on the MPTCP project, these options might be a bit confusing for some. Mentioning that the 'signal' flag is typically for a server, and the 'subflow' one is typically for a client should help the user knowing in which context which flag should be picked. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- man/man8/ip-mptcp.8 | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/man/man8/ip-mptcp.8 b/man/man8/ip-mptcp.8 index XXXXXXX..XXXXXXX 100644 --- a/man/man8/ip-mptcp.8 +++ b/man/man8/ip-mptcp.8 @@ -XXX,XX +XXX,XX @@ is a unique numeric identifier for the given endpoint .TP .BR signal The endpoint will be announced/signaled to each peer via an MPTCP ADD_ADDR -sub-option. Upon reception of an ADD_ADDR sub-option, the peer can try to +sub-option. Typically, a server would be responsible for this. Upon reception of +an ADD_ADDR sub-option, the other peer, typically the client side, can try to create additional subflows, see .BR ADD_ADDR_ACCEPTED_NR. @@ -XXX,XX +XXX,XX @@ create additional subflows, see .BR subflow If additional subflow creation is allowed by the MPTCP limits, the MPTCP path manager will try to create an additional subflow using this endpoint -as the source address after the MPTCP connection is established. +as the source address after the MPTCP connection is established. A client would +typically do this. .TP .BR backup -- 2.45.2
That's what is enforced by the kernel: the 'port' is used to create a new listening socket on that port, not to create a new subflow from/to that port. It then requires the 'signal' flag. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- man/man8/ip-mptcp.8 | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/man/man8/ip-mptcp.8 b/man/man8/ip-mptcp.8 index XXXXXXX..XXXXXXX 100644 --- a/man/man8/ip-mptcp.8 +++ b/man/man8/ip-mptcp.8 @@ -XXX,XX +XXX,XX @@ is 0. When a port number is specified, incoming MPTCP subflows for already established MPTCP sockets will be accepted on the specified port, regardless the original listener port accepting the first MPTCP subflow and/or -this peer being actually on the client side. +this peer being actually on the client side. This option has to be used in +combination with the +.BR signal +flag. .TP .IR IFNAME -- 2.45.2
That's the behaviour with the default packet scheduler. In some early design, the default scheduler was supposed to take into account only the received backup flags, but it ended up not being the case, and setting the flag would also affect outgoing data. Suggested-by: Mat Martineau <martineau@kernel.org> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Notes: - v2: Reword (Mat) --- man/man8/ip-mptcp.8 | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/man/man8/ip-mptcp.8 b/man/man8/ip-mptcp.8 index XXXXXXX..XXXXXXX 100644 --- a/man/man8/ip-mptcp.8 +++ b/man/man8/ip-mptcp.8 @@ -XXX,XX +XXX,XX @@ typically do this. If this is a .BR subflow endpoint, the subflows created using this endpoint will have the backup -flag set during the connection process. This flag instructs the peer to -only send data on a given subflow when all non-backup subflows are -unavailable. This does not affect outgoing data, where subflow priority -is determined by the backup/non-backup flag received from the peer +flag set during the connection process. This flag instructs the remote +peer to only send data on a given subflow when all non-backup subflows +are unavailable. When using the default packet scheduler with a 'backup' +endpoint, outgoing data from the local peer is also affected: packets +will only be sent from this endpoint when all non-backup subflows are +unavailable. .TP .BR fullmesh -- 2.45.2
'fullmesh' affects the subflow creation, it has to be used with the 'subflow' flag. That's what is enforced on the kernel side. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- man/man8/ip-mptcp.8 | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/man/man8/ip-mptcp.8 b/man/man8/ip-mptcp.8 index XXXXXXX..XXXXXXX 100644 --- a/man/man8/ip-mptcp.8 +++ b/man/man8/ip-mptcp.8 @@ -XXX,XX +XXX,XX @@ this will behave the same as a plain .BR subflow endpoint. When the peer does announce addresses, each received ADD_ADDR sub-option will trigger creation of an additional subflow to generate a -full mesh topology. +full mesh topology. This +.BR fullmesh +flag should always be used in combination with the +.BR subflow +one to be useful, except for the address used by the initial subflow, +where +.BR subflow +is then optional. .TP .BR implicit -- 2.45.2
Explain the range (u8), and the special case for ID 0. The endpoints here are for all the connections, while the ID 0 is a special case per connection, depending on the source address used by the initial subflow. This ID 0 can then not be used for the global endpoints. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- man/man8/ip-mptcp.8 | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/man/man8/ip-mptcp.8 b/man/man8/ip-mptcp.8 index XXXXXXX..XXXXXXX 100644 --- a/man/man8/ip-mptcp.8 +++ b/man/man8/ip-mptcp.8 @@ -XXX,XX +XXX,XX @@ expected behavior. .TP .IR ID -is a unique numeric identifier for the given endpoint +is a unique numeric identifier, between 0 and 255, for the given endpoint. It is +not possible to add endpoints with ID 0, because this special ID is reserved for +the initial subflow. For rules linked to the initial subflow, the path-manager +will look at endpoints matching the same address, and port if set, ignoring the +ID. .TP .BR signal -- 2.45.2
Adding an endpoint with 'id 0' is not allowed. In this case, the kernel will ignore this 'id 0' and set another one. Similarly, because there are no endpoints with this 'id 0', changing an attribute for such endpoint will not be possible. To avoid some confusions, it sounds better to clearly report an error that the ID cannot be 0 in these cases. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- ip/ipmptcp.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c index XXXXXXX..XXXXXXX 100644 --- a/ip/ipmptcp.c +++ b/ip/ipmptcp.c @@ -XXX,XX +XXX,XX @@ static int mptcp_parse_opt(int argc, char **argv, struct nlmsghdr *n, int cmd) invarg("invalid for non-zero id address\n", "ADDRESS"); else if (!id && !addr_set) invarg("address is needed for deleting id 0 address\n", "ID"); + } else if (id_set && !deling && !id) { + invarg("cannot be 0\n", "ID"); } if (adding && port && !(flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) -- 2.45.2