Re: [mptcp-next] mptcp: fix invalid addr occupy 'add_addr_accepted'

[Matthieu Baerts <matttbe@kernel.org>]

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.