netdev - Re: [RFC PATCH 1/3] mptcp: add new sock flag to deal with join subflows

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 12 May 2020 17:09:32 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     netdev <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Christoph Paasch <cpaasch@...le.com>
Subject: Re: [RFC PATCH 1/3] mptcp: add new sock flag to deal with join
 subflows

Hi,

On Tue, 2020-05-12 at 07:46 -0700, Eric Dumazet wrote:
> On Tue, May 12, 2020 at 7:11 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > MP_JOIN subflows must not land into the accept queue.
> > Currently tcp_check_req() calls an mptcp specific helper
> > to detect such scenario.
> > 
> > Such helper leverages the subflow context to check for
> > MP_JOIN subflows. We need to deal also with MP JOIN
> > failures, even when the subflow context is not available
> > due to allocation failure.
> > 
> > A possible solution would be changing the syn_recv_sock()
> > signature to allow returning a more descriptive action/
> > error code and deal with that in tcp_check_req().
> > 
> > Since the above need is MPTCP specific, this patch instead
> > uses a TCP socket hole to add an MPTCP specific flag.
> > Such flag is used by the MPTCP syn_recv_sock() to tell
> > tcp_check_req() how to deal with the request socket.
> > 
> > This change is a no-op for !MPTCP build, and makes the
> > MPTCP code simpler. It allows also the next patch to deal
> > correctly with MP JOIN failure.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> > ---
> >  include/linux/tcp.h      |  1 +
> >  include/net/mptcp.h      | 17 ++++++++++-------
> >  net/ipv4/tcp_minisocks.c |  2 +-
> >  net/mptcp/protocol.c     |  7 -------
> >  net/mptcp/subflow.c      |  2 ++
> >  5 files changed, 14 insertions(+), 15 deletions(-)
> > 
> > diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> > index e60db06ec28d..dc12c59db41e 100644
> > --- a/include/linux/tcp.h
> > +++ b/include/linux/tcp.h
> > @@ -385,6 +385,7 @@ struct tcp_sock {
> >                            */
> >  #if IS_ENABLED(CONFIG_MPTCP)
> >         bool    is_mptcp;
> > +       bool    drop_req;
> >  #endif
> 
> This looks like this should only be needed in struct tcp_request_sock ?
> 
> Does this information need to be kept in the TCP socket after accept() ?

Thank you for the feedback, indeed you are right! this should be moved
inside tcp_request_sock. We still have 2 bytes hole there. I will keep
the flag under a CONFIG_MPTCP conditional.

Would you be ok with such approach, or do you think we should look for
some other schema (like the alternative mentioned in the cover letter)?

Cheers,

Paolo