1:30 p.m.
This series addresses the known pending issue in mptcp_sendmsg,
specifically:
* the crash repored by matt (patch 3/3)
* the possible data corruption on ext allocation failure - the "TODO"" in
sendmsg_frag (patches 1,2/3)
uunfortunately none of the above is actually tested, since I can't
reproduce the issues - still the problems look real.
This is not squashed yet, as it will conflict with most patches, I'm just
trying to collect feedback.
Open points:
The patch 2/3 introduces a per socket cache of a single skb_ext. As per
recent upstream changes related to TCP rx/tx skb cache, that is debatable,
could have subtle performance and memory usage implications.
We could avoid such cache allways allocating the extension for each
sendmsg_frag() call, and than free it if unused. Both options looks
sub-optiomal to me do you have any preference? - I do have some for the
selected impl ;)
The new API exposed by patch 1/3 is open to misusage. e.g. the caller could
allocate a skb_ext, set arbitrary ext->offset[id] values and than call
__skb_ext_add(). That will lead to memory leak or GPF. I did no try to
address the issue, the '__' prefix should guide the users to extra care.
Can we do any better?
(this is a pre-existing topic) the sk_stream_wait_memory() calls in
sk_stream_wait_memory(), waits on the subflow. Should we wait on the mptcp
socket instead? Otherwise should we release the mptcp socket lock, before such
call?
Paolo Abeni (3):
skb: add helpers to allocate ext independently from sk_buff
mptcp: avoid data stream corruption on allocation failure.
mptcp: properly detect skb collapsing
include/linux/skbuff.h | 3 ++
net/core/skbuff.c | 16 ++++++++-
net/mptcp/protocol.c | 79 ++++++++++++++++++++++++++----------------
net/mptcp/protocol.h | 1 +
4 files changed, 68 insertions(+), 31 deletions(-)
--
2.21.0
1:31 p.m.
New subject: [RFC PATCH 1/3] skb: add helpers to allocate ext independently from sk_buff
Currently we can allocate the extension only after the skb,
this change allow the user to do the opposite, will simplify
allocation failure handling from MPTCP.
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
include/linux/skbuff.h | 3 +++
net/core/skbuff.c | 16 +++++++++++++++-
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 165232301b4f..69584b9132a1 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4111,6 +4111,9 @@ struct skb_ext {
char data[0] __aligned(8);
};
+struct skb_ext *skb_ext_alloc(void);
+void *__skb_ext_add(struct sk_buff *skb, enum skb_ext_id id,
+ struct skb_ext *ext);
void *skb_ext_add(struct sk_buff *skb, enum skb_ext_id id);
void __skb_ext_del(struct sk_buff *skb, enum skb_ext_id id);
void __skb_ext_put(struct skb_ext *ext);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 0aba2cd69881..e8ed6d28fba8 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5981,7 +5981,7 @@ static void *skb_ext_get_ptr(struct skb_ext *ext, enum skb_ext_id
id)
return (void *)ext + (ext->offset[id] * SKB_EXT_ALIGN_VALUE);
}
-static struct skb_ext *skb_ext_alloc(void)
+struct skb_ext *skb_ext_alloc(void)
{
struct skb_ext *new = kmem_cache_alloc(skbuff_ext_cache, GFP_ATOMIC);
@@ -6021,6 +6021,20 @@ static struct skb_ext *skb_ext_maybe_cow(struct skb_ext *old,
return new;
}
+void *__skb_ext_add(struct sk_buff *skb, enum skb_ext_id id,
+ struct skb_ext *ext)
+{
+ unsigned int newlen, newoff = SKB_EXT_CHUNKSIZEOF(*ext);
+
+ skb_ext_put(skb);
+ newlen = newoff + skb_ext_type_len[id];
+ ext->chunks = newlen;
+ ext->offset[id] = newoff;
+ skb->extensions = ext;
+ skb->active_extensions |= 1 << id;
+ return skb_ext_get_ptr(ext, id);
+}
+
/**
* skb_ext_add - allocate space for given extension, COW if needed
* @skb: buffer
--
2.21.0
1:31 p.m.
New subject: [RFC PATCH 2/3] mptcp: avoid data stream corruption on allocation failure.
Currently, when if mpext allocation fails, the data stream
is corrupted, as we can add the required DSS nor roll-back
the TCP status.
Allocate a skb_ext before the actuall skb_buff creation/TCP
status update: in case of memory allocation failure we can
bail early, avoiding the above condition.
Additionally, if the ext is unused to to skb collapsing,
cache the ext in the msk for the next send.
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/protocol.c | 64 +++++++++++++++++++++++++-------------------
net/mptcp/protocol.h | 1 +
2 files changed, 38 insertions(+), 27 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 1a432abfb176..38259a8a5df3 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -190,6 +190,14 @@ mptcp_carve_data_frag(const struct mptcp_sock *msk, struct page_frag
*pfrag,
return dfrag;
}
+static bool mptcp_ext_cache_refill(struct mptcp_sock *msk)
+{
+ if (!msk->cached_ext)
+ msk->cached_ext = skb_ext_alloc();
+
+ return msk->cached_ext;
+}
+
static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
struct msghdr *msg, struct mptcp_data_frag *dfrag,
long *timeo, int *pmss_now,
@@ -208,21 +216,22 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
/* use the mptcp page cache so that we can easily move the data
* from one substream to another, but do per subflow memory accounting
- * Note: pfrag is used only !retransmission, but the compiler if
+ * Note: pfrag is used only !retransmission, but the compiler is
* fooled into a warning if we don't init here
*/
pfrag = sk_page_frag(sk);
+ while (!(retransmission || mptcp_page_frag_refill(ssk, pfrag)) ||
+ !mptcp_ext_cache_refill(msk)) {
+ ret = sk_stream_wait_memory(ssk, timeo);
+ if (ret)
+ return ret;
+
+ /* if sk_stream_wait_memory() sleeps snd_una can change
+ * significantly, refresh the rtx queue
+ */
+ mptcp_clean_una(sk);
+ }
if (!retransmission) {
- while (!mptcp_page_frag_refill(ssk, pfrag)) {
- ret = sk_stream_wait_memory(ssk, timeo);
- if (ret)
- return ret;
-
- /* id sk_stream_wait_memory() sleeps snd_una can change
- * significantly, refresh the rtx queue
- */
- mptcp_clean_una(sk);
- }
write_seq = &msk->write_seq;
page = pfrag->page;
} else {
@@ -321,22 +330,19 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
}
skb = tcp_write_queue_tail(ssk);
- mpext = skb_ext_add(skb, SKB_EXT_MPTCP);
- if (mpext) {
- memset(mpext, 0, sizeof(*mpext));
- mpext->data_seq = *write_seq;
- mpext->subflow_seq = mptcp_subflow_ctx(ssk)->rel_write_seq;
- mpext->data_len = ret;
- mpext->use_map = 1;
- mpext->dsn64 = 1;
-
- pr_debug("data_seq=%llu subflow_seq=%u data_len=%u dsn64=%d",
- mpext->data_seq, mpext->subflow_seq, mpext->data_len,
- mpext->dsn64);
- }
- /* TODO: else fallback; allocation can fail, but we can't easily retire
- * skbs from the write_queue, as we need to roll-back TCP status
- */
+ mpext = __skb_ext_add(skb, SKB_EXT_MPTCP, msk->cached_ext);
+ msk->cached_ext = NULL;
+
+ memset(mpext, 0, sizeof(*mpext));
+ mpext->data_seq = *write_seq;
+ mpext->subflow_seq = mptcp_subflow_ctx(ssk)->rel_write_seq;
+ mpext->data_len = ret;
+ mpext->use_map = 1;
+ mpext->dsn64 = 1;
+
+ pr_debug("data_seq=%llu subflow_seq=%u data_len=%u dsn64=%d",
+ mpext->data_seq, mpext->subflow_seq, mpext->data_len,
+ mpext->dsn64);
out:
if (!retransmission)
@@ -686,6 +692,7 @@ static int __mptcp_init_sock(struct sock *sk)
INIT_LIST_HEAD(&msk->conn_list);
INIT_LIST_HEAD(&msk->rtx_queue);
+ msk->cached_ext = NULL;
INIT_WORK(&msk->rtx_work, mptcp_retransmit);
@@ -760,6 +767,9 @@ static void mptcp_close(struct sock *sk, long timeout)
sock_release(mptcp_subflow_tcp_socket(subflow));
}
+ if (msk->cached_ext)
+ __skb_ext_put(msk->cached_ext);
+
__mptcp_clear_xmit(sk);
release_sock(sk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 63ff8bd8a098..b95ea66b2f0e 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -133,6 +133,7 @@ struct mptcp_sock {
u32 token;
unsigned long flags;
u16 dport;
+ struct skb_ext *cached_ext; /* for the next sendmsg */
struct work_struct rtx_work;
struct list_head conn_list;
struct list_head rtx_queue;
--
2.21.0
2:17 p.m.
New subject: [RFC PATCH 2/3] mptcp: avoid data stream corruption on allocation failure.
On Thu, 2019-11-21 at 11:31 +0100, Paolo Abeni wrote:
Currently, when if mpext allocation fails, the data stream
is corrupted, as we can add the required DSS nor roll-back
the TCP status.
Allocate a skb_ext before the actuall skb_buff creation/TCP
status update: in case of memory allocation failure we can
bail early, avoiding the above condition.
Additionally, if the ext is unused to to skb collapsing,
cache the ext in the msk for the next send.
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/protocol.c | 64 +++++++++++++++++++++++++-------------------
net/mptcp/protocol.h | 1 +
2 files changed, 38 insertions(+), 27 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 1a432abfb176..38259a8a5df3 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -190,6 +190,14 @@ mptcp_carve_data_frag(const struct mptcp_sock *msk, struct page_frag
*pfrag,
return dfrag;
}
+static bool mptcp_ext_cache_refill(struct mptcp_sock *msk)
+{
+ if (!msk->cached_ext)
+ msk->cached_ext = skb_ext_alloc();
+
+ return msk->cached_ext;
nit: maybe it's better to make an explicit conversion to bool, like done
elsewhere, e.g.:
return !!msk->cached_ext;
WDYT?
--
davide
3:30 p.m.
New subject: [RFC PATCH 2/3] mptcp: avoid data stream corruption on allocation failure.
On Tue, 2019-12-03 at 12:17 +0100, Davide Caratti wrote:
On Thu, 2019-11-21 at 11:31 +0100, Paolo Abeni wrote:
> Currently, when if mpext allocation fails, the data stream
> is corrupted, as we can add the required DSS nor roll-back
> the TCP status.
>
> Allocate a skb_ext before the actuall skb_buff creation/TCP
> status update: in case of memory allocation failure we can
> bail early, avoiding the above condition.
>
> Additionally, if the ext is unused to to skb collapsing,
> cache the ext in the msk for the next send.
>
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> net/mptcp/protocol.c | 64 +++++++++++++++++++++++++-------------------
> net/mptcp/protocol.h | 1 +
> 2 files changed, 38 insertions(+), 27 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 1a432abfb176..38259a8a5df3 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -190,6 +190,14 @@ mptcp_carve_data_frag(const struct mptcp_sock *msk, struct
page_frag *pfrag,
> return dfrag;
> }
>
> +static bool mptcp_ext_cache_refill(struct mptcp_sock *msk)
> +{
> + if (!msk->cached_ext)
> + msk->cached_ext = skb_ext_alloc();
> +
> + return msk->cached_ext;
nit: maybe it's better to make an explicit conversion to bool, like done
elsewhere, e.g.:
return !!msk->cached_ext;
WDYT?
Good catch, will send a v2 for this patch only
(in reply to the current series)
Note: this code is not changed from RFC to v1.
Cheers,
Paolo
1:31 p.m.
New subject: [RFC PATCH 3/3] mptcp: properly detect skb collapsing
Currently we can misdetect skb collapsing, which will lead
to a kernel warning and crash.
Changing a bit the detection logic for skb collapsing we
can address the issue.
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/protocol.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 38259a8a5df3..6a59ca249f59 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -321,15 +321,24 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
sk->sk_forward_alloc -= frag_truesize;
}
- collapsed = skb == tcp_write_queue_tail(ssk);
+ skb = tcp_write_queue_tail(ssk);
+ if (WARN_ON_ONCE(!skb)) {
+ ret = -EBADFD;
+ goto out;
+ }
+
+ /* if the tail skb extension is still the cached one, collapsing
+ * really happened. Note: we can't check for 'same skb' as the sk_buff
+ * hdr on tail can be transmitted, freed and re-allocated by the
+ * do_tcp_sendpages() call
+ */
+ collapsed = mpext && mpext == skb_ext_find(skb, SKB_EXT_MPTCP);
if (collapsed) {
WARN_ON_ONCE(!can_collapse);
- /* when collapsing mpext always exists */
mpext->data_len += ret;
goto out;
}
- skb = tcp_write_queue_tail(ssk);
mpext = __skb_ext_add(skb, SKB_EXT_MPTCP, msk->cached_ext);
msk->cached_ext = NULL;
--
2.21.0
300
days inactive
312
days old
5 comments
2 participants
participants (2)
-
Davide Caratti -
Paolo Abeni