[MPTCP] Re: [PATCH mptcp 4/7] mptcp: avoid callback invocation when mptcp parent socket doesn't exist

[MPTCP] Re: [PATCH mptcp 4/7] mptcp: avoid callback invocation when mptcp parent socket doesn't exist

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 622 bytes --]

On Wed, 2020-04-15 at 19:19 +0200, Florian Westphal wrote:
> We can crash with a NULL dereference in case a packet arrives on the new
> socket before ctx->conn has been initialized.

I don't understand this race. I thought tcp_v4_syn_recv_sock() creates
the new socket and acquires the socket lock atomically, and ctx->conn =
new_msk happens in the same critical section ?!?

So no packets should reach the newly created socket while conn is NULL
!?!

but I do see a similar crash even if ctx->conn is not NULL, is a low,
invalid value.

I'll try to give this patch a spin in my testbed.

Thanks!


/P

[MPTCP] Re: [PATCH mptcp 4/7] mptcp: avoid callback invocation when mptcp parent socket doesn't exist

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Wed, 2020-04-15 at 19:19 +0200, Florian Westphal wrote:
> > We can crash with a NULL dereference in case a packet arrives on the new
> > socket before ctx->conn has been initialized.
> 
> I don't understand this race. I thought tcp_v4_syn_recv_sock() creates
> the new socket and acquires the socket lock atomically, and ctx->conn =
> new_msk happens in the same critical section ?!?
> 
> So no packets should reach the newly created socket while conn is NULL
> !?!

Might be related to the last patch, i.e. this patch is bogus because the
NULL was because of lack of is_mptcp = 0 assignment?

I will test again with this patch removed from the series.

sorry.

[MPTCP] Re: [PATCH mptcp 4/7] mptcp: avoid callback invocation when mptcp parent socket doesn't exist

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

Florian Westphal <fw(a)strlen.de> wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > On Wed, 2020-04-15 at 19:19 +0200, Florian Westphal wrote:
> > > We can crash with a NULL dereference in case a packet arrives on the new
> > > socket before ctx->conn has been initialized.
> > 
> > I don't understand this race. I thought tcp_v4_syn_recv_sock() creates
> > the new socket and acquires the socket lock atomically, and ctx->conn =
> > new_msk happens in the same critical section ?!?
> > 
> > So no packets should reach the newly created socket while conn is NULL
> > !?!
> 
> Might be related to the last patch, i.e. this patch is bogus because the
> NULL was because of lack of is_mptcp = 0 assignment?
> 
> I will test again with this patch removed from the series.

I powered off my VM after ~8h of continuos tests; but without this patch I
get crash after ~30 minutes.

I think its related to the case where we return child with subflow but
with ctx->conn == NULL.

I plan to mangle the last patch with this one and reset the sk callbacks
in that case.

Alternative is to keep this patch as-is, squash in the last one and
amdend the commit message.