Re: [PATCH mptcp-net v2 3/5] mptcp: fix status reset on disconnect()

[Matthieu Baerts <matttbe@kernel.org>]

Hi Paolo,

Thank you for your reply!

On 09/07/2025 11:27, Paolo Abeni wrote:
> 
> 
> On 7/8/25 5:43 PM, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 05/07/2025 09:24, Paolo Abeni wrote:
>>> disconnect() can still race with subflow fallback leaving the msk in
>>> inconsistent status. To avoid such race, disconnect() must be blocking,
>>> error out if the argument prevent such wait: the socket will linger
>>> in SS_DISCONNECT state forever.
>>
>> Good idea!
>>
>> Just to be sure, this is not really a behaviour change compared to TCP,
>> right?
> 
> Actually it is, as the TCP disconnect never blocks. On the flip side the
> race actually exists and I haven't find another way to plug it.
> 
> An alternative I thought about was to create a new first subflow on
> disconnect and let the old one 'detach' from the mptcp socket, but that
> was problematic as the 'detached' ssk would have a NULL subflow->conn
> and currently we assume that to be ~always != NULL

Indeed, it doesn't sound safer :)

>> No risks of breaking some cases? (io_uring, etc.?)
> 
> Do io_uring work with mptcp?

I don't know, I never tried. But I don't see why it wouldn't. (Of
course, not when used with zero copy, etc.)

> The disconnect failure when blocking is not
> possible should really never be a problem from a functional PoV:
> disconnect() can fail per documentation and the application should
> handle that case.

But it could set "O_NONBLOCK" and do the close later, no? Or is this
correctly handled?

> Blocking could cause some additional latency or slowdown in that code
> path. I think it should be not problematic, but I have no real idea and
> tend to be too optimistic WRT this kind of issues.
> 
> I suggest not sending this to stable (with a note in the commit message).

Maybe safer indeed.

>> In which cases is this MSG_DONTWAIT flag set with disconnect()?
> 
> Typo in the patch, should be O_NONBLOCK. The socket layer, the vfs layer
>  and iouring could set that.

So typically what we do in the packetdrill tests. But maybe we don't
check the returned value?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.