From: Gang Yan <yangang@kylinos.cn>
This patch fixes an issue where an invalid address is announce as a
signal, the 'add_addr_accepted' is incorrectly added several times
when 'retransmit ADD_ADDR'. So we need to update this variable
when the connection is removed from conn_list by mptcp_worker. So that
the available address can be added in time.
In fact, the 'add_addr_accepted' is only declined when 'RM_ADDR'
by now, so when subflows are getting closed from the other peer,
the new signal is not accepted as well.
We noticed there have exist some problems related to this.I think
this patch effectively resolves them.
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/498
Signed-off-by: Gang Yan <yangang@kylinos.cn>
---
net/mptcp/protocol.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 21bc3586c33e..f99dddca859d 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2569,6 +2569,10 @@ static void __mptcp_close_subflow(struct sock *sk)
continue;
mptcp_close_ssk(sk, ssk, subflow);
+
+ if (READ_ONCE(subflow->remote_id) &&
+ --msk->pm.add_addr_accepted < mptcp_pm_get_add_addr_accept_max(msk))
+ WRITE_ONCE(msk->pm.accept_addr, true);
}
}
--
2.25.1Hi Gang Yan, Thank you for this patch! I have a few suggestions below. Do you mind sending the next version(s) to the MPTCP list only, no need to include the netdev ML and the net maintainers for the moment I think. On 11/12/2024 10:03, Gang Yan wrote: > From: Gang Yan <yangang@kylinos.cn> > > This patch fixes an issue where an invalid address is announce as a > signal, the 'add_addr_accepted' is incorrectly added several times > when 'retransmit ADD_ADDR'. So we need to update this variable > when the connection is removed from conn_list by mptcp_worker. So that > the available address can be added in time. > > In fact, the 'add_addr_accepted' is only declined when 'RM_ADDR' > by now, so when subflows are getting closed from the other peer, Does it mean that in this case, the counter will be decreased twice: when the RM_ADDR is received, and when the subflow is closed, no? I guess no because you hooked this in a different path, right? > the new signal is not accepted as well. > > We noticed there have exist some problems related to this.I think > this patch effectively resolves them. Please add new test cases for these problems in the MPTCP selftests: to better understand what is being fixed, and to avoid regressions later. > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/498 > Signed-off-by: Gang Yan <yangang@kylinos.cn> > --- > net/mptcp/protocol.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 21bc3586c33e..f99dddca859d 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -2569,6 +2569,10 @@ static void __mptcp_close_subflow(struct sock *sk) > continue; > > mptcp_close_ssk(sk, ssk, subflow); > + > + if (READ_ONCE(subflow->remote_id) && > + --msk->pm.add_addr_accepted < mptcp_pm_get_add_addr_accept_max(msk)) > + WRITE_ONCE(msk->pm.accept_addr, true); Mmh, I don't think it can be that simple: potentially, an accepted ADD_ADDR can trigger multiple subflows (i.e. when the fullmesh flag is used). In this case, the counter has been incremented once, not once for each created subflow. So before decrementing the counter, it should then be needed to check if no other subflows connected to the same remote ID are still alive. I think it is better not to decrement this counter in "unusual situations" -- the situation before this patch -- than wrongly decrementing it, and ended up with an underflow. Another thing is that subflows might have not been created upon the reception of an ADD_ADDR: typically if you take the view of a server, the subflows have been initiated by the client, and not because the server got an ADD_ADDR. If I'm not mistaken, the counter would be decremented here as well. We could restrict this by checking "subflow->request_join" I suppose. Still, I'm wondering if that's covering all cases, and if we should not track ADD_ADDR that are received: https://github.com/multipath-tcp/mptcp_net-next/issues/496 (which is more complex) Also, because this counter is specific to the in-kernel PM, I think it would be cleaner f its manipulation is done in pm_netlink.c. pw-bot: cr Cheers, Matt -- Sponsored by the NGI0 Core fund.
On Mon, Dec 16, 2024 at 5:00:03PM +0800, Gang Yan wrote: Hi Matthieu, Happy to receive your reply! >On Thu, Dec 12, 2024 at 07:23:02PM +0100, Matthieu Baerts wrote: > Hi Gang Yan, > > Thank you for this patch! > > I have a few suggestions below. Do you mind sending the next version(s) > to the MPTCP list only, no need to include the netdev ML and the net > maintainers for the moment I think. > OK, sorry for this. I'll be careful next time. > On 11/12/2024 10:03, Gang Yan wrote: > > From: Gang Yan <yangang@kylinos.cn> > > > > This patch fixes an issue where an invalid address is announce as a > > signal, the 'add_addr_accepted' is incorrectly added several times > > when 'retransmit ADD_ADDR'. So we need to update this variable > > when the connection is removed from conn_list by mptcp_worker. So that > > the available address can be added in time. > > > > In fact, the 'add_addr_accepted' is only declined when 'RM_ADDR' > > by now, so when subflows are getting closed from the other peer, > > Does it mean that in this case, the counter will be decreased twice: > when the RM_ADDR is received, and when the subflow is closed, no? > > I guess no because you hooked this in a different path, right? Yes, the counter will not be declined twice. The RM_ADDR will invoke mptcp_pm_nl_rm_addr_or_subflow() in pm_netlink.c, which will ignore the subflow of state '(TCPF_FIN_WAIT1 | TCPF_FIN_WAIT2 | TCPF_CLOSING | TCPF_CLOSE)' , the subflow of these states will turn into 'TCPF_CLOSE' and finally closed in __mptcp_close_subflow() because of mptcp_worker. > > the new signal is not accepted as well. > > > > We noticed there have exist some problems related to this.I think > > this patch effectively resolves them. > > Please add new test cases for these problems in the MPTCP selftests: to > better understand what is being fixed, and to avoid regressions later. > Actually, I noticed this issue due to "./mptcp_join.sh 28" running failed in my machine, but passed in docker-virtme, and it can be reproduced very easily. That's mainly because the status of invalid_addr's tcp connection is different. It is 'TCPF_SYNC_SENT' in virtme, but 'TCPF_CLOSE' in another. So the differences of states will cause a different return value of lookup_subflow_by_daddr() in mptcp_pm_nl_add_addr_received() which will add the counter twice when retransmit 'ADD_ADDR'. Further, I'll add a new test for covering the specific scene. > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/498 > > Signed-off-by: Gang Yan <yangang@kylinos.cn> > > --- > > net/mptcp/protocol.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > > index 21bc3586c33e..f99dddca859d 100644 > > --- a/net/mptcp/protocol.c > > +++ b/net/mptcp/protocol.c > > @@ -2569,6 +2569,10 @@ static void __mptcp_close_subflow(struct sock *sk) > > continue; > > > > mptcp_close_ssk(sk, ssk, subflow); > > + > > + if (READ_ONCE(subflow->remote_id) && > > + --msk->pm.add_addr_accepted < mptcp_pm_get_add_addr_accept_max(msk)) > > + WRITE_ONCE(msk->pm.accept_addr, true); > Mmh, I don't think it can be that simple: potentially, an accepted > ADD_ADDR can trigger multiple subflows (i.e. when the fullmesh flag is > used). In this case, the counter has been incremented once, not once for > each created subflow. So before decrementing the counter, it should then > be needed to check if no other subflows connected to the same remote ID > are still alive. Thanks for your advice, sorry for not considering the 'fullmesh' situations, the check seems necessary. > I think it is better not to decrement this counter in "unusual > situations" -- the situation before this patch -- than wrongly > decrementing it, and ended up with an underflow. As the description beyond, the invalid address has caused not accepting an available address due to this counter, I think we should ensure the available address can be added at least. And an restrict condition can solve the underflow problem. > Another thing is that subflows might have not been created upon the > reception of an ADD_ADDR: typically if you take the view of a server, > the subflows have been initiated by the client, and not because the > server got an ADD_ADDR. If I'm not mistaken, the counter would be > decremented here as well. We could restrict this by checking > "subflow->request_join" I suppose. Still, I'm wondering if that's > covering all cases, But does the subflow in server side have 'remote id'? if doesn't, the counter will not be declined in server side. For covering all cases, I suppose put the patch code in '__mptcp_close_ssk', do you think so? > and if we should not track ADD_ADDR that are received: > > https://github.com/multipath-tcp/mptcp_net-next/issues/496 > > (which is more complex) For the problem 1 and 3, I noticed there exists 'anno_list' in server side, and we have timer for retransmitting 'add_addr', do you think we can modify the two modules to fix these problems rather than add a new list. For the 'add_addr_accepted', I think using a restrict condition to update the counter is enough. > Also, because this counter is specific to the in-kernel PM, I think it > would be cleaner f its manipulation is done in pm_netlink.c. > > pw-bot: cr > > Cheers, > Matt > -- > Sponsored by the NGI0 Core fund. > Thanks for the advices again. Cheers, Gang
Hi Gang,
On 16/12/2024 10:05, Gang Yan wrote:
> On Mon, Dec 16, 2024 at 5:00:03PM +0800, Gang Yan wrote:
>
> Hi Matthieu,
>
> Happy to receive your reply!
>> On Thu, Dec 12, 2024 at 07:23:02PM +0100, Matthieu Baerts wrote:
>> Hi Gang Yan,
>>
>> Thank you for this patch!
>>
>> I have a few suggestions below. Do you mind sending the next version(s)
>> to the MPTCP list only, no need to include the netdev ML and the net
>> maintainers for the moment I think.
>>
>
> OK, sorry for this. I'll be careful next time.
For (urgent/small/occasional) fixes, that's fine to send them directly
to netdev. For new features, I think it is better to reduce the audience
first, and suggest something reviewed to netdev maintainers.
>> On 11/12/2024 10:03, Gang Yan wrote:
>>> From: Gang Yan <yangang@kylinos.cn>
>>>
>>> This patch fixes an issue where an invalid address is announce as a
>>> signal, the 'add_addr_accepted' is incorrectly added several times
>>> when 'retransmit ADD_ADDR'. So we need to update this variable
>>> when the connection is removed from conn_list by mptcp_worker. So that
>>> the available address can be added in time.
>>>
>>> In fact, the 'add_addr_accepted' is only declined when 'RM_ADDR'
>>> by now, so when subflows are getting closed from the other peer,
>>
>> Does it mean that in this case, the counter will be decreased twice:
>> when the RM_ADDR is received, and when the subflow is closed, no?
>>
>> I guess no because you hooked this in a different path, right?
>
> Yes, the counter will not be declined twice. The RM_ADDR will invoke
> mptcp_pm_nl_rm_addr_or_subflow() in pm_netlink.c, which will ignore
> the subflow of state '(TCPF_FIN_WAIT1 | TCPF_FIN_WAIT2 | TCPF_CLOSING |
> TCPF_CLOSE)' , the subflow of these states will turn into 'TCPF_CLOSE'
> and finally closed in __mptcp_close_subflow() because of mptcp_worker.
Thank you for having checked.
>>> the new signal is not accepted as well.
>>>
>>> We noticed there have exist some problems related to this.I think
>>> this patch effectively resolves them.
>>
>> Please add new test cases for these problems in the MPTCP selftests: to
>> better understand what is being fixed, and to avoid regressions later.
>>
>
> Actually, I noticed this issue due to "./mptcp_join.sh 28" running failed in my
> machine, but passed in docker-virtme, and it can be reproduced very easily.
> That's mainly because the status of invalid_addr's tcp connection is different.
> It is 'TCPF_SYNC_SENT' in virtme, but 'TCPF_CLOSE' in another.
Strange, do you know why? Is it due to a different kernel configuration?
Do you have more details about that?
> So the
> differences of states will cause a different return value of
> lookup_subflow_by_daddr() in mptcp_pm_nl_add_addr_received() which will add
> the counter twice when retransmit 'ADD_ADDR'.
Sounds like a bug that needs to be addressed first, no?
> Further, I'll add a new test for covering the specific scene.
Thanks!
>>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/498
>>> Signed-off-by: Gang Yan <yangang@kylinos.cn>
>>> ---
>>> net/mptcp/protocol.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 21bc3586c33e..f99dddca859d 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -2569,6 +2569,10 @@ static void __mptcp_close_subflow(struct sock *sk)
>>> continue;
>>>
>>> mptcp_close_ssk(sk, ssk, subflow);
>>> +
>>> + if (READ_ONCE(subflow->remote_id) &&
>>> + --msk->pm.add_addr_accepted < mptcp_pm_get_add_addr_accept_max(msk))
>>> + WRITE_ONCE(msk->pm.accept_addr, true);
>> Mmh, I don't think it can be that simple: potentially, an accepted
>> ADD_ADDR can trigger multiple subflows (i.e. when the fullmesh flag is
>> used). In this case, the counter has been incremented once, not once for
>> each created subflow. So before decrementing the counter, it should then
>> be needed to check if no other subflows connected to the same remote ID
>> are still alive.
>
> Thanks for your advice, sorry for not considering the 'fullmesh' situations,
> the check seems necessary.
>
>> I think it is better not to decrement this counter in "unusual
>> situations" -- the situation before this patch -- than wrongly
>> decrementing it, and ended up with an underflow.
>
> As the description beyond, the invalid address has caused not accepting
> an available address due to this counter, I think we should ensure the
> available address can be added at least. And an restrict condition can solve
> the underflow problem.
OK, there is maybe another issue to fix first. But what I meant is that:
if it is not possible to have a perfect solution, I think it is better
not to decrement the counter than wrongly decrementing it, and get other
issues.
>> Another thing is that subflows might have not been created upon the
>> reception of an ADD_ADDR: typically if you take the view of a server,
>> the subflows have been initiated by the client, and not because the
>> server got an ADD_ADDR. If I'm not mistaken, the counter would be
>> decremented here as well. We could restrict this by checking
>> "subflow->request_join" I suppose. Still, I'm wondering if that's
>> covering all cases,
>
> But does the subflow in server side have 'remote id'? if doesn't, the counter
> will not be declined in server side.
It can be >0, e.g. a client has two addresses (C1, C2), and the server
one (S1) → the client might open 2 subflows:
- C1-S1 → IDs: 0-0
- C2-S1 → IDs: 1-0
From the server side, the second subflow will have a remote ID of 1.
> For covering all cases, I suppose put the
> patch code in '__mptcp_close_ssk', do you think so?
Could it be possible to have a hook from there? e.g.
mptcp_pm_subflow_closed_external() ("_external" to clarify that it has
been closed by an external factor: this hook will not be called when the
PM itself asks to close the subflow). Then each PM can do extra actions
if needed.
>> and if we should not track ADD_ADDR that are received:
>>
>> https://github.com/multipath-tcp/mptcp_net-next/issues/496
>>
>> (which is more complex)
>
> For the problem 1 and 3, I noticed there exists 'anno_list' in server side,
> and we have timer for retransmitting 'add_addr', do you think we can
> modify the two modules to fix these problems rather than add a new list.
> For the 'add_addr_accepted', I think using a restrict condition to update
> the counter is enough.
There are quite a few cases that are possible, I cannot say at 100% that
a restrict condition is enough. I think it is, but best to double-check:
with the in-kernel PM, in which cases can we have a subflow created to
an address announced by the server? (+ be careful with the "corner"
cases: what if the server sends a RM_ADDR for its first address (ID 0),
then re-announce it later on?)
Maybe enough to check for 'subflow->remote_id > 0' and
'subflow->request_join'?
Could we have concurrency issues here? e.g. the other host sends a
RM_ADDR, followed by the closure of the subflow: I guess it is fine
because the receiver will treat the RM_ADDR first, close the subflow,
then handle the closure from the other side. What if there is are
reordering: first the closure, then the RM_ADDR? Will the kernel discard
the RM_ADDR? (I didn't check, but I guess yes, I recently added a check
of the state: if it is closed/closing, ignore the RM_ADDR, no?)
>> Also, because this counter is specific to the in-kernel PM, I think it
>> would be cleaner f its manipulation is done in pm_netlink.c.
Something else to take into consideration: this counter will only be
incremented with the in-kernel PM. We should then not decrement it when
the userspace PM is used.
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
Hi Gang,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/12275226711
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/5ad61844af3a
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=916728
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
© 2016 - 2024 Red Hat, Inc.