8:53 a.m.
As per last public mtg discussion, md5sig will cause TCP option space
exaustion. Without md5sig we can't exhaust the TCP option space.
This series explcitly disable MPTCP when md5sig is set, and cleanup
later option len checks with the assumption that TCP option space exhaustion
is not expected - add a single WARN_ON() for that.
Paolo Abeni (1):
mptcp: move mp_capable initialization at subflow_init_req() start
mptcp: disable on req sk if MD5SIG is enabled
mptcp: warn once if exceeding tcp opt space for dss/mp_capable
mptcp: remove unneeded check in mptcp_established_options_mp()
net/mptcp/subflow.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
2.21.0
8:53 a.m.
New subject: [PATCH 1/4] mptcp: move mp_capable initialization at subflow_init_req() start
This will avoid moving the code in later patch and makes disabling
with md5 easier
Squash-to: "mptcp: Create SUBFLOW socket for incoming connections"
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/subflow.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 869010587df4..9b56fe9a83f2 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -28,6 +28,8 @@ static void subflow_init_req(struct request_sock *req,
memset(&rx_opt.mptcp, 0, sizeof(rx_opt.mptcp));
mptcp_get_options(skb, &rx_opt);
+ subflow_req->mp_capable = 0;
+
if (rx_opt.mptcp.mp_capable && listener->request_mptcp) {
subflow_req->mp_capable = 1;
if (rx_opt.mptcp.version >= listener->request_version)
@@ -35,8 +37,6 @@ static void subflow_init_req(struct request_sock *req,
else
subflow_req->version = rx_opt.mptcp.version;
subflow_req->remote_key = rx_opt.mptcp.sndr_key;
- } else {
- subflow_req->mp_capable = 0;
}
}
--
2.21.0
8:53 a.m.
New subject: [PATCH 2/4] mptcp: disable on req sk if MD5SIG is enabled
Likley we want to update the "mptcp: Create SUBFLOW socket for incoming
connections"
commit message with something alike:
Explicitly disable MPTCP if MD5SIG is enable, to prevent TCP option space
exaustion.
Squash-to: mptcp: Create SUBFLOW socket for incoming connections
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/protocol.c | 8 ++++++++
net/mptcp/subflow.c | 8 ++++++++
2 files changed, 16 insertions(+)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 81b7c21f4d3c..bb297e9dd6c5 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -342,6 +342,14 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr
*uaddr,
if (IS_ERR(ssock))
return PTR_ERR(ssock);
+#ifdef CONFIG_TCP_MD5SIG
+ /* no MPTCP is MD5SIG is enabled on this socket or we may run out of
+ * TCP option space.
+ */
+ if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
+ mptcp_subflow_ctx(ssock->sk)->request_mptcp = 0;
+#endif
+
err = ssock->ops->connect(ssock, uaddr, addr_len, flags);
sock_put(ssock->sk);
return err;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 9b56fe9a83f2..4ab751163965 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -30,6 +30,14 @@ static void subflow_init_req(struct request_sock *req,
subflow_req->mp_capable = 0;
+#ifdef CONFIG_TCP_MD5SIG
+ /* no MPTCP is MD5SIG is enabled on this socket or we may run out of
+ * TCP option space.
+ */
+ if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info))
+ return;
+#endif
+
if (rx_opt.mptcp.mp_capable && listener->request_mptcp) {
subflow_req->mp_capable = 1;
if (rx_opt.mptcp.version >= listener->request_version)
--
2.21.0
8:53 a.m.
New subject: [PATCH 3/4] mptcp: warn once if exceeding tcp opt space for dss/mp_capable
since such option will always fit the tcp header, module bugs.
Note that this patch preserve a bool ret value for
mptcp_established_options_dss(),
even if it now always returns true. That will simplify a bit later patches
for v1 support.
Squash-to: "mptcp: Write MPTCP DSS headers to outgoing data packets"
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/options.c | 71 ++++++++++++++++++++-------------------------
1 file changed, 31 insertions(+), 40 deletions(-)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index fa10086b5b74..cd16baea0666 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -200,7 +200,7 @@ static bool mptcp_established_options_mp(struct sock *sk, unsigned int
*size,
{
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
- if (!subflow->fourth_ack && remaining >= TCPOLEN_MPTCP_MPC_ACK) {
+ if (!subflow->fourth_ack) {
opts->suboptions = OPTION_MPTCP_MPC_ACK;
opts->sndr_key = subflow->local_key;
opts->rcvr_key = subflow->remote_key;
@@ -220,6 +220,7 @@ static bool mptcp_established_options_dss(struct sock *sk, struct
sk_buff *skb,
{
unsigned int dss_size = 0;
struct mptcp_ext *mpext;
+ struct mptcp_sock *msk;
unsigned int ack_size;
mpext = skb ? mptcp_get_ext(skb) : NULL;
@@ -229,15 +230,10 @@ static bool mptcp_established_options_dss(struct sock *sk, struct
sk_buff *skb,
map_size = TCPOLEN_MPTCP_DSS_BASE + TCPOLEN_MPTCP_DSS_MAP64;
- if (map_size <= remaining) {
- remaining -= map_size;
- dss_size = map_size;
- if (mpext)
- opts->ext_copy = *mpext;
- } else {
- opts->ext_copy.use_map = 0;
- WARN_ONCE(1, "MPTCP: Map dropped");
- }
+ remaining -= map_size;
+ dss_size = map_size;
+ if (mpext)
+ opts->ext_copy = *mpext;
}
ack_size = TCPOLEN_MPTCP_DSS_ACK64;
@@ -246,29 +242,19 @@ static bool mptcp_established_options_dss(struct sock *sk, struct
sk_buff *skb,
if (dss_size == 0)
ack_size += TCPOLEN_MPTCP_DSS_BASE;
- if (ack_size <= remaining) {
- struct mptcp_sock *msk;
-
- dss_size += ack_size;
-
- msk = mptcp_sk(mptcp_subflow_ctx(sk)->conn);
- if (msk) {
- opts->ext_copy.data_ack = msk->ack_seq;
- } else {
- mptcp_crypto_key_sha1(mptcp_subflow_ctx(sk)->remote_key,
- NULL, &opts->ext_copy.data_ack);
- opts->ext_copy.data_ack++;
- }
+ dss_size += ack_size;
- opts->ext_copy.ack64 = 1;
- opts->ext_copy.use_ack = 1;
+ msk = mptcp_sk(mptcp_subflow_ctx(sk)->conn);
+ if (msk) {
+ opts->ext_copy.data_ack = msk->ack_seq;
} else {
- opts->ext_copy.use_ack = 0;
- WARN(1, "MPTCP: Ack dropped");
+ mptcp_crypto_key_sha1(mptcp_subflow_ctx(sk)->remote_key,
+ NULL, &opts->ext_copy.data_ack);
+ opts->ext_copy.data_ack++;
}
- if (!dss_size)
- return false;
+ opts->ext_copy.ack64 = 1;
+ opts->ext_copy.use_ack = 1;
*size = ALIGN(dss_size, 4);
return true;
@@ -279,22 +265,27 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff
*skb,
struct mptcp_out_options *opts)
{
unsigned int opt_size = 0;
+ bool ret = true;
if (!mptcp_subflow_ctx(sk)->mp_capable)
return false;
- if (mptcp_established_options_mp(sk, &opt_size, remaining, opts)) {
- *size += opt_size;
- remaining -= opt_size;
- return true;
- } else if (mptcp_established_options_dss(sk, skb, &opt_size, remaining,
- opts)) {
- *size += opt_size;
- remaining -= opt_size;
- return true;
- }
+ if (mptcp_established_options_mp(sk, &opt_size, remaining, opts))
+ ret = true;
+ else if (mptcp_established_options_dss(sk, skb, &opt_size, remaining,
+ opts))
+ ret = true;
- return false;
+ /* we reserved enough space for the above options, and exceeding the
+ * TCP option space would be fatal
+ */
+ if (WARN_ON_ONCE(opt_size > remaining))
+ return false;
+
+ *size += opt_size;
+ remaining -= opt_size;
+
+ return ret;
}
bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
--
2.21.0
8:53 a.m.
New subject: [PATCH 4/4] mptcp: remove unneeded check in mptcp_established_options_mp()
we now have centralized check in mptcp_established_options()
Squash-to: "mptcp: Add handling of outgoing MP_JOIN requests"
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/options.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index e0c6a3b98544..9080a5fed918 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -371,8 +371,7 @@ static bool mptcp_established_options_mp(struct sock *sk, unsigned int
*size,
pr_debug("subflow=%p, local_key=%llu, remote_key=%llu",
subflow, subflow->local_key, subflow->remote_key);
return true;
- } else if (subflow->mp_join && !subflow->fourth_ack &&
- remaining >= TCPOLEN_MPTCP_MPJ_ACK) {
+ } else if (subflow->mp_join && !subflow->fourth_ack) {
opts->suboptions = OPTION_MPTCP_MPJ_ACK;
memcpy(opts->hmac, subflow->hmac, MPTCPOPT_HMAC_LEN);
*size = TCPOLEN_MPTCP_MPJ_ACK;
--
2.21.0
8:58 a.m.
Paolo Abeni <pabeni(a)redhat.com> wrote:
As per last public mtg discussion, md5sig will cause TCP option
space
exaustion. Without md5sig we can't exhaust the TCP option space.
How can you enable MD5SIG on an mptcp socket? I don't think its
possible, as we don't expose the subflow tcp sockets to userspace and
we don't allow setsockotps anymore.
9:06 a.m.
On Mon, 2019-11-11 at 17:53 +0100, Paolo Abeni wrote:
As per last public mtg discussion, md5sig will cause TCP option
space
exaustion. Without md5sig we can't exhaust the TCP option space.
This series explcitly disable MPTCP when md5sig is set, and cleanup
later option len checks with the assumption that TCP option space exhaustion
is not expected - add a single WARN_ON() for that.
Paolo Abeni (1):
mptcp: move mp_capable initialization at subflow_init_req() start
mptcp: disable on req sk if MD5SIG is enabled
mptcp: warn once if exceeding tcp opt space for dss/mp_capable
mptcp: remove unneeded check in mptcp_established_options_mp()
net/mptcp/subflow.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Addenda:
- I dumbly edited this series manually and ended-up with the wrong
prefix on this cover letter [PATCH 1/4] instead of [PATCH 0/4], I'm
sorry.
- the selftests run successful on a build from the patched git
tree@"mptcp: add basic kselftest for mptcp".
- I'm wondering about adding an explicit self-test vs md5sig - easier
way would be checking the MIB counters after the transfer completes
successfully.
/P
9:09 a.m.
On Mon, 2019-11-11 at 17:58 +0100, Florian Westphal wrote:
Paolo Abeni <pabeni(a)redhat.com> wrote:
> As per last public mtg discussion, md5sig will cause TCP option space
> exaustion. Without md5sig we can't exhaust the TCP option space.
How can you enable MD5SIG on an mptcp socket? I don't think its
possible, as we don't expose the subflow tcp sockets to userspace and
we don't allow setsockotps anymore.
I think you can via:
socket(MPTCP)
setsockopt()
connect() // or listen/accept()
AFAICS we still allow setsockopt on the fallback socket and US can
create the MPTCP connection later.
Cheers,
Paolo
1:44 p.m.
New subject: [PATCH 2/4] mptcp: disable on req sk if MD5SIG is enabled
On Mon, 2019-11-11 at 17:53 +0100, Paolo Abeni wrote:
Likley we want to update the "mptcp: Create SUBFLOW socket for
incoming connections"
commit message with something alike:
Explicitly disable MPTCP if MD5SIG is enable, to prevent TCP option space
exaustion.
Squash-to: mptcp: Create SUBFLOW socket for incoming connections
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/protocol.c | 8 ++++++++
net/mptcp/subflow.c | 8 ++++++++
2 files changed, 16 insertions(+)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 81b7c21f4d3c..bb297e9dd6c5 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -342,6 +342,14 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr
*uaddr,
if (IS_ERR(ssock))
return PTR_ERR(ssock);
+#ifdef CONFIG_TCP_MD5SIG
+ /* no MPTCP is MD5SIG is enabled on this socket or we may run out of
+ * TCP option space.
+ */
typo in comment [two places]: "no MPTCP if MD5SIG"
+ if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
+ mptcp_subflow_ctx(ssock->sk)->request_mptcp = 0;
Would it be better to put this check in mptcp_socket_create_get() where
request_mptcp is being initially? Then it only needs to be in one place and
will cause bind() to fail appropriately as well.
Peter.
+#endif
+
err = ssock->ops->connect(ssock, uaddr, addr_len, flags);
sock_put(ssock->sk);
return err;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 9b56fe9a83f2..4ab751163965 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -30,6 +30,14 @@ static void subflow_init_req(struct request_sock *req,
subflow_req->mp_capable = 0;
+#ifdef CONFIG_TCP_MD5SIG
+ /* no MPTCP is MD5SIG is enabled on this socket or we may run out of
+ * TCP option space.
+ */
+ if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info))
+ return;
+#endif
+
if (rx_opt.mptcp.mp_capable && listener->request_mptcp) {
subflow_req->mp_capable = 1;
if (rx_opt.mptcp.version >= listener->request_version)
1:50 p.m.
New subject: [PATCH 3/4] mptcp: warn once if exceeding tcp opt space for dss/mp_capable
Hi Paolo -
On Mon, 2019-11-11 at 17:53 +0100, Paolo Abeni wrote:
since such option will always fit the tcp header, module bugs.
"modulo"
How is it guaranteed that it will always fit, now and in the future?
Peter.
Note that this patch preserve a bool ret value for
mptcp_established_options_dss(),
even if it now always returns true. That will simplify a bit later patches
for v1 support.
Squash-to: "mptcp: Write MPTCP DSS headers to outgoing data packets"
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/options.c | 71 ++++++++++++++++++++-------------------------
1 file changed, 31 insertions(+), 40 deletions(-)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index fa10086b5b74..cd16baea0666 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -200,7 +200,7 @@ static bool mptcp_established_options_mp(struct sock *sk, unsigned
int *size,
{
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
- if (!subflow->fourth_ack && remaining >= TCPOLEN_MPTCP_MPC_ACK) {
+ if (!subflow->fourth_ack) {
opts->suboptions = OPTION_MPTCP_MPC_ACK;
opts->sndr_key = subflow->local_key;
opts->rcvr_key = subflow->remote_key;
@@ -220,6 +220,7 @@ static bool mptcp_established_options_dss(struct sock *sk, struct
sk_buff *skb,
{
unsigned int dss_size = 0;
struct mptcp_ext *mpext;
+ struct mptcp_sock *msk;
unsigned int ack_size;
mpext = skb ? mptcp_get_ext(skb) : NULL;
@@ -229,15 +230,10 @@ static bool mptcp_established_options_dss(struct sock *sk, struct
sk_buff *skb,
map_size = TCPOLEN_MPTCP_DSS_BASE + TCPOLEN_MPTCP_DSS_MAP64;
- if (map_size <= remaining) {
- remaining -= map_size;
- dss_size = map_size;
- if (mpext)
- opts->ext_copy = *mpext;
- } else {
- opts->ext_copy.use_map = 0;
- WARN_ONCE(1, "MPTCP: Map dropped");
- }
+ remaining -= map_size;
+ dss_size = map_size;
+ if (mpext)
+ opts->ext_copy = *mpext;
}
ack_size = TCPOLEN_MPTCP_DSS_ACK64;
@@ -246,29 +242,19 @@ static bool mptcp_established_options_dss(struct sock *sk, struct
sk_buff *skb,
if (dss_size == 0)
ack_size += TCPOLEN_MPTCP_DSS_BASE;
- if (ack_size <= remaining) {
- struct mptcp_sock *msk;
-
- dss_size += ack_size;
-
- msk = mptcp_sk(mptcp_subflow_ctx(sk)->conn);
- if (msk) {
- opts->ext_copy.data_ack = msk->ack_seq;
- } else {
- mptcp_crypto_key_sha1(mptcp_subflow_ctx(sk)->remote_key,
- NULL, &opts->ext_copy.data_ack);
- opts->ext_copy.data_ack++;
- }
+ dss_size += ack_size;
- opts->ext_copy.ack64 = 1;
- opts->ext_copy.use_ack = 1;
+ msk = mptcp_sk(mptcp_subflow_ctx(sk)->conn);
+ if (msk) {
+ opts->ext_copy.data_ack = msk->ack_seq;
} else {
- opts->ext_copy.use_ack = 0;
- WARN(1, "MPTCP: Ack dropped");
+ mptcp_crypto_key_sha1(mptcp_subflow_ctx(sk)->remote_key,
+ NULL, &opts->ext_copy.data_ack);
+ opts->ext_copy.data_ack++;
}
- if (!dss_size)
- return false;
+ opts->ext_copy.ack64 = 1;
+ opts->ext_copy.use_ack = 1;
*size = ALIGN(dss_size, 4);
return true;
@@ -279,22 +265,27 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff
*skb,
struct mptcp_out_options *opts)
{
unsigned int opt_size = 0;
+ bool ret = true;
if (!mptcp_subflow_ctx(sk)->mp_capable)
return false;
- if (mptcp_established_options_mp(sk, &opt_size, remaining, opts)) {
- *size += opt_size;
- remaining -= opt_size;
- return true;
- } else if (mptcp_established_options_dss(sk, skb, &opt_size, remaining,
- opts)) {
- *size += opt_size;
- remaining -= opt_size;
- return true;
- }
+ if (mptcp_established_options_mp(sk, &opt_size, remaining, opts))
+ ret = true;
+ else if (mptcp_established_options_dss(sk, skb, &opt_size, remaining,
+ opts))
+ ret = true;
- return false;
+ /* we reserved enough space for the above options, and exceeding the
+ * TCP option space would be fatal
+ */
+ if (WARN_ON_ONCE(opt_size > remaining))
+ return false;
+
+ *size += opt_size;
+ remaining -= opt_size;
+
+ return ret;
}
bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
12:21 a.m.
New subject: [PATCH 3/4] mptcp: warn once if exceeding tcp opt space for dss/mp_capable
On Mon, 2019-11-11 at 13:50 -0800, Peter Krystad wrote:
Hi Paolo -
On Mon, 2019-11-11 at 17:53 +0100, Paolo Abeni wrote:
> since such option will always fit the tcp header, module bugs.
"modulo"
thnx! Hopefully comment should be dropped when squashing ;)
How is it guaranteed that it will always fit, now and in the future?
TCP option space must accomodate tstamp (12 bytes) and MPTCP options.
AFAICS DSS, MP_CAPABLE and MP_JOIN are mutually esclusive, and the
bigger is DSS with 64 bytes ack/seq and csum with a total of 28 bytes -
which fit the TCP option space.
Additional MPTCP options (ADD/DEL_ADDR/PRIO/FAIL/FASTCLOSE/...) can
indeed run out of space, but that failure is not fatal (do not break
the connection nor causes fallback) and is handled gracefully - no
WARN, just omit the option. Additionally we could increment some MIB
counter.
Thanks,
Paolo
12:34 a.m.
New subject: [PATCH 2/4] mptcp: disable on req sk if MD5SIG is enabled
On Mon, 2019-11-11 at 13:44 -0800, Peter Krystad wrote:
On Mon, 2019-11-11 at 17:53 +0100, Paolo Abeni wrote:
> Likley we want to update the "mptcp: Create SUBFLOW socket for incoming
connections"
> commit message with something alike:
>
> Explicitly disable MPTCP if MD5SIG is enable, to prevent TCP option space
> exaustion.
>
> Squash-to: mptcp: Create SUBFLOW socket for incoming connections
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> net/mptcp/protocol.c | 8 ++++++++
> net/mptcp/subflow.c | 8 ++++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 81b7c21f4d3c..bb297e9dd6c5 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -342,6 +342,14 @@ static int mptcp_stream_connect(struct socket *sock, struct
sockaddr *uaddr,
> if (IS_ERR(ssock))
> return PTR_ERR(ssock);
>
> +#ifdef CONFIG_TCP_MD5SIG
> + /* no MPTCP is MD5SIG is enabled on this socket or we may run out of
> + * TCP option space.
> + */
typo in comment [two places]: "no MPTCP if MD5SIG"
thanx, will fix in v2.
> + if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
> + mptcp_subflow_ctx(ssock->sk)->request_mptcp = 0;
Would it be better to put this check in mptcp_socket_create_get() where
request_mptcp is being initially? Then it only needs to be in one place and
will cause bind() to fail appropriately as well.
In my reply to Florian I reported an incorrect sequence leading to
MPTCP + MD5SIG, the correct one should be:
socket(MPTCP)
bind() // creates fallback socket
setsockopt(MD5SIG) // fallback found, calls setsockopt() on it
connect()
In the above sequence, when mptcp_socket_create_get() is called by
bind(), rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info) is NULL. The
md5sig info will be allocated by the next setsockopt().
Moving the check inside mptcp_socket_create_get() should not help.
Additionally I don't see why bind() should fail here, can you please
explain?
Thank you!
Paolo
10:03 a.m.
New subject: [PATCH 2/4] mptcp: disable on req sk if MD5SIG is enabled
On Tue, 2019-11-12 at 09:34 +0100, Paolo Abeni wrote:
On Mon, 2019-11-11 at 13:44 -0800, Peter Krystad wrote:
> On Mon, 2019-11-11 at 17:53 +0100, Paolo Abeni wrote:
> > Likley we want to update the "mptcp: Create SUBFLOW socket for incoming
connections"
> > commit message with something alike:
> >
> > Explicitly disable MPTCP if MD5SIG is enable, to prevent TCP option space
> > exaustion.
> >
> > Squash-to: mptcp: Create SUBFLOW socket for incoming connections
> > Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> > ---
> > net/mptcp/protocol.c | 8 ++++++++
> > net/mptcp/subflow.c | 8 ++++++++
> > 2 files changed, 16 insertions(+)
> >
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 81b7c21f4d3c..bb297e9dd6c5 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -342,6 +342,14 @@ static int mptcp_stream_connect(struct socket *sock,
struct sockaddr *uaddr,
> > if (IS_ERR(ssock))
> > return PTR_ERR(ssock);
> >
> > +#ifdef CONFIG_TCP_MD5SIG
> > + /* no MPTCP is MD5SIG is enabled on this socket or we may run out of
> > + * TCP option space.
> > + */
>
> typo in comment [two places]: "no MPTCP if MD5SIG"
thanx, will fix in v2.
> > + if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
> > + mptcp_subflow_ctx(ssock->sk)->request_mptcp = 0;
>
> Would it be better to put this check in mptcp_socket_create_get() where
> request_mptcp is being initially? Then it only needs to be in one place and
> will cause bind() to fail appropriately as well.
In my reply to Florian I reported an incorrect sequence leading to
MPTCP + MD5SIG, the correct one should be:
socket(MPTCP)
bind() // creates fallback socket
setsockopt(MD5SIG) // fallback found, calls setsockopt() on it
connect()
In the above sequence, when mptcp_socket_create_get() is called by
bind(), rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info) is NULL. The
md5sig info will be allocated by the next setsockopt().
Moving the check inside mptcp_socket_create_get() should not help.
Additionally I don't see why bind() should fail here, can you please
explain?
I see the sequence now. My poor words for "bind fail", I meant setting
request_mptcp=0 in bind(), which, from the above sequence, isn't right.
Carry on!
Peter.
Thank you!
Paolo
7:48 a.m.
New subject: [PATCH 3/4] mptcp: warn once if exceeding tcp opt space for dss/mp_capable
Hi Paolo,
On 12/11/2019 09:21, Paolo Abeni wrote:
On Mon, 2019-11-11 at 13:50 -0800, Peter Krystad wrote:
> Hi Paolo -
>
> On Mon, 2019-11-11 at 17:53 +0100, Paolo Abeni wrote:
>> since such option will always fit the tcp header, module bugs.
>
> "modulo"
thnx! Hopefully comment should be dropped when squashing ;)
> How is it guaranteed that it will always fit, now and in the future?
TCP option space must accomodate tstamp (12 bytes) and MPTCP options.
AFAICS DSS, MP_CAPABLE and MP_JOIN are mutually esclusive, and the
bigger is DSS with 64 bytes ack/seq and csum with a total of 28 bytes -
which fit the TCP option space.
Additional MPTCP options (ADD/DEL_ADDR/PRIO/FAIL/FASTCLOSE/...) can
indeed run out of space, but that failure is not fatal (do not break
the connection nor causes fallback) and is handled gracefully - no
WARN, just omit the option. Additionally we could increment some MIB
counter.
When you say that the option is omitted, do you mean that the option is
sent in the new (dedicated?) ACK packet or dropped/never sent? :)
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
8:18 a.m.
New subject: [PATCH 3/4] mptcp: warn once if exceeding tcp opt space for dss/mp_capable
On Wed, 2019-11-13 at 16:48 +0100, Matthieu Baerts wrote:
<On 12/11/2019 09:21, Paolo Abeni wrote:
> AFAICS DSS, MP_CAPABLE and MP_JOIN are mutually esclusive, and the
> bigger is DSS with 64 bytes ack/seq and csum with a total of 28 bytes -
> which fit the TCP option space.
>
> Additional MPTCP options (ADD/DEL_ADDR/PRIO/FAIL/FASTCLOSE/...) can
> indeed run out of space, but that failure is not fatal (do not break
> the connection nor causes fallback) and is handled gracefully - no
> WARN, just omit the option. Additionally we could increment some MIB
> counter.
When you say that the option is omitted, do you mean that the option is
sent in the new (dedicated?) ACK packet or dropped/never sent? :)
With the current code, prior to this series, the ADD/DEL_ADDR option
exceeding the TCP option space is dropped/never sent. This series do
not change such behavior.
Inserting a (duplicate) ack to try to fit such option looks doable, but
also out-of-scope for this change.
Cheers,
Paolo
8:35 a.m.
New subject: [PATCH 3/4] mptcp: warn once if exceeding tcp opt space for dss/mp_capable
Hi Paolo,
On 13/11/2019 17:18, Paolo Abeni wrote:
On Wed, 2019-11-13 at 16:48 +0100, Matthieu Baerts wrote:
> <On 12/11/2019 09:21, Paolo Abeni wrote:
>> AFAICS DSS, MP_CAPABLE and MP_JOIN are mutually esclusive, and the
>> bigger is DSS with 64 bytes ack/seq and csum with a total of 28 bytes -
>> which fit the TCP option space.
>>
>> Additional MPTCP options (ADD/DEL_ADDR/PRIO/FAIL/FASTCLOSE/...) can
>> indeed run out of space, but that failure is not fatal (do not break
>> the connection nor causes fallback) and is handled gracefully - no
>> WARN, just omit the option. Additionally we could increment some MIB
>> counter.
>
> When you say that the option is omitted, do you mean that the option is
> sent in the new (dedicated?) ACK packet or dropped/never sent? :)
With the current code, prior to this series, the ADD/DEL_ADDR option
exceeding the TCP option space is dropped/never sent. This series do
not change such behavior.
Indeed, it was already the case. Thank you for your quick reply!
Should we already add the MIB counter? And/or add a TODO in the code?
We are not there yet but missing an ADD_ADDR can have bad consequences.
And it would be unexpected for the userspace to drop it, especially with
MPTCPv1 which should make it reliable :)
Inserting a (duplicate) ack to try to fit such option looks doable,
but
also out-of-scope for this change.
Agreed. We should add this in a TODO list of "things for later" :) (or a
TODO in the code?)
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
9:37 a.m.
New subject: [PATCH 3/4] mptcp: warn once if exceeding tcp opt space for dss/mp_capable
On Wed, 2019-11-13 at 17:35 +0100, Matthieu Baerts wrote:
Should we already add the MIB counter? And/or add a TODO in the code?
I think adding more MIB counter is a later tasks.
We are not there yet but missing an ADD_ADDR can have bad
consequences.
And it would be unexpected for the userspace to drop it, especially with
MPTCPv1 which should make it reliable :)
whoops... I missed that difference between v0 and v1. That opens a full
can of worms, I think/fear:
ADD_ADDR6 echo (30 bytes) can't fit TCP option space easily, without
dropping also the tstamp option. Why does echo includes the IP address
itself ?!? why is the address ID not enough?
bold/stupid question: is still possible change v1 specs in that regard?
(e.g. removing the address from ADD_ADDR echo option)
> Inserting a (duplicate) ack to try to fit such option looks
doable, but
> also out-of-scope for this change.
Here I missed the side effects of duplicate acks on CC. We can't easily
plug there MPTCP specific behavior and if we start sending TCP dup ack
to carry MPTCP info we may end-up needing hook to avoid interference on
CC.
Agreed. We should add this in a TODO list of "things for
later" :) (or a
TODO in the code?)
I think tracking this on the framepad is the better option.
Cheers,
Paolo
10:13 a.m.
New subject: [PATCH 3/4] mptcp: warn once if exceeding tcp opt space for dss/mp_capable
Hi Paolo, Christoph,
On 13/11/2019 18:37, Paolo Abeni wrote:
On Wed, 2019-11-13 at 17:35 +0100, Matthieu Baerts wrote:
> Should we already add the MIB counter? And/or add a TODO in the code?
I think adding more MIB counter is a later tasks.
Should we add a comment in the code is it useless when there are
thousands of lines around? :)
> We are not there yet but missing an ADD_ADDR can have bad
consequences.
> And it would be unexpected for the userspace to drop it, especially with
> MPTCPv1 which should make it reliable :)
whoops... I missed that difference between v0 and v1. That opens a full
can of worms, I think/fear:
ADD_ADDR6 echo (30 bytes) can't fit TCP option space easily, without
dropping also the tstamp option. Why does echo includes the IP address
itself ?!? why is the address ID not enough?
bold/stupid question: is still possible change v1 specs in that regard?
(e.g. removing the address from ADD_ADDR echo option)
Good question. I just added a new topic in the list for the meeting
tomorrow just in case Christoph misses the question :)
>> Inserting a (duplicate) ack to try to fit such option looks
doable, but
>> also out-of-scope for this change.
Here I missed the side effects of duplicate acks on CC. We can't easily
plug there MPTCP specific behavior and if we start sending TCP dup ack
to carry MPTCP info we may end-up needing hook to avoid interference on
CC.
I guess we will have to support that at some points.
> Agreed. We should add this in a TODO list of "things for
later" :) (or a
> TODO in the code?)
I think tracking this on the framepad is the better option.
Thanks! I added a topic for tomorrow's meeting.
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
10:32 a.m.
New subject: [PATCH 3/4] mptcp: warn once if exceeding tcp opt space for dss/mp_capable
On Wed, 2019-11-13 at 19:13 +0100, Matthieu Baerts wrote:
Hi Paolo, Christoph,
On 13/11/2019 18:37, Paolo Abeni wrote:
> On Wed, 2019-11-13 at 17:35 +0100, Matthieu Baerts wrote:
> > Should we already add the MIB counter? And/or add a TODO in the code?
>
> I think adding more MIB counter is a later tasks.
Should we add a comment in the code is it useless when there are
thousands of lines around? :)
At this point, I would like to avoid adding todos inside comments for
the first 2 chunks patches - to avoid additional complex rebases
squashing even later.
Cheers,
Paolo
3:43 p.m.
New subject: [PATCH 3/4] mptcp: warn once if exceeding tcp opt space for dss/mp_capable
On Tue, 2019-11-12 at 09:21 +0100, Paolo Abeni wrote:
On Mon, 2019-11-11 at 13:50 -0800, Peter Krystad wrote:
> Hi Paolo -
>
> On Mon, 2019-11-11 at 17:53 +0100, Paolo Abeni wrote:
> > since such option will always fit the tcp header, module bugs.
>
> "modulo"
thnx! Hopefully comment should be dropped when squashing ;)
> How is it guaranteed that it will always fit, now and in the future?
TCP option space must accomodate tstamp (12 bytes) and MPTCP options.
AFAICS DSS, MP_CAPABLE and MP_JOIN are mutually esclusive, and the
bigger is DSS with 64 bytes ack/seq and csum with a total of 28 bytes -
which fit the TCP option space.
Just FYI, I should have mentioned this earlier, I have observed the mptcp.org
kernel sending 32-bit DSS ack-only in the same header as final MP_CAPABLE ACK
so this statement isn't completely true.
Peter.
Additional MPTCP options (ADD/DEL_ADDR/PRIO/FAIL/FASTCLOSE/...) can
indeed run out of space, but that failure is not fatal (do not break
the connection nor causes fallback) and is handled gracefully - no
WARN, just omit the option. Additionally we could increment some MIB
counter.
Thanks,
Paolo
12:56 a.m.
New subject: [PATCH 3/4] mptcp: warn once if exceeding tcp opt space for dss/mp_capable
On Wed, 2019-11-13 at 15:43 -0800, Peter Krystad wrote:
On Tue, 2019-11-12 at 09:21 +0100, Paolo Abeni wrote:
> AFAICS DSS, MP_CAPABLE and MP_JOIN are mutually esclusive, and the
> bigger is DSS with 64 bytes ack/seq and csum with a total of 28 bytes -
> which fit the TCP option space.
Just FYI, I should have mentioned this earlier, I have observed the mptcp.org
kernel sending 32-bit DSS ack-only in the same header as final MP_CAPABLE ACK
so this statement isn't completely true.
Thank you for the info.
It looks like a .org bug to me ?!? the sender should not send DSS
before receiving the MP_CAPABLE ack, and the client should not send ack
before receving a DSS ?!?
Back to this series, do you prefer I'll revert to the existing checks?
Thanks,
Paolo
10:14 a.m.
New subject: [PATCH 3/4] mptcp: warn once if exceeding tcp opt space for dss/mp_capable
On Mon, 2019-11-11 at 17:53 +0100, Paolo Abeni wrote:
@@ -279,22 +265,27 @@ bool mptcp_established_options(struct sock *sk,
struct sk_buff *skb,
struct mptcp_out_options *opts)
{
unsigned int opt_size = 0;
+ bool ret = true;
^^^^
this is wrong (should be false). I'll send a v3 rebased, including that
change
Cheers,
Paolo
10:36 a.m.
New subject: [PATCH 3/4] mptcp: warn once if exceeding tcp opt space for dss/mp_capable
On Nov 13, 2019, at 9:37 AM, Paolo Abeni <pabeni(a)redhat.com>
wrote:
On Wed, 2019-11-13 at 17:35 +0100, Matthieu Baerts wrote:
> Should we already add the MIB counter? And/or add a TODO in the code?
I think adding more MIB counter is a later tasks.
> We are not there yet but missing an ADD_ADDR can have bad consequences.
> And it would be unexpected for the userspace to drop it, especially with
> MPTCPv1 which should make it reliable :)
whoops... I missed that difference between v0 and v1. That opens a full
can of worms, I think/fear:
ADD_ADDR6 echo (30 bytes) can't fit TCP option space easily, without
dropping also the tstamp option. Why does echo includes the IP address
itself ?!? why is the address ID not enough?
bold/stupid question: is still possible change v1 specs in that regard?
(e.g. removing the address from ADD_ADDR echo option)
To clarify: When the echo-bit is set the HMAC is not present int the option. That means
the ADD_ADDR option is at most 4 + 16 + 2 = 22 bytes on the echo.
However, on the initial ADD_ADDR (the one with echo == 0), the maximum is indeed 30 bytes.
That is only the case when it includes a port as well. And in that scenario I don't
see another solution than to remove the TCP Timestamp option on that packet (it is not
necessary to have the Timestamp option on each packet).
Does that work?
Christopbh
>> Inserting a (duplicate) ack to try to fit such option looks doable, but
>> also out-of-scope for this change.
Here I missed the side effects of duplicate acks on CC. We can't easily
plug there MPTCP specific behavior and if we start sending TCP dup ack
to carry MPTCP info we may end-up needing hook to avoid interference on
CC.
> Agreed. We should add this in a TODO list of "things for later" :) (or a
> TODO in the code?)
I think tracking this on the framepad is the better option.
Cheers,
Paolo
433
days inactive
436
days old
22 comments
5 participants
participants (5)
-
Christoph Paasch -
Florian Westphal -
Matthieu Baerts -
Paolo Abeni -
Peter Krystad