| Message ID | 20200227083935.11585-1-fw@strlen.de |
|---|---|
| State | Accepted, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | mptcp: re-check dsn before reading from subflow | expand |
On Thu, 27 Feb 2020, Florian Westphal wrote: > mptcp_subflow_data_available() is commonly called via > ssk->sk_data_ready(), in this case the mptcp socket lock > cannot be acquired. > > Therefore, while we can safely discard subflow data that > was already received up to msk->ack_seq, we cannot be sure > that 'subflow->data_avail' will still be valid at the time > userspace wants to read the data -- a previous read on a > different subflow might have carried this data already. > > In that (unlikely) event, msk->ack_seq will have been updated > and will be ahead of the subflow dsn. > > We can check for this condition and skip/resync to the expected > sequence number. > > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > I could also submit this directly for net-next, but this > patch is only needed w. MP_JOIN support. I think this is ok to integrate in the topgit tree. It didn't apply directly due to upstream changes (sorry about the review delay) but the only conflict was straightforward to fix. Thanks, -- Mat Martineau Intel
Hi Florian, Mat, On 13/03/2020 00:12, Mat Martineau wrote: > > On Thu, 27 Feb 2020, Florian Westphal wrote: > >> mptcp_subflow_data_available() is commonly called via >> ssk->sk_data_ready(), in this case the mptcp socket lock >> cannot be acquired. >> >> Therefore, while we can safely discard subflow data that >> was already received up to msk->ack_seq, we cannot be sure >> that 'subflow->data_avail' will still be valid at the time >> userspace wants to read the data -- a previous read on a >> different subflow might have carried this data already. >> >> In that (unlikely) event, msk->ack_seq will have been updated >> and will be ahead of the subflow dsn. >> >> We can check for this condition and skip/resync to the expected >> sequence number. >> >> Signed-off-by: Florian Westphal <fw@strlen.de> >> --- >> I could also submit this directly for net-next, but this >> patch is only needed w. MP_JOIN support. > > I think this is ok to integrate in the topgit tree. Thank you for the patch and the review! I just added this patch at the end of the series, as a new commit (t/mptcp-re-check-dsn-before-reading-from-subflow). Was it what you had in mind? I can easily move it somewhere else if needed. > It didn't apply > directly due to upstream changes (sorry about the review delay) but the > only conflict was straightforward to fix. Indeed, I had one conflict, simple to resolve. Tests are successful and the "export" has been updated. Cheers, Matt
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 53a2b0ba2241..8f2fc72fc5ed 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -121,6 +121,27 @@ static void __mptcp_move_skb(struct mptcp_sock *msk, struct sock *ssk, MPTCP_SKB_CB(skb)->offset = offset; } +/* both sockets must be locked */ +static bool mptcp_subflow_dsn_valid(const struct mptcp_sock *msk, + struct sock *ssk) +{ + struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk); + u64 dsn = mptcp_subflow_get_mapped_dsn(subflow); + + /* revalidate data sequence number. + * + * mptcp_subflow_data_available() is usually called + * without msk lock. Its unlikely (but possible) + * that msk->ack_seq has been advanced since the last + * call found in-sequence data. + */ + if (likely(dsn == msk->ack_seq)) + return true; + + subflow->data_avail = 0; + return mptcp_subflow_data_available(ssk); +} + static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk, struct sock *ssk, unsigned int *bytes) @@ -133,6 +154,11 @@ static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk, bool done = false; int rcvbuf; + if (!mptcp_subflow_dsn_valid(msk, ssk)) { + *bytes = 0; + return false; + } + rcvbuf = max(ssk->sk_rcvbuf, sk->sk_rcvbuf); if (rcvbuf > sk->sk_rcvbuf) sk->sk_rcvbuf = rcvbuf;
mptcp_subflow_data_available() is commonly called via ssk->sk_data_ready(), in this case the mptcp socket lock cannot be acquired. Therefore, while we can safely discard subflow data that was already received up to msk->ack_seq, we cannot be sure that 'subflow->data_avail' will still be valid at the time userspace wants to read the data -- a previous read on a different subflow might have carried this data already. In that (unlikely) event, msk->ack_seq will have been updated and will be ahead of the subflow dsn. We can check for this condition and skip/resync to the expected sequence number. Signed-off-by: Florian Westphal <fw@strlen.de> --- I could also submit this directly for net-next, but this patch is only needed w. MP_JOIN support. net/mptcp/protocol.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)