* [PATCH mptcp-net] mptcp: properly account bulk freed memory
@ 2021-06-22 10:34 Paolo Abeni
2021-06-23 0:23 ` Mat Martineau
0 siblings, 1 reply; 2+ messages in thread
From: Paolo Abeni @ 2021-06-22 10:34 UTC (permalink / raw)
To: mptcp; +Cc: Matthieu Baerts
After commit 879526030c8b ("mptcp: protect the rx path with
the msk socket spinlock") the rmem currently used by a given
msk is really sk_rmem_alloc - rmem_released.
The safety check in mptcp_data_ready() does not take the above
in due account, as a result legit incoming data is keept in
subflow receive queue with no reason, delaying or blocking
MPTCP-level ack generation.
This change addresses the issue updating the check in mptcp_data_ready()
as needed. Additionally add a counter for such exceptional event
- the peer is misbehaving.
Finally update __mptcp_space() to properly compute the window size
given the current memory allocation.
Fixes: 879526030c8b ("mptcp: protect the rx path with the msk socket spinlock")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/211
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
This additionally improves simult_flows.sh stability. @matttbe: could
you please have a spin in your testbad to see if we could close
issues/137, too?
side note: if the VM is slow enough, it could end-up causing failures
even if the protocol would be "perfect", not sure if we should consider
baremetal (or less VM nesting) here.
---
net/mptcp/mib.c | 1 +
net/mptcp/mib.h | 1 +
net/mptcp/protocol.c | 4 +++-
net/mptcp/protocol.h | 7 ++++++-
4 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index 52ea2517e856..ff2cc0e3273d 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -44,6 +44,7 @@ static const struct snmp_mib mptcp_snmp_list[] = {
SNMP_MIB_ITEM("RmSubflow", MPTCP_MIB_RMSUBFLOW),
SNMP_MIB_ITEM("MPPrioTx", MPTCP_MIB_MPPRIOTX),
SNMP_MIB_ITEM("MPPrioRx", MPTCP_MIB_MPPRIORX),
+ SNMP_MIB_ITEM("RcvPruned", MPTCP_MIB_RCVPRUNED),
SNMP_MIB_SENTINEL
};
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index 193466c9b549..0663cb12b448 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -37,6 +37,7 @@ enum linux_mptcp_mib_field {
MPTCP_MIB_RMSUBFLOW, /* Remove a subflow */
MPTCP_MIB_MPPRIOTX, /* Transmit a MP_PRIO */
MPTCP_MIB_MPPRIORX, /* Received a MP_PRIO */
+ MPTCP_MIB_RCVPRUNED, /* Incoming packet dropped due to memory limit */
__MPTCP_MIB_MAX
};
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index ddce5b7bbefd..105b312b1036 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -720,8 +720,10 @@ void mptcp_data_ready(struct sock *sk, struct sock *ssk)
sk_rbuf = ssk_rbuf;
/* over limit? can't append more skbs to msk, Also, no need to wake-up*/
- if (atomic_read(&sk->sk_rmem_alloc) > sk_rbuf)
+ if (atomic_read(&sk->sk_rmem_alloc) - READ_ONCE(msk->rmem_released) > sk_rbuf) {
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RCVPRUNED);
return;
+ }
/* Wake-up the reader only for in-sequence data */
mptcp_data_lock(sk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 426ed80fe72f..60bd78210a36 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -296,9 +296,14 @@ static inline struct mptcp_sock *mptcp_sk(const struct sock *sk)
return (struct mptcp_sock *)sk;
}
+/* the msk socket don't use the backlog, also account for the bulk
+ * free memory
+ */
static inline int __mptcp_space(const struct sock *sk)
{
- return tcp_space(sk) + READ_ONCE(mptcp_sk(sk)->rmem_released);
+ return tcp_win_from_space(sk, READ_ONCE(sk->sk_rcvbuf) -
+ atomic_read(&sk->sk_rmem_alloc) +
+ mptcp_sk(sk)->rmem_released);
}
static inline struct mptcp_data_frag *mptcp_send_head(const struct sock *sk)
--
2.26.3
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH mptcp-net] mptcp: properly account bulk freed memory
2021-06-22 10:34 [PATCH mptcp-net] mptcp: properly account bulk freed memory Paolo Abeni
@ 2021-06-23 0:23 ` Mat Martineau
0 siblings, 0 replies; 2+ messages in thread
From: Mat Martineau @ 2021-06-23 0:23 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp, Matthieu Baerts
On Tue, 22 Jun 2021, Paolo Abeni wrote:
> After commit 879526030c8b ("mptcp: protect the rx path with
> the msk socket spinlock") the rmem currently used by a given
> msk is really sk_rmem_alloc - rmem_released.
>
> The safety check in mptcp_data_ready() does not take the above
> in due account, as a result legit incoming data is keept in
> subflow receive queue with no reason, delaying or blocking
> MPTCP-level ack generation.
>
> This change addresses the issue updating the check in mptcp_data_ready()
> as needed. Additionally add a counter for such exceptional event
> - the peer is misbehaving.
>
> Finally update __mptcp_space() to properly compute the window size
> given the current memory allocation.
>
> Fixes: 879526030c8b ("mptcp: protect the rx path with the msk socket spinlock")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/211
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> This additionally improves simult_flows.sh stability. @matttbe: could
> you please have a spin in your testbad to see if we could close
> issues/137, too?
>
> side note: if the VM is slow enough, it could end-up causing failures
> even if the protocol would be "perfect", not sure if we should consider
> baremetal (or less VM nesting) here.
> ---
> net/mptcp/mib.c | 1 +
> net/mptcp/mib.h | 1 +
> net/mptcp/protocol.c | 4 +++-
> net/mptcp/protocol.h | 7 ++++++-
> 4 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
> index 52ea2517e856..ff2cc0e3273d 100644
> --- a/net/mptcp/mib.c
> +++ b/net/mptcp/mib.c
> @@ -44,6 +44,7 @@ static const struct snmp_mib mptcp_snmp_list[] = {
> SNMP_MIB_ITEM("RmSubflow", MPTCP_MIB_RMSUBFLOW),
> SNMP_MIB_ITEM("MPPrioTx", MPTCP_MIB_MPPRIOTX),
> SNMP_MIB_ITEM("MPPrioRx", MPTCP_MIB_MPPRIORX),
> + SNMP_MIB_ITEM("RcvPruned", MPTCP_MIB_RCVPRUNED),
> SNMP_MIB_SENTINEL
> };
>
> diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
> index 193466c9b549..0663cb12b448 100644
> --- a/net/mptcp/mib.h
> +++ b/net/mptcp/mib.h
> @@ -37,6 +37,7 @@ enum linux_mptcp_mib_field {
> MPTCP_MIB_RMSUBFLOW, /* Remove a subflow */
> MPTCP_MIB_MPPRIOTX, /* Transmit a MP_PRIO */
> MPTCP_MIB_MPPRIORX, /* Received a MP_PRIO */
> + MPTCP_MIB_RCVPRUNED, /* Incoming packet dropped due to memory limit */
> __MPTCP_MIB_MAX
> };
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index ddce5b7bbefd..105b312b1036 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -720,8 +720,10 @@ void mptcp_data_ready(struct sock *sk, struct sock *ssk)
> sk_rbuf = ssk_rbuf;
>
> /* over limit? can't append more skbs to msk, Also, no need to wake-up*/
> - if (atomic_read(&sk->sk_rmem_alloc) > sk_rbuf)
> + if (atomic_read(&sk->sk_rmem_alloc) - READ_ONCE(msk->rmem_released) > sk_rbuf) {
> + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RCVPRUNED);
> return;
> + }
>
> /* Wake-up the reader only for in-sequence data */
> mptcp_data_lock(sk);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 426ed80fe72f..60bd78210a36 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -296,9 +296,14 @@ static inline struct mptcp_sock *mptcp_sk(const struct sock *sk)
> return (struct mptcp_sock *)sk;
> }
>
> +/* the msk socket don't use the backlog, also account for the bulk
> + * free memory
> + */
> static inline int __mptcp_space(const struct sock *sk)
> {
> - return tcp_space(sk) + READ_ONCE(mptcp_sk(sk)->rmem_released);
> + return tcp_win_from_space(sk, READ_ONCE(sk->sk_rcvbuf) -
> + atomic_read(&sk->sk_rmem_alloc) +
> + mptcp_sk(sk)->rmem_released);
Does this last rmem_released need a READ_ONCE() like it had before?
__mptcp_space is called without the MPTCP data lock.
Looks like msk->rmem_released is updated without WRITE_ONCE() in a couple
of places: __mptcp_update_rmem() and __mptcp_recvmsg_mskq(). Best to fix
that too - seems like it's needed.
> }
>
> static inline struct mptcp_data_frag *mptcp_send_head(const struct sock *sk)
> --
> 2.26.3
>
>
>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-06-23 0:23 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-22 10:34 [PATCH mptcp-net] mptcp: properly account bulk freed memory Paolo Abeni
2021-06-23 0:23 ` Mat Martineau
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).