| Message ID | cf358e0e1e2b22a4951b87d90e71d8b07211c1a6.1611783704.git.dcaratti@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [net] net: mptcp: fix length of MP_PRIO sub-option | expand |
Hi Davide, Thanks for adding the MP_PRIO support in the packetdrill, and for this fix. Davide Caratti <dcaratti@redhat.com> 于2021年1月28日周四 上午5:43写道： > > 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' of MP_PRIO changed from 4 to 3 bytes. > > 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 kernel: > $ tcpdump --number -tnnr unpatched.pcap | grep prio > reading from file unpatched.pcap, link-type LINUX_SLL (Linux cooked v1), snapshot length 65535 > 76 IP 10.0.1.1.10006 > 10.0.1.2.53982: Flags [.], ack 301, win 256, options [nop,nop,TS val 600859890 ecr 1213685745,mptcp prio non-backup id 1,mptcp dss ack 13257926699604102366], length 0 > 77 IP 10.0.3.2.40019 > 10.0.1.1.10006: Flags [.], ack 1, win 256, options [nop,nop,TS val 2488720994 ecr 3187547357,mptcp prio non-backup id 1,mptcp dss ack 4016099686801387019], length 0 > > patched kernel: > $ tcpdump --number -tnnr patched.pcap | grep prio > reading from file patched.pcap, link-type LINUX_SLL (Linux cooked v1), snapshot length 65535 > 77 IP 10.0.1.1.10006 > 10.0.1.2.44058: Flags [.], ack 301, win 509, options [nop,nop,TS val 1313569662 ecr 1198445302,mptcp prio non-backup,nop,mptcp dss ack 14169869632263667042], length 0 > 78 IP 10.0.3.2.42645 > 10.0.1.1.10006: Flags [.], ack 1, win 502, options [nop,nop,TS val 4000297703 ecr 2471781012,mptcp prio non-backup,nop,mptcp dss ack 5132559192566117977], length 0 > > Fixes: 40453a5c61f4 ("mptcp: add the incoming MP_PRIO support") > Fixes: 067065422fcd ("mptcp: add the outgoing MP_PRIO support") > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > --- > net/mptcp/options.c | 4 ++-- > net/mptcp/protocol.h | 1 + > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > index 775f0576592e..bb9230dda9fe 100644 > --- a/net/mptcp/options.c > +++ b/net/mptcp/options.c > @@ -283,7 +283,7 @@ static void mptcp_parse_option(const struct sk_buff *skb, > break; > > case MPTCPOPT_MP_PRIO: > - if (opsize != TCPOLEN_MPTCP_PRIO) > + if (opsize != TCPOLEN_MPTCP_PRIO_NOID) > break; > > mp_opt->mp_prio = 1; > @@ -1224,7 +1224,7 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, > subflow->send_mp_prio = 0; > > *ptr++ = mptcp_option(MPTCPOPT_MP_PRIO, > - TCPOLEN_MPTCP_PRIO, > + TCPOLEN_MPTCP_PRIO_NOID, > opts->backup, TCPOPT_NOP); > } > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > index 447ce4631b43..7e18bbfe8f43 100644 > --- a/net/mptcp/protocol.h > +++ b/net/mptcp/protocol.h > @@ -62,6 +62,7 @@ > #define TCPOLEN_MPTCP_PORT_LEN 4 > #define TCPOLEN_MPTCP_RM_ADDR_BASE 4 > #define TCPOLEN_MPTCP_PRIO 4 > +#define TCPOLEN_MPTCP_PRIO_NOID 3 I think it's better to change TCPOLEN_MPTCP_PRIO to 3 directly, rather than adding a new macro TCPOLEN_MPTCP_PRIO_NOID, since the implementation in kernel is for v1 only, no need to consider compatibility with v0. Accordingly, the "Fixes" tags need to be updated too. This tag "Fixes: 40453a5c61f4 ("mptcp: add the incoming MP_PRIO support")" needs to be dropped. -Geliang > #define TCPOLEN_MPTCP_FASTCLOSE 12 > > /* MPTCP MP_JOIN flags */ > -- > 2.29.2 >
hello, thanks a lot for reviewing! On Thu, 2021-01-28 at 11:08 +0800, Geliang Tang wrote: [...] > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > > index 447ce4631b43..7e18bbfe8f43 100644 > > --- a/net/mptcp/protocol.h > > +++ b/net/mptcp/protocol.h > > @@ -62,6 +62,7 @@ > > #define TCPOLEN_MPTCP_PORT_LEN 4 > > #define TCPOLEN_MPTCP_RM_ADDR_BASE 4 > > #define TCPOLEN_MPTCP_PRIO 4 > > +#define TCPOLEN_MPTCP_PRIO_NOID 3 > > I think it's better to change TCPOLEN_MPTCP_PRIO to 3 directly, rather than > adding a new macro TCPOLEN_MPTCP_PRIO_NOID, since the implementation in > kernel is for v1 only, no need to consider compatibility with v0. Ok, but please note that mptcp_option(x) writes always 4 bytes, even when x is 3. Because of that, functions accounting MP_PRIO space in the TCP options (namely, mptcp_established_options_mp_prio() ) will need to use TCPOLEN_MPTCP_PRIO + 1 to account for the trailing NOP. > Accordingly, the "Fixes" tags need to be updated too. This tag > "Fixes: 40453a5c61f4 ("mptcp: add the incoming MP_PRIO support")" > needs to be dropped. > sure, I added it because one of the lines I changed was introduced in 40453a5c61f4; moreover, this change also impacts reception, because the kernel will start discarding MP_PRIO packets with Legth equal to 4. Anyway it's non-realistic that somebody backports it without backporting 40453a5c61f4 and its follow-ups. I'm about to send v2 directly to net- next including both of your suggestions. thanks!
diff --git a/net/mptcp/options.c b/net/mptcp/options.c index 775f0576592e..bb9230dda9fe 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -283,7 +283,7 @@ static void mptcp_parse_option(const struct sk_buff *skb, break; case MPTCPOPT_MP_PRIO: - if (opsize != TCPOLEN_MPTCP_PRIO) + if (opsize != TCPOLEN_MPTCP_PRIO_NOID) break; mp_opt->mp_prio = 1; @@ -1224,7 +1224,7 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, subflow->send_mp_prio = 0; *ptr++ = mptcp_option(MPTCPOPT_MP_PRIO, - TCPOLEN_MPTCP_PRIO, + TCPOLEN_MPTCP_PRIO_NOID, opts->backup, TCPOPT_NOP); } diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index 447ce4631b43..7e18bbfe8f43 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -62,6 +62,7 @@ #define TCPOLEN_MPTCP_PORT_LEN 4 #define TCPOLEN_MPTCP_RM_ADDR_BASE 4 #define TCPOLEN_MPTCP_PRIO 4 +#define TCPOLEN_MPTCP_PRIO_NOID 3 #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' of MP_PRIO changed from 4 to 3 bytes. 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 kernel: $ tcpdump --number -tnnr unpatched.pcap | grep prio reading from file unpatched.pcap, link-type LINUX_SLL (Linux cooked v1), snapshot length 65535 76 IP 10.0.1.1.10006 > 10.0.1.2.53982: Flags [.], ack 301, win 256, options [nop,nop,TS val 600859890 ecr 1213685745,mptcp prio non-backup id 1,mptcp dss ack 13257926699604102366], length 0 77 IP 10.0.3.2.40019 > 10.0.1.1.10006: Flags [.], ack 1, win 256, options [nop,nop,TS val 2488720994 ecr 3187547357,mptcp prio non-backup id 1,mptcp dss ack 4016099686801387019], length 0 patched kernel: $ tcpdump --number -tnnr patched.pcap | grep prio reading from file patched.pcap, link-type LINUX_SLL (Linux cooked v1), snapshot length 65535 77 IP 10.0.1.1.10006 > 10.0.1.2.44058: Flags [.], ack 301, win 509, options [nop,nop,TS val 1313569662 ecr 1198445302,mptcp prio non-backup,nop,mptcp dss ack 14169869632263667042], length 0 78 IP 10.0.3.2.42645 > 10.0.1.1.10006: Flags [.], ack 1, win 502, options [nop,nop,TS val 4000297703 ecr 2471781012,mptcp prio non-backup,nop,mptcp dss ack 5132559192566117977], length 0 Fixes: 40453a5c61f4 ("mptcp: add the incoming MP_PRIO support") Fixes: 067065422fcd ("mptcp: add the outgoing MP_PRIO support") Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- net/mptcp/options.c | 4 ++-- net/mptcp/protocol.h | 1 + 2 files changed, 3 insertions(+), 2 deletions(-)