| Message ID | 002345ed30765f5a65a86e12a0b5b8f5bc023ead.1573738696.git.pabeni@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | mptcp: disable mptcp when md5sig is set | expand |
Hi Paolo - On Thu, 2019-11-14 at 19:15 +0100, Paolo Abeni wrote: > since such option will always fit the tcp header, modulo bugs > > Squash-to: "mptcp: Write MPTCP DSS headers to outgoing data packets" > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > v2 -> v3: > - fix mptcp_established_options() return value > v1 -> v2: > - fix typo in commit message > --- > 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..202514088067 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; Or add else { WARN_ON_ONCE() } here, see final comment below. > @@ -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 = false; > > 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; It seems to me, and maybe I am missing something, but all this change adds to existing functionality is showing this warning when mptcp_established_options_mp() fails due to lack of remaining option space. The small change I propose above does the same, yes? I also think the logic here needs to be re-visited, since in both solutions if we want to send MP_CAPABLE and there isn't room we should fallback to TCP and not 'else if' the dss option. Peter. > + *size += opt_size; > + remaining -= opt_size; > + > + return ret; > } > > bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
On Thu, 2019-11-14 at 22:43 -0800, Peter Krystad 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 = false; > > > > 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; > > It seems to me, and maybe I am missing something, but all this change adds to > existing functionality is showing this warning when > mptcp_established_options_mp() fails due to lack of remaining option space. > The small change I propose above does the same, yes? This change has a few goals: - consolidating the check vs TCP option space exhaustion: there is a single warn_on(). Currently we have already one, and we need to add another one in mptcp_established_options_mp(), plus v1 support will require adding at least one more. - documenting that we don't actually expect to exhaust TCP option space with MP_CAPABLE/MP_JOIN or DSS (as we don't send DSS and MP_CAPABLE together) - the assumption that DSS will never exhaust TCP option space allow to simplify the code a bit. yes, alternatively we can add more warn_on(). That will uglify a bit the v1 code. > I also think the logic here needs to be re-visited, since in both solutions if > we want to send MP_CAPABLE and there isn't room we should fallback to TCP and > not 'else if' the dss option. TCP option exhaustion with MP_CAPABLE should only happen in case of bugs, I think WARN_ON() is enough. Cheers, Paolo
On Fri, 2019-11-15 at 13:35 +0100, Paolo Abeni wrote: > On Thu, 2019-11-14 at 22:43 -0800, Peter Krystad 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 = false; > > > > > > 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; > > > > It seems to me, and maybe I am missing something, but all this change adds to > > existing functionality is showing this warning when > > mptcp_established_options_mp() fails due to lack of remaining option space. > > The small change I propose above does the same, yes? > > This change has a few goals: > - consolidating the check vs TCP option space exhaustion: there is a > single warn_on(). Currently we have already one, and we need to add > another one in mptcp_established_options_mp(), plus v1 support will > require adding at least one more. > - documenting that we don't actually expect to exhaust TCP option space > with MP_CAPABLE/MP_JOIN or DSS (as we don't send DSS and MP_CAPABLE > together) > - the assumption that DSS will never exhaust TCP option space allow to > simplify the code a bit. > > yes, alternatively we can add more warn_on(). That will uglify a bit > the v1 code. I guess I'm OK with a single WARN_ON. This patchset LGTM. Peter. > > > I also think the logic here needs to be re-visited, since in both solutions if > > we want to send MP_CAPABLE and there isn't room we should fallback to TCP and > > not 'else if' the dss option. > > TCP option exhaustion with MP_CAPABLE should only happen in case of > bugs, I think WARN_ON() is enough. > > Cheers, > > Paolo >
Hi Paolo, Peter, On 16/11/2019 00:41, Peter Krystad wrote: > On Fri, 2019-11-15 at 13:35 +0100, Paolo Abeni wrote: >> On Thu, 2019-11-14 at 22:43 -0800, Peter Krystad 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 = false; >>>> >>>> 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; >>> >>> It seems to me, and maybe I am missing something, but all this change adds to >>> existing functionality is showing this warning when >>> mptcp_established_options_mp() fails due to lack of remaining option space. >>> The small change I propose above does the same, yes? >> >> This change has a few goals: >> - consolidating the check vs TCP option space exhaustion: there is a >> single warn_on(). Currently we have already one, and we need to add >> another one in mptcp_established_options_mp(), plus v1 support will >> require adding at least one more. >> - documenting that we don't actually expect to exhaust TCP option space >> with MP_CAPABLE/MP_JOIN or DSS (as we don't send DSS and MP_CAPABLE >> together) >> - the assumption that DSS will never exhaust TCP option space allow to >> simplify the code a bit. >> >> yes, alternatively we can add more warn_on(). That will uglify a bit >> the v1 code. > > I guess I'm OK with a single WARN_ON. This patchset LGTM. Thank you for the patches and the reviews! @Paolo: I *think* that the first patches of this series were not based at the end of the series but at the end of part 2 (kselftest introduction). I had to add some extra code and resolve some conflicts with what I hope would be what you had in mind :-) Please see the different conflict resolution and the extra commit here below, especially 7d8b4fe573fb which I guess there are better ways to do but I hope the code is OK. - 3ce88efe9902: "squashed" patch 1/4 (or 1/1 :) ) in "mptcp: Create SUBFLOW socket for incoming connections" - 9c58989db059: "squashed" patch 2/4 in "mptcp: Create SUBFLOW socket for incoming connections" - 946c99f17805: "squashed" patch 3/4 in "mptcp: Write MPTCP DSS headers to outgoing data packets" - 27a10380b320: conflict in t/mptcp-Implement-MPTCP-receive-path - a17134e4116a: conflict in t/mptcp-Add-ADD_ADDR-handling - 7d8b4fe573fb: extra code: take extra established options into account - 34c0738e7ed4: conflict in t/mptcp-Add-handling-of-incoming-MP_JOIN-requests - 63e57f2facbc: conflict in t/mptcp-Add-handling-of-outgoing-MP_JOIN-requests - 0f2c93ad449d: "squashed" patch 4/4 in "mptcp: Add handling of outgoing MP_JOIN requests" - 84cd31e06aaa: "Signed-off-by" + "Co-developed-by" - aad61f16b421: conflict in t/mptcp-increment-MIB-counters-in-a-few-places - 84af4075aa4f..e73eef882a05: result Tests are still all passing! Cheers, Matt
On Wed, 2019-11-20 at 18:27 +0100, Matthieu Baerts wrote: > @Paolo: I *think* that the first patches of this series were not based > at the end of the series but at the end of part 2 (kselftest > introduction). Every patch was based/applied locally just after the related 'squash- to' patch, hoping to minimize the resulting conflicts, I see that did not work very well, sorry. > I had to add some extra code and resolve some conflicts > with what I hope would be what you had in mind :-) > Please see the different conflict resolution and the extra commit here > below, especially 7d8b4fe573fb which I guess there are better ways to do > but I hope the code is OK. > > - 3ce88efe9902: "squashed" patch 1/4 (or 1/1 :) ) in "mptcp: Create > SUBFLOW socket for incoming connections" > - 9c58989db059: "squashed" patch 2/4 in "mptcp: Create SUBFLOW socket > for incoming connections" > - 946c99f17805: "squashed" patch 3/4 in "mptcp: Write MPTCP DSS headers > to outgoing data packets" > - 27a10380b320: conflict in t/mptcp-Implement-MPTCP-receive-path > - a17134e4116a: conflict in t/mptcp-Add-ADD_ADDR-handling > - 7d8b4fe573fb: extra code: take extra established options into account > - 34c0738e7ed4: conflict in > t/mptcp-Add-handling-of-incoming-MP_JOIN-requests > - 63e57f2facbc: conflict in > t/mptcp-Add-handling-of-outgoing-MP_JOIN-requests > - 0f2c93ad449d: "squashed" patch 4/4 in "mptcp: Add handling of outgoing > MP_JOIN requests" > - 84cd31e06aaa: "Signed-off-by" + "Co-developed-by" > - aad61f16b421: conflict in t/mptcp-increment-MIB-counters-in-a-few-places > - 84af4075aa4f..e73eef882a05: result > > Tests are still all passing! Ok, will review and report here. Thank you, Paolo
diff --git a/net/mptcp/options.c b/net/mptcp/options.c index fa10086b5b74..202514088067 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 = false; 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,
since such option will always fit the tcp header, modulo bugs Squash-to: "mptcp: Write MPTCP DSS headers to outgoing data packets" Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- v2 -> v3: - fix mptcp_established_options() return value v1 -> v2: - fix typo in commit message --- net/mptcp/options.c | 71 ++++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 40 deletions(-)