| Message ID | 07135f8a644672e1847788341e39f6dd25b1c32c.1611852408.git.dcaratti@redhat.com |
|---|---|
| State | Accepted, archived |
| Commit | 91384be37ad9b9408c9321a76eddba6439cac9da |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [net-next,v3] mptcp: fix length of MP_PRIO suboption | expand |
On Thu, 28 Jan 2021, Davide Caratti 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") > 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(-) > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > index 775f0576592e..bb874c5d663a 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 447ce4631b43..d31edbae8da8 100644 > --- a/net/mptcp/protocol.h > +++ b/net/mptcp/protocol.h > @@ -61,7 +61,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 */ > -- > 2.29.2 Looks good to me, thanks Davide. Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com> -- Mat Martineau Intel
Hi Davide, Mat, On 28/01/2021 18:34, Davide Caratti 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. Thank you for this new version! Just added in our tree, at the bottom, with Mat and my RvB tag. - 91384be37ad9 mptcp: fix length of MP_PRIO suboption Tests + export have been almost a success! Simult Flow selftest with a debug kernel was unstable but it is not new, see: https://github.com/multipath-tcp/mptcp_net-next/issues/137 So feel free to send this patch to netdev when you have the opportunity ;-) It doesn't conflict with other pending patches from Geliang that have been recently sent by Mat. Cheers, Matt
On Fri, 2021-01-29 at 18:08 +0100, Matthieu Baerts wrote: > Hi Davide, Mat, > > On 28/01/2021 18:34, Davide Caratti wrote: [...] > Thank you for this new version! > > Just added in our tree, at the bottom, with Mat and my RvB tag. > > - 91384be37ad9 mptcp: fix length of MP_PRIO suboption > > Tests + export have been almost a success! Simult Flow selftest with a > debug kernel was unstable but it is not new, see: > > https://github.com/multipath-tcp/mptcp_net-next/issues/137 > > So feel free to send this patch to netdev when you have the opportunity ;-) > It doesn't conflict with other pending patches from Geliang that have > been recently sent by Mat. Ok, I'll post it today. thanks!
diff --git a/net/mptcp/options.c b/net/mptcp/options.c index 775f0576592e..bb874c5d663a 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 447ce4631b43..d31edbae8da8 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -61,7 +61,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") 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(-)