diff mbox series

[RFC,net,1/2] mptcp: Consistently use READ_ONCE/WRITE_ONCE with msk->ack_seq

Message ID 20200926002408.432576-1-mathew.j.martineau@linux.intel.com
State Accepted, archived
Delegated to: Matthieu Baerts
Headers show
Series [RFC,net,1/2] mptcp: Consistently use READ_ONCE/WRITE_ONCE with msk->ack_seq | expand

Commit Message

Mat Martineau Sept. 26, 2020, 12:24 a.m. UTC 
The msk->ack_seq value is sometimes read without the msk lock held, so
make proper use of READ_ONCE and WRITE_ONCE.

Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---

This does create some merge conflicts with net-next - what's the best
way to handle that?

net/mptcp/options.c  | 4 ++--
 net/mptcp/protocol.c | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)


base-commit: 4e1b469ab0c2c6e6611a0af9f88c7745250dbf64

Comments

Matthieu Baerts Sept. 30, 2020, 10:19 a.m. UTC | #1 
Hi Mat,

On 26/09/2020 02:24, Mat Martineau wrote:
> The msk->ack_seq value is sometimes read without the msk lock held, so
> make proper use of READ_ONCE and WRITE_ONCE.
> 
> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> ---
> 
> This does create some merge conflicts with net-next - what's the best
> way to handle that?

Should we send to netdev ML about how we resolve the conflicts?

I am adding the two patches you sent upstream to the "export" branch and 
resolved the conflicts.

- d80ba0d7cf7f: mptcp: Consistently use READ_ONCE/WRITE_ONCE with 
msk->ack_seq
- 69bd9c6e1c45: mptcp: Handle incoming 32-bit DATA_FIN values

About the conflicts I had:


Patch 1:
- protocol.c: using the version from net-next and modifying 
__mptcp_move_skb() where we have:

   msk->ack_seq += copy_len;


Patch 2:
- subflow.c: we don't use the returned value of 
mptcp_update_rcv_data_fin() in net-next
- protocol.h: we take the new version of mptcp_update_rcv_data_fin() and 
we keep mptcp_destroy_common() unchanged.

I don't know if we need to provide a Git repo or a diff.
Note that on my side, I don't have the result of a merge but a 
modification of the patches for net-next. I don't think that's what they 
want.

Note that we no longer read the output of mptcp_update_rcv_data_fin().


Cheers,
Matt
Matthieu Baerts Sept. 30, 2020, 10:31 a.m. UTC | #2 
Hi Mat,

On 30/09/2020 12:19, Matthieu Baerts wrote:
> Hi Mat,
> 
> On 26/09/2020 02:24, Mat Martineau wrote:
>> The msk->ack_seq value is sometimes read without the msk lock held, so
>> make proper use of READ_ONCE and WRITE_ONCE.
>>
>> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>> ---
>>
>> This does create some merge conflicts with net-next - what's the best
>> way to handle that?
> 
> Should we send to netdev ML about how we resolve the conflicts?

(...)

> Patch 2:
> - subflow.c: we don't use the returned value of 
> mptcp_update_rcv_data_fin() in net-next

I just noticed your commit ef59b1953c26 ("mptcp: Wake up MPTCP worker 
when DATA_FIN found on a TCP FIN packet") is not on net-next yet.

So we do use the returned value if I put your two new patches on top of 
ef59b1953c26!

The "export" branch is going to be updated soon.

Cheers,
Matt
Matthieu Baerts Sept. 30, 2020, 11:03 a.m. UTC | #3 
Hi Mat,

On 30/09/2020 12:31, Matthieu Baerts wrote:
> Hi Mat,
> 
> On 30/09/2020 12:19, Matthieu Baerts wrote:
>> Hi Mat,
>>
>> On 26/09/2020 02:24, Mat Martineau wrote:
>>> The msk->ack_seq value is sometimes read without the msk lock held, so
>>> make proper use of READ_ONCE and WRITE_ONCE.
>>>
>>> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>>> ---
>>>
>>> This does create some merge conflicts with net-next - what's the best
>>> way to handle that?
>>
>> Should we send to netdev ML about how we resolve the conflicts?
> 
> (...)
> 
>> Patch 2:
>> - subflow.c: we don't use the returned value of 
>> mptcp_update_rcv_data_fin() in net-next
> 
> I just noticed your commit ef59b1953c26 ("mptcp: Wake up MPTCP worker 
> when DATA_FIN found on a TCP FIN packet") is not on net-next yet.
> 
> So we do use the returned value if I put your two new patches on top of 
> ef59b1953c26!
> 
> The "export" branch is going to be updated soon.

I also just pushed a new branch: merge_net_net-next_20200930

You can have a look at bac51c27a3c9 ("Merge remote-tracking branch 'net' 
into net-next") for the merge resolution. We have the explanation with 
'git show' but I don't know if we can have the same view on GitHub.

This link is not helpful :)

https://github.com/multipath-tcp/mptcp_net-next/commit/merge_net_net-next_20200930

Cheers,
Matt
Mat Martineau Sept. 30, 2020, 5:33 p.m. UTC | #4 
On Wed, 30 Sep 2020, Matthieu Baerts wrote:

> Hi Mat,
>
> On 30/09/2020 12:31, Matthieu Baerts wrote:
>> Hi Mat,
>> 
>> On 30/09/2020 12:19, Matthieu Baerts wrote:
>>> Hi Mat,
>>> 
>>> On 26/09/2020 02:24, Mat Martineau wrote:
>>>> The msk->ack_seq value is sometimes read without the msk lock held, so
>>>> make proper use of READ_ONCE and WRITE_ONCE.
>>>> 
>>>> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>>>> ---
>>>> 
>>>> This does create some merge conflicts with net-next - what's the best
>>>> way to handle that?
>>> 
>>> Should we send to netdev ML about how we resolve the conflicts?
>> 
>> (...)
>> 
>>> Patch 2:
>>> - subflow.c: we don't use the returned value of 
>>> mptcp_update_rcv_data_fin() in net-next
>> 
>> I just noticed your commit ef59b1953c26 ("mptcp: Wake up MPTCP worker when 
>> DATA_FIN found on a TCP FIN packet") is not on net-next yet.
>> 
>> So we do use the returned value if I put your two new patches on top of 
>> ef59b1953c26!
>> 
>> The "export" branch is going to be updated soon.
>
> I also just pushed a new branch: merge_net_net-next_20200930
>
> You can have a look at bac51c27a3c9 ("Merge remote-tracking branch 'net' into 
> net-next") for the merge resolution. We have the explanation with 'git show' 
> but I don't know if we can have the same view on GitHub.
>
> This link is not helpful :)
>
> https://github.com/multipath-tcp/mptcp_net-next/commit/merge_net_net-next_20200930
>

Hi Matthieu -

I mentioned my recommended merge resolution in the cover letter for the 
latest -net changes:

"""
This does introduce two merge conflicts with net-next, but both have
straightforward resolution. Patch 1 modifies a line that got removed in
net-next so the modification can be dropped when merging. Patch 2 will
require a trivial conflict resolution for a modified function
declaration.
"""

Rather than update __mptcp_move_skb() as part of the merge commit, I think 
it would be better to just drop the change during the merge and send a 
separate patch to net-next to add the WRITE_ONCE() to __mptcp_move_skb().

--
Mat Martineau
Intel
Matthieu Baerts Sept. 30, 2020, 7:35 p.m. UTC | #5 
Hi Mat,

On 30/09/2020 19:33, Mat Martineau wrote:
> On Wed, 30 Sep 2020, Matthieu Baerts wrote:
> 
>> Hi Mat,
>>
>> On 30/09/2020 12:31, Matthieu Baerts wrote:
>>> Hi Mat,
>>>
>>> On 30/09/2020 12:19, Matthieu Baerts wrote:
>>>> Hi Mat,
>>>>
>>>> On 26/09/2020 02:24, Mat Martineau wrote:
>>>>> The msk->ack_seq value is sometimes read without the msk lock held, so
>>>>> make proper use of READ_ONCE and WRITE_ONCE.
>>>>>
>>>>> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>>>>> ---
>>>>>
>>>>> This does create some merge conflicts with net-next - what's the best
>>>>> way to handle that?
>>>>
>>>> Should we send to netdev ML about how we resolve the conflicts?
>>>
>>> (...)
>>>
>>>> Patch 2:
>>>> - subflow.c: we don't use the returned value of 
>>>> mptcp_update_rcv_data_fin() in net-next
>>>
>>> I just noticed your commit ef59b1953c26 ("mptcp: Wake up MPTCP worker 
>>> when DATA_FIN found on a TCP FIN packet") is not on net-next yet.
>>>
>>> So we do use the returned value if I put your two new patches on top 
>>> of ef59b1953c26!
>>>
>>> The "export" branch is going to be updated soon.
>>
>> I also just pushed a new branch: merge_net_net-next_20200930
>>
>> You can have a look at bac51c27a3c9 ("Merge remote-tracking branch 
>> 'net' into net-next") for the merge resolution. We have the 
>> explanation with 'git show' but I don't know if we can have the same 
>> view on GitHub.
>>
>> This link is not helpful :)
>>
>> https://github.com/multipath-tcp/mptcp_net-next/commit/merge_net_net-next_20200930 
>>
>>
> 
> Hi Matthieu -
> 
> I mentioned my recommended merge resolution in the cover letter for the 
> latest -net changes:
> 
> """
> This does introduce two merge conflicts with net-next, but both have
> straightforward resolution. Patch 1 modifies a line that got removed in
> net-next so the modification can be dropped when merging. Patch 2 will
> require a trivial conflict resolution for a modified function
> declaration.
> """

Indeed, my bad, I missed that!

> Rather than update __mptcp_move_skb() as part of the merge commit, I 
> think it would be better to just drop the change during the merge and 
> send a separate patch to net-next to add the WRITE_ONCE() to 
> __mptcp_move_skb().

Indeed, certainly easier for the maintainer. We will have to remember 
fixing this but I should keep track of this particular modification in 
the "export" branch anyway.

Cheers,
Matt
diff mbox series

Patch

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 7fa822b55c34..120ef39fe589 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -518,11 +518,11 @@  static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
 
 	if (subflow->use_64bit_ack) {
 		ack_size = TCPOLEN_MPTCP_DSS_ACK64;
-		opts->ext_copy.data_ack = msk->ack_seq;
+		opts->ext_copy.data_ack = READ_ONCE(msk->ack_seq);
 		opts->ext_copy.ack64 = 1;
 	} else {
 		ack_size = TCPOLEN_MPTCP_DSS_ACK32;
-		opts->ext_copy.data_ack32 = (uint32_t)(msk->ack_seq);
+		opts->ext_copy.data_ack32 = (uint32_t)READ_ONCE(msk->ack_seq);
 		opts->ext_copy.ack64 = 0;
 	}
 	opts->ext_copy.use_ack = 1;
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 365ba96c84b0..5d747c6a610e 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -123,7 +123,7 @@  static void __mptcp_move_skb(struct mptcp_sock *msk, struct sock *ssk,
 
 	skb_ext_reset(skb);
 	skb_orphan(skb);
-	msk->ack_seq += copy_len;
+	WRITE_ONCE(msk->ack_seq, msk->ack_seq + copy_len);
 
 	tail = skb_peek_tail(&sk->sk_receive_queue);
 	if (offset == 0 && tail) {
@@ -261,7 +261,7 @@  static void mptcp_check_data_fin(struct sock *sk)
 	if (mptcp_pending_data_fin(sk, &rcv_data_fin_seq)) {
 		struct mptcp_subflow_context *subflow;
 
-		msk->ack_seq++;
+		WRITE_ONCE(msk->ack_seq, msk->ack_seq + 1);
 		WRITE_ONCE(msk->rcv_data_fin, 0);
 
 		sk->sk_shutdown |= RCV_SHUTDOWN;
@@ -1720,7 +1720,7 @@  struct sock *mptcp_sk_clone(const struct sock *sk,
 		msk->remote_key = mp_opt->sndr_key;
 		mptcp_crypto_key_sha(msk->remote_key, NULL, &ack_seq);
 		ack_seq++;
-		msk->ack_seq = ack_seq;
+		WRITE_ONCE(msk->ack_seq, ack_seq);
 	}
 
 	sock_reset_flag(nsk, SOCK_RCU_FREE);
@@ -2072,7 +2072,7 @@  bool mptcp_finish_join(struct sock *sk)
 	parent_sock = READ_ONCE(parent->sk_socket);
 	if (parent_sock && !sk->sk_socket)
 		mptcp_sock_graft(sk, parent_sock);
-	subflow->map_seq = msk->ack_seq;
+	subflow->map_seq = READ_ONCE(msk->ack_seq);
 	return true;
 }