[MPTCP] Re: [PATCH] mptcp: reset 'first' and ack_hint on subflow close

[MPTCP] Re: [PATCH] mptcp: reset 'first' and ack_hint on subflow close

[Paolo Abeni]

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

On Thu, 2021-02-18 at 16:59 +0100, Florian Westphal wrote:
> Just like with last_snd, we have to NULL 'first' on subflow close.
> 
> ack_hint isn't strictly required (its never dereferenced), but better to
> clear this as well.
> 
> msk->first is dereferenced unconditionally at accept time, but
> at that point the ssk is not on the conn_list yet -- this means
> worker can't see it when iterating the conn_list.

There is a __mptcp_flush_join_list() before __mptcp_close_subflow(),
could that matter? IIRC even the PM can move the subflow from join_list
to conn_list to take some actions.

not 110% related, we have a couple of !msk->first in 
mptcp_update_rcv_data_fin() and mptcp_check_data_fin() which are likely
obsoleted/to be removed.

Thanks!

Paolo

[MPTCP] Re: [PATCH] mptcp: reset 'first' and ack_hint on subflow close

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Thu, 2021-02-18 at 16:59 +0100, Florian Westphal wrote:
> > Just like with last_snd, we have to NULL 'first' on subflow close.
> > 
> > ack_hint isn't strictly required (its never dereferenced), but better to
> > clear this as well.
> > 
> > msk->first is dereferenced unconditionally at accept time, but
> > at that point the ssk is not on the conn_list yet -- this means
> > worker can't see it when iterating the conn_list.
> 
> There is a __mptcp_flush_join_list() before __mptcp_close_subflow(),
> could that matter? IIRC even the PM can move the subflow from join_list
> to conn_list to take some actions.
> 
> not 110% related, we have a couple of !msk->first in 
> mptcp_update_rcv_data_fin() and mptcp_check_data_fin() which are likely
> obsoleted/to be removed.

Hmm, if those are redunant, what exactly do we need msk->first for?
Fallback mode could just fetch the subflow off conn_list.

Matthieu, please ignore this patch for now, I'd prefer to remove
msk->first completely if possible.

[MPTCP] Re: [PATCH] mptcp: reset 'first' and ack_hint on subflow close

[Paolo Abeni]

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

On Thu, 2021-02-18 at 17:23 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > On Thu, 2021-02-18 at 16:59 +0100, Florian Westphal wrote:
> > > Just like with last_snd, we have to NULL 'first' on subflow close.
> > > 
> > > ack_hint isn't strictly required (its never dereferenced), but better to
> > > clear this as well.
> > > 
> > > msk->first is dereferenced unconditionally at accept time, but
> > > at that point the ssk is not on the conn_list yet -- this means
> > > worker can't see it when iterating the conn_list.
> > 
> > There is a __mptcp_flush_join_list() before __mptcp_close_subflow(),
> > could that matter? IIRC even the PM can move the subflow from join_list
> > to conn_list to take some actions.
> > 
> > not 110% related, we have a couple of !msk->first in 
> > mptcp_update_rcv_data_fin() and mptcp_check_data_fin() which are likely
> > obsoleted/to be removed.
> 
> Hmm, if those are redunant, what exactly do we need msk->first for?
> Fallback mode could just fetch the subflow off conn_list.

AFAICS it mostly avoids traversing the conn_list in some scenarios ;)
IIRC I added that thing as early optimization (== root of all evil)

> 
> Matthieu, please ignore this patch for now, I'd prefer to remove
> msk->first completely if possible.

I would love this latter option!

(in the long term I think it would be nice also remote ->subflow)

Thanks!

Paolo

[MPTCP] Re: [PATCH] mptcp: reset 'first' and ack_hint on subflow close

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Thu, 2021-02-18 at 16:59 +0100, Florian Westphal wrote:
> > Just like with last_snd, we have to NULL 'first' on subflow close.
> > 
> > ack_hint isn't strictly required (its never dereferenced), but better to
> > clear this as well.
> > 
> > msk->first is dereferenced unconditionally at accept time, but
> > at that point the ssk is not on the conn_list yet -- this means
> > worker can't see it when iterating the conn_list.
> 
> There is a __mptcp_flush_join_list() before __mptcp_close_subflow(),
> could that matter? IIRC even the PM can move the subflow from join_list
> to conn_list to take some actions.

AFAICS msk->first gets set/used to avoid use of join_list.
The latter is only used for incoming mp_join, not mp_capable.

I guess we can use the join_list but we whould have to stop splicing it
to the conn_list unconditionally -- we would need to keep mp_capable one
on the join_list until accept() time.

I think its better to apply this as-is at the moment, removing of
->first seems possible but the naive approach un-does several fixes
you made to avoid exposure of the first subflow to the work queue.

[MPTCP] Re: [PATCH] mptcp: reset 'first' and ack_hint on subflow close

[Paolo Abeni]

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

On Mon, 2021-02-22 at 09:28 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > On Thu, 2021-02-18 at 16:59 +0100, Florian Westphal wrote:
> > > Just like with last_snd, we have to NULL 'first' on subflow close.
> > > 
> > > ack_hint isn't strictly required (its never dereferenced), but better to
> > > clear this as well.
> > > 
> > > msk->first is dereferenced unconditionally at accept time, but
> > > at that point the ssk is not on the conn_list yet -- this means
> > > worker can't see it when iterating the conn_list.
> > 
> > There is a __mptcp_flush_join_list() before __mptcp_close_subflow(),
> > could that matter? IIRC even the PM can move the subflow from join_list
> > to conn_list to take some actions.
> 
> AFAICS msk->first gets set/used to avoid use of join_list.
> The latter is only used for incoming mp_join, not mp_capable.

Sharing here private conversation.

If msk->first is closed before accept, accept will still return an
established msk socket, with msk->first still valid and pointing to the
original subflow, in TCP_CLOSE status (and still with a valid refcnt
[==1])

TL;DR: the patch looks safe, no need for a complex codying to remove
msk->first field.

Acked-by: Paolo Abeni <pabeni(a)redhat.com>