netdev - Re: [PATCH net-next v2 4/6] tcp: rstreason: introduce SK_RST_REASON_TCP

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL+tcoD5OK6usHhJUGLn0WogBxVDJV8HF209ZBkd=xNLP34SfQ@mail.gmail.com>
Date: Thu, 1 Aug 2024 17:51:20 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, 
	dsahern@...nel.org, kuniyu@...zon.com, netdev@...r.kernel.org, 
	Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v2 4/6] tcp: rstreason: introduce
 SK_RST_REASON_TCP_STATE for active reset

Hello Eric,

On Thu, Aug 1, 2024 at 2:56 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Wed, Jul 31, 2024 at 2:10 PM Jason Xing <kerneljasonxing@...il.com> wrote:
> >
> > From: Jason Xing <kernelxing@...cent.com>
> >
> > Introducing a new type TCP_STATE to handle some reset conditions
> > appearing in RFC 793 due to its socket state. Actually, we can look
> > into RFC 9293 which has no discrepancy about this part.
> >
> > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > ---
> > V2
> > Link: https://lore.kernel.org/all/20240730200633.93761-1-kuniyu@amazon.com/
> > 1. use RFC 9293 instead of RFC 793 which is too old (Kuniyuki)
> > ---
> >  include/net/rstreason.h | 6 ++++++
> >  net/ipv4/tcp.c          | 4 ++--
> >  net/ipv4/tcp_timer.c    | 2 +-
> >  3 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/net/rstreason.h b/include/net/rstreason.h
> > index eef658da8952..bbf20d0bbde7 100644
> > --- a/include/net/rstreason.h
> > +++ b/include/net/rstreason.h
> > @@ -20,6 +20,7 @@
> >         FN(TCP_ABORT_ON_CLOSE)          \
> >         FN(TCP_ABORT_ON_LINGER)         \
> >         FN(TCP_ABORT_ON_MEMORY)         \
> > +       FN(TCP_STATE)                   \
> >         FN(MPTCP_RST_EUNSPEC)           \
> >         FN(MPTCP_RST_EMPTCP)            \
> >         FN(MPTCP_RST_ERESOURCE)         \
> > @@ -102,6 +103,11 @@ enum sk_rst_reason {
> >          * corresponding to LINUX_MIB_TCPABORTONMEMORY
> >          */
> >         SK_RST_REASON_TCP_ABORT_ON_MEMORY,
> > +       /**
> > +        * @SK_RST_REASON_TCP_STATE: abort on tcp state
> > +        * Please see RFC 9293 for all possible reset conditions
> > +        */
> > +       SK_RST_REASON_TCP_STATE,
> >
> >         /* Copy from include/uapi/linux/mptcp.h.
> >          * These reset fields will not be changed since they adhere to
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index fd928c447ce8..64a49cb714e1 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -3031,7 +3031,7 @@ int tcp_disconnect(struct sock *sk, int flags)
> >                 /* The last check adjusts for discrepancy of Linux wrt. RFC
> >                  * states
> >                  */
> > -               tcp_send_active_reset(sk, gfp_any(), SK_RST_REASON_NOT_SPECIFIED);
> > +               tcp_send_active_reset(sk, gfp_any(), SK_RST_REASON_TCP_STATE);
>
> I disagree with this. tcp_disconnect() is initiated by the user.
>
> You are conflating two possible conditions :
>
> 1) tcp_need_reset(old_state)

For this one, I can keep the TCP_STATE reason, right?

> 2) (tp->snd_nxt != tp->write_seq && (1 << old_state) & (TCPF_CLOSING |
> TCPF_LAST_ACK)))
>

For this one, I wonder if I need to separate this condition with
'tcp_need_reset()' and put it into another 'else-if' branch?
I decided to name it as 'CLOSE_WITH_DATA' because it can reflect that
the write queue of the socket is not empty (at this time the user may
think he has more data to send) but it stays in the active close
state.
How about it?

Thanks,
Jason