| Message ID | 846cdd41e6ad6ec88ef23fee1552ab39c2f5a3d1.1612184361.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ec99a470c7d5517c97dee6dd7953275a92c63834 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3] mptcp: fix length of MP_PRIO suboption | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: mptcp@lists.01.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 3 this patch: 3 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 22 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 3 this patch: 3 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Mon, Feb 1, 2021 at 2:08 PM Davide Caratti <dcaratti@redhat.com> wrote: > > With version 0 of the protocol it was legal to encode the 'Subflow Id' in > the MP_PRIO suboption, to specify which subflow would change its 'Backup' > flag. This has been removed from v1 specification: thus, according to RFC > 8684 §3.3.8, the resulting 'Length' for MP_PRIO changed from 4 to 3 byte. > > Current Linux generates / parses MP_PRIO according to the old spec, using > 'Length' equal to 4, and hardcoding 1 as 'Subflow Id'; RFC compliance can > improve if we change 'Length' in other to become 3, leaving a 'Nop' after > the MP_PRIO suboption. In this way the kernel will emit and accept *only* > MP_PRIO suboptions that are compliant to version 1 of the MPTCP protocol. > > unpatched 5.11-rc kernel: > [root@bottarga ~]# tcpdump -tnnr unpatched.pcap | grep prio > reading from file unpatched.pcap, link-type LINUX_SLL (Linux cooked v1) > dropped privs to tcpdump > IP 10.0.3.2.48433 > 10.0.1.1.10006: Flags [.], ack 1, win 502, options [nop,nop,TS val 4032325513 ecr 1876514270,mptcp prio non-backup id 1,mptcp dss ack 14084896651682217737], length 0 > > patched 5.11-rc kernel: > [root@bottarga ~]# tcpdump -tnnr patched.pcap | grep prio > reading from file patched.pcap, link-type LINUX_SLL (Linux cooked v1) > dropped privs to tcpdump > IP 10.0.3.2.49735 > 10.0.1.1.10006: Flags [.], ack 1, win 502, options [nop,nop,TS val 1276737699 ecr 2686399734,mptcp prio non-backup,nop,mptcp dss ack 18433038869082491686], length 0 > > Changes since v2: > - when accounting for option space, don't increment 'TCPOLEN_MPTCP_PRIO' > and use 'TCPOLEN_MPTCP_PRIO_ALIGN' instead, thanks to Matthieu Baerts. > Changes since v1: > - refactor patch to avoid using 'TCPOLEN_MPTCP_PRIO' with its old value, > thanks to Geliang Tang. > > Fixes: 067065422fcd ("mptcp: add the outgoing MP_PRIO support") > Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com> > Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net> > Signed-off-by: Davide Caratti <dcaratti@redhat.com> Reviewed-by: Matteo Croce <mcroce@linux.microsoft.com>
Hello: This patch was applied to netdev/net-next.git (refs/heads/master): On Mon, 1 Feb 2021 14:05:26 +0100 you wrote: > With version 0 of the protocol it was legal to encode the 'Subflow Id' in > the MP_PRIO suboption, to specify which subflow would change its 'Backup' > flag. This has been removed from v1 specification: thus, according to RFC > 8684 §3.3.8, the resulting 'Length' for MP_PRIO changed from 4 to 3 byte. > > Current Linux generates / parses MP_PRIO according to the old spec, using > 'Length' equal to 4, and hardcoding 1 as 'Subflow Id'; RFC compliance can > improve if we change 'Length' in other to become 3, leaving a 'Nop' after > the MP_PRIO suboption. In this way the kernel will emit and accept *only* > MP_PRIO suboptions that are compliant to version 1 of the MPTCP protocol. > > [...] Here is the summary with links: - [net-next,v3] mptcp: fix length of MP_PRIO suboption https://git.kernel.org/netdev/net-next/c/ec99a470c7d5 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/net/mptcp/options.c b/net/mptcp/options.c index c9643344a8d7..17ad42c65087 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -699,10 +699,11 @@ static bool mptcp_established_options_mp_prio(struct sock *sk, if (!subflow->send_mp_prio) return false; - if (remaining < TCPOLEN_MPTCP_PRIO) + /* account for the trailing 'nop' option */ + if (remaining < TCPOLEN_MPTCP_PRIO_ALIGN) return false; - *size = TCPOLEN_MPTCP_PRIO; + *size = TCPOLEN_MPTCP_PRIO_ALIGN; opts->suboptions |= OPTION_MPTCP_PRIO; opts->backup = subflow->request_bkup; diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index 1460705aaad0..07ee319f7847 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -60,7 +60,8 @@ #define TCPOLEN_MPTCP_ADD_ADDR6_BASE_PORT 24 #define TCPOLEN_MPTCP_PORT_LEN 4 #define TCPOLEN_MPTCP_RM_ADDR_BASE 4 -#define TCPOLEN_MPTCP_PRIO 4 +#define TCPOLEN_MPTCP_PRIO 3 +#define TCPOLEN_MPTCP_PRIO_ALIGN 4 #define TCPOLEN_MPTCP_FASTCLOSE 12 /* MPTCP MP_JOIN flags */
With version 0 of the protocol it was legal to encode the 'Subflow Id' in the MP_PRIO suboption, to specify which subflow would change its 'Backup' flag. This has been removed from v1 specification: thus, according to RFC 8684 §3.3.8, the resulting 'Length' for MP_PRIO changed from 4 to 3 byte. Current Linux generates / parses MP_PRIO according to the old spec, using 'Length' equal to 4, and hardcoding 1 as 'Subflow Id'; RFC compliance can improve if we change 'Length' in other to become 3, leaving a 'Nop' after the MP_PRIO suboption. In this way the kernel will emit and accept *only* MP_PRIO suboptions that are compliant to version 1 of the MPTCP protocol. unpatched 5.11-rc kernel: [root@bottarga ~]# tcpdump -tnnr unpatched.pcap | grep prio reading from file unpatched.pcap, link-type LINUX_SLL (Linux cooked v1) dropped privs to tcpdump IP 10.0.3.2.48433 > 10.0.1.1.10006: Flags [.], ack 1, win 502, options [nop,nop,TS val 4032325513 ecr 1876514270,mptcp prio non-backup id 1,mptcp dss ack 14084896651682217737], length 0 patched 5.11-rc kernel: [root@bottarga ~]# tcpdump -tnnr patched.pcap | grep prio reading from file patched.pcap, link-type LINUX_SLL (Linux cooked v1) dropped privs to tcpdump IP 10.0.3.2.49735 > 10.0.1.1.10006: Flags [.], ack 1, win 502, options [nop,nop,TS val 1276737699 ecr 2686399734,mptcp prio non-backup,nop,mptcp dss ack 18433038869082491686], length 0 Changes since v2: - when accounting for option space, don't increment 'TCPOLEN_MPTCP_PRIO' and use 'TCPOLEN_MPTCP_PRIO_ALIGN' instead, thanks to Matthieu Baerts. Changes since v1: - refactor patch to avoid using 'TCPOLEN_MPTCP_PRIO' with its old value, thanks to Geliang Tang. Fixes: 067065422fcd ("mptcp: add the outgoing MP_PRIO support") Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com> Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net> Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- net/mptcp/options.c | 5 +++-- net/mptcp/protocol.h | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-)