: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. 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 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 | 39 ++++++++++++++++++++++++++++++++------- 2 files changed, 34 insertions(+), 7 deletions(-) --- base-commit: 554ea3649dd15b19202d7e9791310c3bbb910b34 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. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- man/man8/ip-mptcp.8 | 5 +++-- 1 file changed, 3 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 @@ If this is a 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 +unavailable. This does also affect outgoing data when the default packets +scheduler is used. With it, the subflow priority is determined by the +backup/non-backup flag sent to or received from the peer. .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
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