9 a.m.
This series bring in a few fixes for the recent recevmsg refactor.
The patch addressing the reported failures is 2/4, while 1/4 avoids another
issue currently observable with debug enabled (recvmsg() do busy loop on some
corner cases).
Patch 3/4 fixes another possible deadlock issue, currently not reported by
the self-tests
Finally patch 4/4 updates the MIB accounting to the new code.
patches 1-3 should be squashed into "mptcp: Implement MPTCP receive path",
while
patch 4 should be quashed into "mptcp: increment MIB counters in a few places".
Notes:
- the last patch is a bit rough, at it abuses an existing MIB instead
of defining a new one, if there is no agreement about that it can be dropped.
- regardless of that, "mptcp: increment MIB counters in a few places" will
conflict on rebase, due to the changes introduced by the previous patches.
Paolo Abeni (3):
mptcp: clear data_avail flag if rtx queue is empty
mptcp: relax map checking
mptcp: deal better with out-of-sequence mapping
mptcp: account unsupported DSS mapping
net/mptcp/subflow.c | 63 +++++++++++++++++++++------------------------
1 file changed, 30 insertions(+), 33 deletions(-)
--
2.21.0
9 a.m.
New subject: [PATCH 1/4] mptcp: clear data_avail flag if rtx queue is empty
Otherwise we can end-up busy polling in recvmsg()
Squash-to: "mptcp: Implement MPTCP receive path"
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/subflow.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 98c39909a68d..ec0c269d07df 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -430,10 +430,8 @@ bool mptcp_subflow_data_available(struct sock *sk)
}
skb = skb_peek(&sk->sk_receive_queue);
- if (!skb)
- return false;
-
- subflow->data_avail = tcp_sk(sk)->copied_seq < TCP_SKB_CB(skb)->end_seq;
+ subflow->data_avail = skb &&
+ before(tcp_sk(sk)->copied_seq, TCP_SKB_CB(skb)->end_seq);
return subflow->data_avail;
}
--
2.21.0
9 a.m.
New subject: [PATCH 2/4] mptcp: relax map checking
We don't need to have the current skb fully covered by the mapping,
just some bytes at the skb start. The previously enforced condition
was too restrictive and wrong, as it did not take in account that
copied_seq/ssn does not correspond to the skb start.
Squash-to: "mptcp: Implement MPTCP receive path"
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/subflow.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index ec0c269d07df..8e6e0f782f5e 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -192,9 +192,9 @@ static bool validate_mapping(struct sock *ssk, struct sk_buff *skb)
warn_bad_map(subflow, ssn);
return false;
}
- if (unlikely(after(ssn + skb->len, subflow->map_subflow_seq +
- subflow->map_data_len))) {
- /* Mapping does not cover the full skb. Invalid */
+ if (unlikely(!before(ssn, subflow->map_subflow_seq +
+ subflow->map_data_len))) {
+ /* Mapping does covers past subflow data, invalid */
warn_bad_map(subflow, ssn + skb->len);
return false;
}
--
2.21.0
9 a.m.
New subject: [PATCH 3/4] mptcp: deal better with out-of-sequence mapping
map colaescing is an evil early optimization, drop it.
Instead ensure we process correctly any map we receive while
there is a pending, active, one:
- allow replacing only with identic mapping
- explicitly bail if the new map covers only data in future skbs
as we don't support map caching (yet)
Squash-to: "mptcp: Implement MPTCP receive path"
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/subflow.c | 51 ++++++++++++++++++++++-----------------------
1 file changed, 25 insertions(+), 26 deletions(-)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 8e6e0f782f5e..3858187df4ce 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -180,6 +180,19 @@ static void warn_bad_map(struct mptcp_subflow_context *subflow, u32
ssn)
ssn, subflow->map_subflow_seq, subflow->map_data_len);
}
+static bool skb_is_fully_mapped(struct sock *ssk, struct sk_buff *skb)
+{
+ struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+ unsigned skb_consumed;
+
+ skb_consumed = tcp_sk(ssk)->copied_seq - TCP_SKB_CB(skb)->seq;
+ if (WARN_ON_ONCE(skb_consumed >= skb->len))
+ return true;
+
+ return skb->len - skb_consumed <= subflow->map_data_len -
+ mptcp_subflow_get_map_offset(subflow);
+}
+
static bool validate_mapping(struct sock *ssk, struct sk_buff *skb)
{
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
@@ -260,36 +273,22 @@ static enum mapping_status get_mapping_status(struct sock *ssk)
}
if (subflow->map_valid) {
- /* check for in-sequence new map; since the current mapping
- * still valid, we can't replace it yet; instead just
- * coalesce it
- */
- if (subflow->map_seq + subflow->map_data_len ==
- mpext->data_seq) {
- subflow->map_data_len += mpext->data_len;
- if (unlikely(subflow->map_data_len > 1 << 30)) {
- unsigned delta = subflow->map_data_len >> 1;
-
- /* to avoid len overflows, just drop old bits of
- * the current mapping when it becomes really
- * big. delta is larger then the max map len
- * carried by the MPTCP extension, so the map
- * start will still be lower then the SSN
- */
- subflow->map_data_len -= delta;
- subflow->map_subflow_seq += delta;
- subflow->map_seq += delta;
- }
- goto validate_seq;
+ /* Allow replacing only with an identical map */
+ if (subflow->map_seq == map_seq &&
+ subflow->map_subflow_seq == mpext->subflow_seq &&
+ subflow->map_data_len == mpext->data_len) {
+ skb_ext_del(skb, SKB_EXT_MPTCP);
+ return MAPPING_OK;
}
- /* Allow replacing only with an identical map */
- if (subflow->map_seq != mpext->data_seq ||
- subflow->map_subflow_seq != mpext->subflow_seq ||
- subflow->map_data_len != mpext->data_len)
+ /* If this skb data are fully covered by the current mapping,
+ * the new map would need caching, which is not supported
+ */
+ if (skb_is_fully_mapped(ssk, skb))
return MAPPING_INVALID;
- goto validate_seq;
+ /* will validate the next map after consuming the current one */
+ return MAPPING_OK;
}
subflow->map_seq = map_seq;
--
2.21.0
1:22 p.m.
New subject: [PATCH 3/4] mptcp: deal better with out-of-sequence mapping
Hi Paolo,
Thank you for the patches!
On 17/10/2019 11:00, Paolo Abeni wrote:
map colaescing is an evil early optimization, drop it.
Instead ensure we process correctly any map we receive while
there is a pending, active, one:
- allow replacing only with identic mapping
- explicitly bail if the new map covers only data in future skbs
as we don't support map caching (yet)
Squash-to: "mptcp: Implement MPTCP receive path"
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/subflow.c | 51 ++++++++++++++++++++++-----------------------
1 file changed, 25 insertions(+), 26 deletions(-)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 8e6e0f782f5e..3858187df4ce 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -180,6 +180,19 @@ static void warn_bad_map(struct mptcp_subflow_context *subflow, u32
ssn)
ssn, subflow->map_subflow_seq, subflow->map_data_len);
}
+static bool skb_is_fully_mapped(struct sock *ssk, struct sk_buff *skb)
+{
+ struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+ unsigned skb_consumed;
+
+ skb_consumed = tcp_sk(ssk)->copied_seq - TCP_SKB_CB(skb)->seq; > + if
(WARN_ON_ONCE(skb_consumed >= skb->len))
+ return true;
+
+ return skb->len - skb_consumed <= subflow->map_data_len -
+ mptcp_subflow_get_map_offset(subflow);
Do we need to do something special here and/or above to deal with
wraparounds?
By only looking at this chunk of code, it looks like skb_consumed can
have a huge value if one of the two seq numbers have done a wraparound.
Then we might have a warning.
Same below with map_data_len and the offset, no?
But maybe it is not possible in this context.
For the rest, it looks good to me! I can apply them to avoid problems
with the tests (even if for the moment, the tests are not launched
because the compilation with i386 and without IPV6 fail :-) )
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
2:03 p.m.
New subject: [PATCH 3/4] mptcp: deal better with out-of-sequence mapping
Hi,
On Thu, 2019-10-17 at 15:22 +0200, Matthieu Baerts wrote:
On 17/10/2019 11:00, Paolo Abeni wrote:
[...]
> +static bool skb_is_fully_mapped(struct sock *ssk, struct
sk_buff *skb)
> +{
> + struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> + unsigned skb_consumed;
> +
> + skb_consumed = tcp_sk(ssk)->copied_seq - TCP_SKB_CB(skb)->seq;
> + if (WARN_ON_ONCE(skb_consumed >= skb->len))
> + return true;
> +
> + return skb->len - skb_consumed <= subflow->map_data_len -
> + mptcp_subflow_get_map_offset(subflow);
Do we need to do something special here and/or above to deal with
wraparounds?
By only looking at this chunk of code, it looks like skb_consumed can
have a huge value if one of the two seq numbers have done a wraparound.
Differences are ok even in case of wrap-around. AFAIK, we could have
problem only if skb_consumed would be signed and the distance between
'copied_seq' and 'seq' would grow above 1<<31. So basically we
can't ;)
'seq' can be greated than 'copied_seq' just in case of some bug
elsewhere (in TCP and/or in our calls to tcp_read_sock()), and we
should get a warn on the next line, if so.
Then we might have a warning.
Same below with map_data_len and the offset, no?
mptcp_subflow_get_map_offset() can be greater of map_data_len only if
we hit a bug somewhere else. Other places using that algebra currently
don't warn, so I did not add an explicit check here. I would avoid
that, to avoid adding more warns in subflow.c and protocol.c
For the rest, it looks good to me! I can apply them to avoid problems
with the tests (even if for the moment, the tests are not launched
because the compilation with i386 and without IPV6 fail :-) )
:(
/P
6:37 a.m.
New subject: [PATCH 3/4] mptcp: deal better with out-of-sequence mapping
Hi Paolo,
On 17/10/2019 16:03, Paolo Abeni wrote:
Hi,
On Thu, 2019-10-17 at 15:22 +0200, Matthieu Baerts wrote:
> On 17/10/2019 11:00, Paolo Abeni wrote:
[...]
>> +static bool skb_is_fully_mapped(struct sock *ssk, struct sk_buff *skb)
>> +{
>> + struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
>> + unsigned skb_consumed;
>> +
>> + skb_consumed = tcp_sk(ssk)->copied_seq - TCP_SKB_CB(skb)->seq;
>> + if (WARN_ON_ONCE(skb_consumed >= skb->len))
>> + return true;
>> +
>> + return skb->len - skb_consumed <= subflow->map_data_len -
>> + mptcp_subflow_get_map_offset(subflow);
>
> Do we need to do something special here and/or above to deal with
> wraparounds?
>
> By only looking at this chunk of code, it looks like skb_consumed can
> have a huge value if one of the two seq numbers have done a wraparound.
Differences are ok even in case of wrap-around. AFAIK, we could have
problem only if skb_consumed would be signed and the distance between
'copied_seq' and 'seq' would grow above 1<<31. So basically we
can't ;)
'seq' can be greated than 'copied_seq' just in case of some bug
elsewhere (in TCP and/or in our calls to tcp_read_sock()), and we
should get a warn on the next line, if so.
Thank you for this explanation!
> Then we might have a warning.
> Same below with map_data_len and the offset, no?
mptcp_subflow_get_map_offset() can be greater of map_data_len only if
we hit a bug somewhere else. Other places using that algebra currently
don't warn, so I did not add an explicit check here. I would avoid
that, to avoid adding more warns in subflow.c and protocol.c
Sounds good to me! Thank you for this note!
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 a.m.
New subject: [PATCH 4/4] mptcp: account unsupported DSS mapping
abusing existing MIB definition instead of defying a new one.
Note that due to change introduced by the previous patches
"mptcp: increment MIB counters in a few places" will conflict
on rebase.
Squash-to: mptcp: increment MIB counters in a few places
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/subflow.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index ae89e4fc6033..ed29e9e2bbef 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -438,8 +438,10 @@ static enum mapping_status get_mapping_status(struct sock *ssk)
/* If this skb data are fully covered by the current mapping,
* the new map would need caching, which is not supported
*/
- if (skb_is_fully_mapped(ssk, skb))
+ if (skb_is_fully_mapped(ssk, skb)) {
+ MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DSSNOMATCH);
return MAPPING_INVALID;
+ }
/* will validate the next map after consuming the current one */
return MAPPING_OK;
--
2.21.0
10:31 p.m.
On Thu, 17 Oct 2019, Paolo Abeni wrote:
This series bring in a few fixes for the recent recevmsg refactor.
The patch addressing the reported failures is 2/4, while 1/4 avoids another
issue currently observable with debug enabled (recvmsg() do busy loop on some
corner cases).
Patch 3/4 fixes another possible deadlock issue, currently not reported by
the self-tests
Finally patch 4/4 updates the MIB accounting to the new code.
patches 1-3 should be squashed into "mptcp: Implement MPTCP receive path",
while
patch 4 should be quashed into "mptcp: increment MIB counters in a few
places".
Notes:
- the last patch is a bit rough, at it abuses an existing MIB instead
of defining a new one, if there is no agreement about that it can be dropped.
- regardless of that, "mptcp: increment MIB counters in a few places" will
conflict on rebase, due to the changes introduced by the previous patches.
Paolo Abeni (3):
mptcp: clear data_avail flag if rtx queue is empty
mptcp: relax map checking
mptcp: deal better with out-of-sequence mapping
mptcp: account unsupported DSS mapping
net/mptcp/subflow.c | 63 +++++++++++++++++++++------------------------
1 file changed, 30 insertions(+), 33 deletions(-)
This looks good for merging, based on patch review and some testing.
I did see some KASAN problems (not associated with these patches, I
think), and lots of flushing of duplicate recv data when testing with
multipath-tcp.org v0.95. I'll look in to that separately, but they
shouldn't delay these patches.
--
Mat Martineau
Intel
8:16 a.m.
Hi Paolo, Mat,
On 18/10/2019 00:31, Mat Martineau wrote:
On Thu, 17 Oct 2019, Paolo Abeni wrote:
> This series bring in a few fixes for the recent recevmsg refactor.
> The patch addressing the reported failures is 2/4, while 1/4 avoids
> another
> issue currently observable with debug enabled (recvmsg() do busy loop
> on some
> corner cases).
> Patch 3/4 fixes another possible deadlock issue, currently not
> reported by
> the self-tests
> Finally patch 4/4 updates the MIB accounting to the new code.
> patches 1-3 should be squashed into "mptcp: Implement MPTCP receive
> path", while
> patch 4 should be quashed into "mptcp: increment MIB counters in a few
> places".
>
> Notes:
> - the last patch is a bit rough, at it abuses an existing MIB instead
> of defining a new one, if there is no agreement about that it can be
> dropped.
> - regardless of that, "mptcp: increment MIB counters in a few places"
> will
> conflict on rebase, due to the changes introduced by the previous
> patches.
>
>
> Paolo Abeni (3):
> mptcp: clear data_avail flag if rtx queue is empty
> mptcp: relax map checking
> mptcp: deal better with out-of-sequence mapping
> mptcp: account unsupported DSS mapping
>
> net/mptcp/subflow.c | 63 +++++++++++++++++++++------------------------
> 1 file changed, 30 insertions(+), 33 deletions(-)
>
This looks good for merging, based on patch review and some testing.
I did see some KASAN problems (not associated with these patches, I
think), and lots of flushing of duplicate recv data when testing with
multipath-tcp.org v0.95. I'll look in to that separately, but they
shouldn't delay these patches.
Thank you for the patches, the reviews and the tests!
- c337df474b7f: "squashed" patch 1/4 in "mptcp: Implement MPTCP receive
path"
- 8ed52e220550: "squashed" patch 2/4 in "mptcp: Implement MPTCP receive
path"
- 74a0a91c1236: "squashed" patch 3/4 in "mptcp: Implement MPTCP receive
path"
- a1bfb0f562a5: conflict in t/mptcp-increment-MIB-counters-in-a-few-places
- 3031e75932d8: "squashed" patch 4/4 in "mptcp: increment MIB counters
in a few places"
- c2238cc1cb6c: "Signed-off-by" + "Co-developed-by"
- 68a8b453521a..f112b4fbab2a: result
The CI just reported me that the different compilations are OK and the
tests are now passing!
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:22 a.m.
On Thu, 2019-10-17 at 15:31 -0700, Mat Martineau wrote:
This looks good for merging, based on patch review and some testing.
Thank you for testing!
I did see some KASAN problems (not associated with these patches, I
think),
Can you please share the kasan report? I'm not seeing them on top of
current git.
and lots of flushing of duplicate recv data when testing with
multipath-tcp.org v0.95.
Who is flushing ?!? mptcp.org or net-next impl? if it's the latter .org
is likely too aggressive in sending retransmission (or we looks too
'lazy' as we ack at MPTCP level only after recvmsg().
Thank you,
Paolo
9:10 p.m.
On Fri, 18 Oct 2019, Paolo Abeni wrote:
On Thu, 2019-10-17 at 15:31 -0700, Mat Martineau wrote:
> This looks good for merging, based on patch review and some testing.
Thank you for testing!
> I did see some KASAN problems (not associated with these patches, I
> think),
Can you please share the kasan report? I'm not seeing them on top of
current git.
This turns out to be the path manager bug I emailed the list about earlier
today. There are a couple of bugs interacting there when no announce
address is configured: pm->local_family is uninitialized, and
the v4/v6 mptcp_pm_create_subflow functions don't handle the NULL addr.
Peter's working on a patch.
> and lots of flushing of duplicate recv data when testing with
> multipath-tcp.org v0.95.
Who is flushing ?!? mptcp.org or net-next impl? if it's the latter .org
is likely too aggressive in sending retransmission (or we looks too
'lazy' as we ack at MPTCP level only after recvmsg().
net-next. I was able to reproduce the behavior on one set of VMs (but not
with a fresh build from today's export branch) - looks like it was a side
effect of the crashed thread. We weren't sending DSS ACKs after the crash,
so the peer kept resending.
--
Mat Martineau
Intel
981
days inactive
982
days old
11 comments
3 participants
participants (3)
-
Mat Martineau -
Matthieu Baerts -
Paolo Abeni