| Message ID | dfea8e080c5e8bbcbeefa64a4b8d7ff2becea639.1614874814.git.dcaratti@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [net,v2] mptcp: fix length of ADD_ADDR with port suboption | expand |
On Thu, 4 Mar 2021, Davide Caratti wrote: > in current Linux, MPTCP peers advertising endpoints with port numbers use > a sub-option length that wrongly accounts for the trailing TCP NOP. Also, > receivers will only process incoming ADD_ADDR with port having such wrong > sub-option length. Fix this, making ADD_ADDR compliant to RFC8684 §3.4.1. > > this can be verified running tcpdump on the kselftests artifacts: > > unpatched kernel: > [root@bottarga mptcp]# tcpdump -tnnr unpatched.pcap | grep add-addr > reading from file unpatched.pcap, link-type LINUX_SLL (Linux cooked v1), snapshot length 65535 > IP 10.0.1.1.10000 > 10.0.1.2.53078: Flags [.], ack 101, win 509, options [nop,nop,TS val 214459678 ecr 521312851,mptcp add-addr v1 id 1 a00:201:2774:2d88:7436:85c3:17fd:101], length 0 > IP 10.0.1.2.53078 > 10.0.1.1.10000: Flags [.], ack 101, win 502, options [nop,nop,TS val 521312852 ecr 214459678,mptcp add-addr[bad opt]] > > patched kernel: > [root@bottarga mptcp]# tcpdump -tnnr patched.pcap | grep add-addr > reading from file patched.pcap, link-type LINUX_SLL (Linux cooked v1), snapshot length 65535 > IP 10.0.1.1.10000 > 10.0.1.2.38178: Flags [.], ack 101, win 509, options [nop,nop,TS val 3728873902 ecr 2732713192,mptcp add-addr v1 id 1 10.0.2.1:10100 hmac 0xbccdfcbe59292a1f,nop,nop], length 0 > IP 10.0.1.2.38178 > 10.0.1.1.10000: Flags [.], ack 101, win 502, options [nop,nop,TS val 2732713195 ecr 3728873902,mptcp add-addr v1-echo id 1 10.0.2.1:10100,nop,nop], length 0 > > Fixes: 22fb85ffaefb ("mptcp: add port support for ADD_ADDR suboption writing") > CC: stable@vger.kernel.org # 5.11+ > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > --- > net/mptcp/protocol.h | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > index 91827d949766..e21a5bc36cf0 100644 > --- a/net/mptcp/protocol.h > +++ b/net/mptcp/protocol.h > @@ -52,14 +52,15 @@ > #define TCPOLEN_MPTCP_DSS_MAP64 14 > #define TCPOLEN_MPTCP_DSS_CHECKSUM 2 > #define TCPOLEN_MPTCP_ADD_ADDR 16 > -#define TCPOLEN_MPTCP_ADD_ADDR_PORT 20 > +#define TCPOLEN_MPTCP_ADD_ADDR_PORT 18 > #define TCPOLEN_MPTCP_ADD_ADDR_BASE 8 > -#define TCPOLEN_MPTCP_ADD_ADDR_BASE_PORT 12 > +#define TCPOLEN_MPTCP_ADD_ADDR_BASE_PORT 10 > #define TCPOLEN_MPTCP_ADD_ADDR6 28 > -#define TCPOLEN_MPTCP_ADD_ADDR6_PORT 32 > +#define TCPOLEN_MPTCP_ADD_ADDR6_PORT 30 > #define TCPOLEN_MPTCP_ADD_ADDR6_BASE 20 > -#define TCPOLEN_MPTCP_ADD_ADDR6_BASE_PORT 24 > -#define TCPOLEN_MPTCP_PORT_LEN 4 > +#define TCPOLEN_MPTCP_ADD_ADDR6_BASE_PORT 22 > +#define TCPOLEN_MPTCP_PORT_LEN 2 > +#define TCPOLEN_MPTCP_PORT_ALIGN 2 > #define TCPOLEN_MPTCP_RM_ADDR_BASE 4 > #define TCPOLEN_MPTCP_PRIO 3 > #define TCPOLEN_MPTCP_PRIO_ALIGN 4 > @@ -701,8 +702,9 @@ static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port) > len = TCPOLEN_MPTCP_ADD_ADDR6_BASE; > if (!echo) > len += MPTCPOPT_THMAC_LEN; > + /* account for 2 trailing 'nop' options */ > if (port) > - len += TCPOLEN_MPTCP_PORT_LEN; > + len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN; > > return len; > } > -- > 2.29.2 Thanks Davide - fix looks good to me for -net and stable. Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com> -- Mat Martineau Intel
Mat Martineau <mathew.j.martineau@linux.intel.com> 于2021年3月5日周五 上午8:28写道： > > > On Thu, 4 Mar 2021, Davide Caratti wrote: > > > in current Linux, MPTCP peers advertising endpoints with port numbers use > > a sub-option length that wrongly accounts for the trailing TCP NOP. Also, > > receivers will only process incoming ADD_ADDR with port having such wrong > > sub-option length. Fix this, making ADD_ADDR compliant to RFC8684 §3.4.1. > > > > this can be verified running tcpdump on the kselftests artifacts: > > > > unpatched kernel: > > [root@bottarga mptcp]# tcpdump -tnnr unpatched.pcap | grep add-addr > > reading from file unpatched.pcap, link-type LINUX_SLL (Linux cooked v1), snapshot length 65535 > > IP 10.0.1.1.10000 > 10.0.1.2.53078: Flags [.], ack 101, win 509, options [nop,nop,TS val 214459678 ecr 521312851,mptcp add-addr v1 id 1 a00:201:2774:2d88:7436:85c3:17fd:101], length 0 > > IP 10.0.1.2.53078 > 10.0.1.1.10000: Flags [.], ack 101, win 502, options [nop,nop,TS val 521312852 ecr 214459678,mptcp add-addr[bad opt]] > > > > patched kernel: > > [root@bottarga mptcp]# tcpdump -tnnr patched.pcap | grep add-addr > > reading from file patched.pcap, link-type LINUX_SLL (Linux cooked v1), snapshot length 65535 > > IP 10.0.1.1.10000 > 10.0.1.2.38178: Flags [.], ack 101, win 509, options [nop,nop,TS val 3728873902 ecr 2732713192,mptcp add-addr v1 id 1 10.0.2.1:10100 hmac 0xbccdfcbe59292a1f,nop,nop], length 0 > > IP 10.0.1.2.38178 > 10.0.1.1.10000: Flags [.], ack 101, win 502, options [nop,nop,TS val 2732713195 ecr 3728873902,mptcp add-addr v1-echo id 1 10.0.2.1:10100,nop,nop], length 0 > > > > Fixes: 22fb85ffaefb ("mptcp: add port support for ADD_ADDR suboption writing") > > CC: stable@vger.kernel.org # 5.11+ > > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > > --- > > net/mptcp/protocol.h | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > > index 91827d949766..e21a5bc36cf0 100644 > > --- a/net/mptcp/protocol.h > > +++ b/net/mptcp/protocol.h > > @@ -52,14 +52,15 @@ > > #define TCPOLEN_MPTCP_DSS_MAP64 14 > > #define TCPOLEN_MPTCP_DSS_CHECKSUM 2 > > #define TCPOLEN_MPTCP_ADD_ADDR 16 > > -#define TCPOLEN_MPTCP_ADD_ADDR_PORT 20 > > +#define TCPOLEN_MPTCP_ADD_ADDR_PORT 18 > > #define TCPOLEN_MPTCP_ADD_ADDR_BASE 8 > > -#define TCPOLEN_MPTCP_ADD_ADDR_BASE_PORT 12 > > +#define TCPOLEN_MPTCP_ADD_ADDR_BASE_PORT 10 > > #define TCPOLEN_MPTCP_ADD_ADDR6 28 > > -#define TCPOLEN_MPTCP_ADD_ADDR6_PORT 32 > > +#define TCPOLEN_MPTCP_ADD_ADDR6_PORT 30 > > #define TCPOLEN_MPTCP_ADD_ADDR6_BASE 20 > > -#define TCPOLEN_MPTCP_ADD_ADDR6_BASE_PORT 24 > > -#define TCPOLEN_MPTCP_PORT_LEN 4 > > +#define TCPOLEN_MPTCP_ADD_ADDR6_BASE_PORT 22 > > +#define TCPOLEN_MPTCP_PORT_LEN 2 > > +#define TCPOLEN_MPTCP_PORT_ALIGN 2 > > #define TCPOLEN_MPTCP_RM_ADDR_BASE 4 > > #define TCPOLEN_MPTCP_PRIO 3 > > #define TCPOLEN_MPTCP_PRIO_ALIGN 4 > > @@ -701,8 +702,9 @@ static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port) > > len = TCPOLEN_MPTCP_ADD_ADDR6_BASE; > > if (!echo) > > len += MPTCPOPT_THMAC_LEN; > > + /* account for 2 trailing 'nop' options */ > > if (port) > > - len += TCPOLEN_MPTCP_PORT_LEN; > > + len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN; > > > > return len; > > } > > -- > > 2.29.2 > > Thanks Davide - fix looks good to me for -net and stable. > > Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com> > Acked-and-tested-by: Geliang Tang <geliangtang@gmail.com> -Geliang > -- > Mat Martineau > Intel
Hi Davide, Geliang, Mat, On 04/03/2021 17:23, Davide Caratti wrote: > in current Linux, MPTCP peers advertising endpoints with port numbers use > a sub-option length that wrongly accounts for the trailing TCP NOP. Also, > receivers will only process incoming ADD_ADDR with port having such wrong > sub-option length. Fix this, making ADD_ADDR compliant to RFC8684 §3.4.1. > > this can be verified running tcpdump on the kselftests artifacts: Thank you for the patch, review and testing! Sorry for the delay to apply it, I was doing other modifications in the tree to ease CI scripts and because I got a conflict with the export branch, I didn't apply it before :) - 268780afbf25: mptcp: fix length of ADD_ADDR with port sub-option - d16c1995a017: conflict in t/mptcp-add-rm_list-in-mptcp_out_options - Results: b6772f0348c6..ec0b131d1b83 Tests + export are going to be started soon! Cheers, Matt
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index 91827d949766..e21a5bc36cf0 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -52,14 +52,15 @@ #define TCPOLEN_MPTCP_DSS_MAP64 14 #define TCPOLEN_MPTCP_DSS_CHECKSUM 2 #define TCPOLEN_MPTCP_ADD_ADDR 16 -#define TCPOLEN_MPTCP_ADD_ADDR_PORT 20 +#define TCPOLEN_MPTCP_ADD_ADDR_PORT 18 #define TCPOLEN_MPTCP_ADD_ADDR_BASE 8 -#define TCPOLEN_MPTCP_ADD_ADDR_BASE_PORT 12 +#define TCPOLEN_MPTCP_ADD_ADDR_BASE_PORT 10 #define TCPOLEN_MPTCP_ADD_ADDR6 28 -#define TCPOLEN_MPTCP_ADD_ADDR6_PORT 32 +#define TCPOLEN_MPTCP_ADD_ADDR6_PORT 30 #define TCPOLEN_MPTCP_ADD_ADDR6_BASE 20 -#define TCPOLEN_MPTCP_ADD_ADDR6_BASE_PORT 24 -#define TCPOLEN_MPTCP_PORT_LEN 4 +#define TCPOLEN_MPTCP_ADD_ADDR6_BASE_PORT 22 +#define TCPOLEN_MPTCP_PORT_LEN 2 +#define TCPOLEN_MPTCP_PORT_ALIGN 2 #define TCPOLEN_MPTCP_RM_ADDR_BASE 4 #define TCPOLEN_MPTCP_PRIO 3 #define TCPOLEN_MPTCP_PRIO_ALIGN 4 @@ -701,8 +702,9 @@ static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port) len = TCPOLEN_MPTCP_ADD_ADDR6_BASE; if (!echo) len += MPTCPOPT_THMAC_LEN; + /* account for 2 trailing 'nop' options */ if (port) - len += TCPOLEN_MPTCP_PORT_LEN; + len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN; return len; }
in current Linux, MPTCP peers advertising endpoints with port numbers use a sub-option length that wrongly accounts for the trailing TCP NOP. Also, receivers will only process incoming ADD_ADDR with port having such wrong sub-option length. Fix this, making ADD_ADDR compliant to RFC8684 §3.4.1. this can be verified running tcpdump on the kselftests artifacts: unpatched kernel: [root@bottarga mptcp]# tcpdump -tnnr unpatched.pcap | grep add-addr reading from file unpatched.pcap, link-type LINUX_SLL (Linux cooked v1), snapshot length 65535 IP 10.0.1.1.10000 > 10.0.1.2.53078: Flags [.], ack 101, win 509, options [nop,nop,TS val 214459678 ecr 521312851,mptcp add-addr v1 id 1 a00:201:2774:2d88:7436:85c3:17fd:101], length 0 IP 10.0.1.2.53078 > 10.0.1.1.10000: Flags [.], ack 101, win 502, options [nop,nop,TS val 521312852 ecr 214459678,mptcp add-addr[bad opt]] patched kernel: [root@bottarga mptcp]# tcpdump -tnnr patched.pcap | grep add-addr reading from file patched.pcap, link-type LINUX_SLL (Linux cooked v1), snapshot length 65535 IP 10.0.1.1.10000 > 10.0.1.2.38178: Flags [.], ack 101, win 509, options [nop,nop,TS val 3728873902 ecr 2732713192,mptcp add-addr v1 id 1 10.0.2.1:10100 hmac 0xbccdfcbe59292a1f,nop,nop], length 0 IP 10.0.1.2.38178 > 10.0.1.1.10000: Flags [.], ack 101, win 502, options [nop,nop,TS val 2732713195 ecr 3728873902,mptcp add-addr v1-echo id 1 10.0.2.1:10100,nop,nop], length 0 Fixes: 22fb85ffaefb ("mptcp: add port support for ADD_ADDR suboption writing") CC: stable@vger.kernel.org # 5.11+ Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- net/mptcp/protocol.h | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)