* [PATCH net-next v2 1/6] tcp: rstreason: introduce SK_RST_REASON_TCP_ABORT_ON_CLOSE for active reset
2024-07-31 12:09 [PATCH net-next v2 0/6] tcp: completely support active reset Jason Xing
@ 2024-07-31 12:09 ` Jason Xing
2024-08-01 6:47 ` Eric Dumazet
2024-07-31 12:09 ` [PATCH net-next v2 2/6] tcp: rstreason: introduce SK_RST_REASON_TCP_ABORT_ON_LINGER " Jason Xing
` (4 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Jason Xing @ 2024-07-31 12:09 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, dsahern, kuniyu; +Cc: netdev, Jason Xing
From: Jason Xing <kernelxing@tencent.com>
Introducing a new type TCP_ABORT_ON_CLOSE for tcp reset reason to handle
the case where more data is unread in closing phase.
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
include/net/rstreason.h | 6 ++++++
net/ipv4/tcp.c | 2 +-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/net/rstreason.h b/include/net/rstreason.h
index 2575c85d7f7a..fa6bfd0d7d69 100644
--- a/include/net/rstreason.h
+++ b/include/net/rstreason.h
@@ -17,6 +17,7 @@
FN(TCP_ABORT_ON_DATA) \
FN(TCP_TIMEWAIT_SOCKET) \
FN(INVALID_SYN) \
+ FN(TCP_ABORT_ON_CLOSE) \
FN(MPTCP_RST_EUNSPEC) \
FN(MPTCP_RST_EMPTCP) \
FN(MPTCP_RST_ERESOURCE) \
@@ -84,6 +85,11 @@ enum sk_rst_reason {
* an error, send a reset"
*/
SK_RST_REASON_INVALID_SYN,
+ /**
+ * @SK_RST_REASON_TCP_ABORT_ON_CLOSE: abort on close
+ * corresponding to LINUX_MIB_TCPABORTONCLOSE
+ */
+ SK_RST_REASON_TCP_ABORT_ON_CLOSE,
/* 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 e03a342c9162..2e010add0317 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2833,7 +2833,7 @@ void __tcp_close(struct sock *sk, long timeout)
NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTONCLOSE);
tcp_set_state(sk, TCP_CLOSE);
tcp_send_active_reset(sk, sk->sk_allocation,
- SK_RST_REASON_NOT_SPECIFIED);
+ SK_RST_REASON_TCP_ABORT_ON_CLOSE);
} else if (sock_flag(sk, SOCK_LINGER) && !sk->sk_lingertime) {
/* Check zero linger _after_ checking for unread data. */
sk->sk_prot->disconnect(sk, 0);
--
2.37.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH net-next v2 2/6] tcp: rstreason: introduce SK_RST_REASON_TCP_ABORT_ON_LINGER for active reset
2024-07-31 12:09 [PATCH net-next v2 0/6] tcp: completely support active reset Jason Xing
2024-07-31 12:09 ` [PATCH net-next v2 1/6] tcp: rstreason: introduce SK_RST_REASON_TCP_ABORT_ON_CLOSE for " Jason Xing
@ 2024-07-31 12:09 ` Jason Xing
2024-08-01 6:48 ` Eric Dumazet
2024-07-31 12:09 ` [PATCH net-next v2 3/6] tcp: rstreason: introduce SK_RST_REASON_TCP_ABORT_ON_MEMORY " Jason Xing
` (3 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Jason Xing @ 2024-07-31 12:09 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, dsahern, kuniyu; +Cc: netdev, Jason Xing
From: Jason Xing <kernelxing@tencent.com>
Introducing a new type TCP_ABORT_ON_LINGER for tcp reset reason to handle
negative linger value case.
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
include/net/rstreason.h | 6 ++++++
net/ipv4/tcp.c | 2 +-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/net/rstreason.h b/include/net/rstreason.h
index fa6bfd0d7d69..fbbaeb969e6a 100644
--- a/include/net/rstreason.h
+++ b/include/net/rstreason.h
@@ -18,6 +18,7 @@
FN(TCP_TIMEWAIT_SOCKET) \
FN(INVALID_SYN) \
FN(TCP_ABORT_ON_CLOSE) \
+ FN(TCP_ABORT_ON_LINGER) \
FN(MPTCP_RST_EUNSPEC) \
FN(MPTCP_RST_EMPTCP) \
FN(MPTCP_RST_ERESOURCE) \
@@ -90,6 +91,11 @@ enum sk_rst_reason {
* corresponding to LINUX_MIB_TCPABORTONCLOSE
*/
SK_RST_REASON_TCP_ABORT_ON_CLOSE,
+ /**
+ * @SK_RST_REASON_TCP_ABORT_ON_LINGER: abort on linger
+ * corresponding to LINUX_MIB_TCPABORTONLINGER
+ */
+ SK_RST_REASON_TCP_ABORT_ON_LINGER,
/* 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 2e010add0317..5b0f1d1fc697 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2908,7 +2908,7 @@ void __tcp_close(struct sock *sk, long timeout)
if (READ_ONCE(tp->linger2) < 0) {
tcp_set_state(sk, TCP_CLOSE);
tcp_send_active_reset(sk, GFP_ATOMIC,
- SK_RST_REASON_NOT_SPECIFIED);
+ SK_RST_REASON_TCP_ABORT_ON_LINGER);
__NET_INC_STATS(sock_net(sk),
LINUX_MIB_TCPABORTONLINGER);
} else {
--
2.37.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH net-next v2 3/6] tcp: rstreason: introduce SK_RST_REASON_TCP_ABORT_ON_MEMORY for active reset
2024-07-31 12:09 [PATCH net-next v2 0/6] tcp: completely support active reset Jason Xing
2024-07-31 12:09 ` [PATCH net-next v2 1/6] tcp: rstreason: introduce SK_RST_REASON_TCP_ABORT_ON_CLOSE for " Jason Xing
2024-07-31 12:09 ` [PATCH net-next v2 2/6] tcp: rstreason: introduce SK_RST_REASON_TCP_ABORT_ON_LINGER " Jason Xing
@ 2024-07-31 12:09 ` Jason Xing
2024-08-01 6:49 ` Eric Dumazet
2024-07-31 12:09 ` [PATCH net-next v2 4/6] tcp: rstreason: introduce SK_RST_REASON_TCP_STATE " Jason Xing
` (2 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Jason Xing @ 2024-07-31 12:09 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, dsahern, kuniyu; +Cc: netdev, Jason Xing
From: Jason Xing <kernelxing@tencent.com>
Introducing a new type TCP_ABORT_ON_MEMORY for tcp reset reason to handle
out of memory case.
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
include/net/rstreason.h | 6 ++++++
net/ipv4/tcp.c | 2 +-
net/ipv4/tcp_timer.c | 2 +-
3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/net/rstreason.h b/include/net/rstreason.h
index fbbaeb969e6a..eef658da8952 100644
--- a/include/net/rstreason.h
+++ b/include/net/rstreason.h
@@ -19,6 +19,7 @@
FN(INVALID_SYN) \
FN(TCP_ABORT_ON_CLOSE) \
FN(TCP_ABORT_ON_LINGER) \
+ FN(TCP_ABORT_ON_MEMORY) \
FN(MPTCP_RST_EUNSPEC) \
FN(MPTCP_RST_EMPTCP) \
FN(MPTCP_RST_ERESOURCE) \
@@ -96,6 +97,11 @@ enum sk_rst_reason {
* corresponding to LINUX_MIB_TCPABORTONLINGER
*/
SK_RST_REASON_TCP_ABORT_ON_LINGER,
+ /**
+ * @SK_RST_REASON_TCP_ABORT_ON_MEMORY: abort on memory
+ * corresponding to LINUX_MIB_TCPABORTONMEMORY
+ */
+ SK_RST_REASON_TCP_ABORT_ON_MEMORY,
/* 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 5b0f1d1fc697..fd928c447ce8 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2927,7 +2927,7 @@ void __tcp_close(struct sock *sk, long timeout)
if (tcp_check_oom(sk, 0)) {
tcp_set_state(sk, TCP_CLOSE);
tcp_send_active_reset(sk, GFP_ATOMIC,
- SK_RST_REASON_NOT_SPECIFIED);
+ SK_RST_REASON_TCP_ABORT_ON_MEMORY);
__NET_INC_STATS(sock_net(sk),
LINUX_MIB_TCPABORTONMEMORY);
} else if (!check_net(sock_net(sk))) {
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 4d40615dc8fc..0fba4a4fb988 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -125,7 +125,7 @@ static int tcp_out_of_resources(struct sock *sk, bool do_reset)
do_reset = true;
if (do_reset)
tcp_send_active_reset(sk, GFP_ATOMIC,
- SK_RST_REASON_NOT_SPECIFIED);
+ SK_RST_REASON_TCP_ABORT_ON_MEMORY);
tcp_done(sk);
__NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTONMEMORY);
return 1;
--
2.37.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH net-next v2 4/6] tcp: rstreason: introduce SK_RST_REASON_TCP_STATE for active reset
2024-07-31 12:09 [PATCH net-next v2 0/6] tcp: completely support active reset Jason Xing
` (2 preceding siblings ...)
2024-07-31 12:09 ` [PATCH net-next v2 3/6] tcp: rstreason: introduce SK_RST_REASON_TCP_ABORT_ON_MEMORY " Jason Xing
@ 2024-07-31 12:09 ` Jason Xing
2024-08-01 6:56 ` Eric Dumazet
2024-07-31 12:09 ` [PATCH net-next v2 5/6] tcp: rstreason: introduce SK_RST_REASON_TCP_TIMEOUT " Jason Xing
2024-07-31 12:09 ` [PATCH net-next v2 6/6] tcp: rstreason: let it work finally in tcp_send_active_reset() Jason Xing
5 siblings, 1 reply; 16+ messages in thread
From: Jason Xing @ 2024-07-31 12:09 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, dsahern, kuniyu; +Cc: netdev, Jason Xing
From: Jason Xing <kernelxing@tencent.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@tencent.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);
WRITE_ONCE(sk->sk_err, ECONNRESET);
} else if (old_state == TCP_SYN_SENT)
WRITE_ONCE(sk->sk_err, ECONNRESET);
@@ -4649,7 +4649,7 @@ int tcp_abort(struct sock *sk, int err)
if (!sock_flag(sk, SOCK_DEAD)) {
if (tcp_need_reset(sk->sk_state))
tcp_send_active_reset(sk, GFP_ATOMIC,
- SK_RST_REASON_NOT_SPECIFIED);
+ SK_RST_REASON_TCP_STATE);
tcp_done_with_error(sk, err);
}
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 0fba4a4fb988..3910f6d8614e 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -779,7 +779,7 @@ static void tcp_keepalive_timer (struct timer_list *t)
goto out;
}
}
- tcp_send_active_reset(sk, GFP_ATOMIC, SK_RST_REASON_NOT_SPECIFIED);
+ tcp_send_active_reset(sk, GFP_ATOMIC, SK_RST_REASON_TCP_STATE);
goto death;
}
--
2.37.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH net-next v2 4/6] tcp: rstreason: introduce SK_RST_REASON_TCP_STATE for active reset
2024-07-31 12:09 ` [PATCH net-next v2 4/6] tcp: rstreason: introduce SK_RST_REASON_TCP_STATE " Jason Xing
@ 2024-08-01 6:56 ` Eric Dumazet
2024-08-01 9:51 ` Jason Xing
0 siblings, 1 reply; 16+ messages in thread
From: Eric Dumazet @ 2024-08-01 6:56 UTC (permalink / raw)
To: Jason Xing; +Cc: davem, kuba, pabeni, dsahern, kuniyu, netdev, Jason Xing
On Wed, Jul 31, 2024 at 2:10 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.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@tencent.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)
2) (tp->snd_nxt != tp->write_seq && (1 << old_state) & (TCPF_CLOSING |
TCPF_LAST_ACK)))
> WRITE_ONCE(sk->sk_err, ECONNRESET);
> } else if (old_state == TCP_SYN_SENT)
> WRITE_ONCE(sk->sk_err, ECONNRESET);
> @@ -4649,7 +4649,7 @@ int tcp_abort(struct sock *sk, int err)
> if (!sock_flag(sk, SOCK_DEAD)) {
> if (tcp_need_reset(sk->sk_state))
> tcp_send_active_reset(sk, GFP_ATOMIC,
> - SK_RST_REASON_NOT_SPECIFIED);
> + SK_RST_REASON_TCP_STATE);
> tcp_done_with_error(sk, err);
> }
>
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 0fba4a4fb988..3910f6d8614e 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -779,7 +779,7 @@ static void tcp_keepalive_timer (struct timer_list *t)
> goto out;
> }
> }
> - tcp_send_active_reset(sk, GFP_ATOMIC, SK_RST_REASON_NOT_SPECIFIED);
> + tcp_send_active_reset(sk, GFP_ATOMIC, SK_RST_REASON_TCP_STATE);
> goto death;
> }
>
> --
> 2.37.3
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH net-next v2 4/6] tcp: rstreason: introduce SK_RST_REASON_TCP_STATE for active reset
2024-08-01 6:56 ` Eric Dumazet
@ 2024-08-01 9:51 ` Jason Xing
2024-08-01 10:06 ` Eric Dumazet
0 siblings, 1 reply; 16+ messages in thread
From: Jason Xing @ 2024-08-01 9:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, kuba, pabeni, dsahern, kuniyu, netdev, Jason Xing
Hello Eric,
On Thu, Aug 1, 2024 at 2:56 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Jul 31, 2024 at 2:10 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > From: Jason Xing <kernelxing@tencent.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@tencent.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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH net-next v2 4/6] tcp: rstreason: introduce SK_RST_REASON_TCP_STATE for active reset
2024-08-01 9:51 ` Jason Xing
@ 2024-08-01 10:06 ` Eric Dumazet
2024-08-01 10:24 ` Jason Xing
0 siblings, 1 reply; 16+ messages in thread
From: Eric Dumazet @ 2024-08-01 10:06 UTC (permalink / raw)
To: Jason Xing; +Cc: davem, kuba, pabeni, dsahern, kuniyu, netdev, Jason Xing
On Thu, Aug 1, 2024 at 11:51 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> Hello Eric,
>
> On Thu, Aug 1, 2024 at 2:56 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Wed, Jul 31, 2024 at 2:10 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > >
> > > From: Jason Xing <kernelxing@tencent.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@tencent.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?
What does it mean ?
>
> > 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?
This is not CLOSE_WITH_DATA, but a disconnect() operation, initiated
by user space.
If we add RST reasons, can we please be careful about the chosen names ?
man connect
<quote>
Some protocol sockets (e.g., TCP sockets as well as datagram
sockets in the UNIX and Internet domains) may dissolve the association
by connecting to an address with the sa_family member of sockaddr set
to AF_UNSPEC; thereafter, the socket can be connected to another ad‐
dress. (AF_UNSPEC is supported since Linux 2.2.)
</quote>
Very different from close()...
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH net-next v2 4/6] tcp: rstreason: introduce SK_RST_REASON_TCP_STATE for active reset
2024-08-01 10:06 ` Eric Dumazet
@ 2024-08-01 10:24 ` Jason Xing
0 siblings, 0 replies; 16+ messages in thread
From: Jason Xing @ 2024-08-01 10:24 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, kuba, pabeni, dsahern, kuniyu, netdev, Jason Xing
Hello Eric,
On Thu, Aug 1, 2024 at 6:06 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Aug 1, 2024 at 11:51 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > Hello Eric,
> >
> > On Thu, Aug 1, 2024 at 2:56 PM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Wed, Jul 31, 2024 at 2:10 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > > >
> > > > From: Jason Xing <kernelxing@tencent.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@tencent.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?
>
> What does it mean ?
I mean I wonder if I can use the TCP_STATE reason in tcp_abort() and
tcp_disconnect() when tcp_need_reset() returns true?
>
> >
> > > 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?
>
> This is not CLOSE_WITH_DATA, but a disconnect() operation, initiated
> by user space.
> If we add RST reasons, can we please be careful about the chosen names ?
Yes, I know, but like old days, I'm struggling with the English name. Sorry.
>
> man connect
>
> <quote>
> Some protocol sockets (e.g., TCP sockets as well as datagram
> sockets in the UNIX and Internet domains) may dissolve the association
> by connecting to an address with the sa_family member of sockaddr set
> to AF_UNSPEC; thereafter, the socket can be connected to another ad‐
> dress. (AF_UNSPEC is supported since Linux 2.2.)
> </quote>
>
> Very different from close()...
Oh, I see. What I was talking about 'CLOSE' is the socket state, but
you are right: the name will finally be displayed to users, which must
clearly reflect the real meaning of the underlying behavior.
I will use "TCP_DISCONNECT_WITH_DATA" instead under this condition.
And then, I will put it into a new patch since it's a different reason
name.
Thanks for your help!
Jason
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH net-next v2 5/6] tcp: rstreason: introduce SK_RST_REASON_TCP_TIMEOUT for active reset
2024-07-31 12:09 [PATCH net-next v2 0/6] tcp: completely support active reset Jason Xing
` (3 preceding siblings ...)
2024-07-31 12:09 ` [PATCH net-next v2 4/6] tcp: rstreason: introduce SK_RST_REASON_TCP_STATE " Jason Xing
@ 2024-07-31 12:09 ` Jason Xing
2024-08-01 6:58 ` Eric Dumazet
2024-07-31 12:09 ` [PATCH net-next v2 6/6] tcp: rstreason: let it work finally in tcp_send_active_reset() Jason Xing
5 siblings, 1 reply; 16+ messages in thread
From: Jason Xing @ 2024-07-31 12:09 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, dsahern, kuniyu; +Cc: netdev, Jason Xing
From: Jason Xing <kernelxing@tencent.com>
Whether user sets TCP_USER_TIMEOUT option or not, when we find there
is no left chance to proceed, we will send an RST to the other side.
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
v2
Link: https://lore.kernel.org/all/CAL+tcoB-12pUS0adK8M_=C97aXewYYmDA6rJKLXvAXEHvEsWjA@mail.gmail.com/
1. correct the comment and changelog
---
include/net/rstreason.h | 8 ++++++++
net/ipv4/tcp_timer.c | 2 +-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/include/net/rstreason.h b/include/net/rstreason.h
index bbf20d0bbde7..739ad1db4212 100644
--- a/include/net/rstreason.h
+++ b/include/net/rstreason.h
@@ -21,6 +21,7 @@
FN(TCP_ABORT_ON_LINGER) \
FN(TCP_ABORT_ON_MEMORY) \
FN(TCP_STATE) \
+ FN(TCP_TIMEOUT) \
FN(MPTCP_RST_EUNSPEC) \
FN(MPTCP_RST_EMPTCP) \
FN(MPTCP_RST_ERESOURCE) \
@@ -108,6 +109,13 @@ enum sk_rst_reason {
* Please see RFC 9293 for all possible reset conditions
*/
SK_RST_REASON_TCP_STATE,
+ /**
+ * @SK_RST_REASON_TCP_TIMEOUT: time to timeout
+ * Whether user sets TCP_USER_TIMEOUT options or not, when we
+ * have already run out of all the chances, we have to reset the
+ * connection
+ */
+ SK_RST_REASON_TCP_TIMEOUT,
/* Copy from include/uapi/linux/mptcp.h.
* These reset fields will not be changed since they adhere to
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 3910f6d8614e..bd403300e4c4 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -807,7 +807,7 @@ static void tcp_keepalive_timer (struct timer_list *t)
(user_timeout == 0 &&
icsk->icsk_probes_out >= keepalive_probes(tp))) {
tcp_send_active_reset(sk, GFP_ATOMIC,
- SK_RST_REASON_NOT_SPECIFIED);
+ SK_RST_REASON_TCP_TIMEOUT);
tcp_write_err(sk);
goto out;
}
--
2.37.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH net-next v2 5/6] tcp: rstreason: introduce SK_RST_REASON_TCP_TIMEOUT for active reset
2024-07-31 12:09 ` [PATCH net-next v2 5/6] tcp: rstreason: introduce SK_RST_REASON_TCP_TIMEOUT " Jason Xing
@ 2024-08-01 6:58 ` Eric Dumazet
2024-08-01 9:30 ` Jason Xing
0 siblings, 1 reply; 16+ messages in thread
From: Eric Dumazet @ 2024-08-01 6:58 UTC (permalink / raw)
To: Jason Xing; +Cc: davem, kuba, pabeni, dsahern, kuniyu, netdev, Jason Xing
On Wed, Jul 31, 2024 at 2:10 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> Whether user sets TCP_USER_TIMEOUT option or not, when we find there
> is no left chance to proceed, we will send an RST to the other side.
>
Not sure why you mention TCP_USER_TIMEOUT here.
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
> v2
> Link: https://lore.kernel.org/all/CAL+tcoB-12pUS0adK8M_=C97aXewYYmDA6rJKLXvAXEHvEsWjA@mail.gmail.com/
> 1. correct the comment and changelog
> ---
> include/net/rstreason.h | 8 ++++++++
> net/ipv4/tcp_timer.c | 2 +-
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/rstreason.h b/include/net/rstreason.h
> index bbf20d0bbde7..739ad1db4212 100644
> --- a/include/net/rstreason.h
> +++ b/include/net/rstreason.h
> @@ -21,6 +21,7 @@
> FN(TCP_ABORT_ON_LINGER) \
> FN(TCP_ABORT_ON_MEMORY) \
> FN(TCP_STATE) \
> + FN(TCP_TIMEOUT) \
> FN(MPTCP_RST_EUNSPEC) \
> FN(MPTCP_RST_EMPTCP) \
> FN(MPTCP_RST_ERESOURCE) \
> @@ -108,6 +109,13 @@ enum sk_rst_reason {
> * Please see RFC 9293 for all possible reset conditions
> */
> SK_RST_REASON_TCP_STATE,
> + /**
> + * @SK_RST_REASON_TCP_TIMEOUT: time to timeout
> + * Whether user sets TCP_USER_TIMEOUT options or not, when we
> + * have already run out of all the chances, we have to reset the
> + * connection
> + */
> + SK_RST_REASON_TCP_TIMEOUT,
>
> /* Copy from include/uapi/linux/mptcp.h.
> * These reset fields will not be changed since they adhere to
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 3910f6d8614e..bd403300e4c4 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -807,7 +807,7 @@ static void tcp_keepalive_timer (struct timer_list *t)
> (user_timeout == 0 &&
> icsk->icsk_probes_out >= keepalive_probes(tp))) {
> tcp_send_active_reset(sk, GFP_ATOMIC,
> - SK_RST_REASON_NOT_SPECIFIED);
> + SK_RST_REASON_TCP_TIMEOUT);
> tcp_write_err(sk);
> goto out;
> }
> --
> 2.37.3
>
This is more about keepalive really. You should use a better name
reflecting that it is a keepalive timeout.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH net-next v2 5/6] tcp: rstreason: introduce SK_RST_REASON_TCP_TIMEOUT for active reset
2024-08-01 6:58 ` Eric Dumazet
@ 2024-08-01 9:30 ` Jason Xing
0 siblings, 0 replies; 16+ messages in thread
From: Jason Xing @ 2024-08-01 9:30 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, kuba, pabeni, dsahern, kuniyu, netdev, Jason Xing
Hello Eric,
On Thu, Aug 1, 2024 at 2:58 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Jul 31, 2024 at 2:10 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Whether user sets TCP_USER_TIMEOUT option or not, when we find there
> > is no left chance to proceed, we will send an RST to the other side.
> >
>
> Not sure why you mention TCP_USER_TIMEOUT here.
>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> > v2
> > Link: https://lore.kernel.org/all/CAL+tcoB-12pUS0adK8M_=C97aXewYYmDA6rJKLXvAXEHvEsWjA@mail.gmail.com/
> > 1. correct the comment and changelog
> > ---
> > include/net/rstreason.h | 8 ++++++++
> > net/ipv4/tcp_timer.c | 2 +-
> > 2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/rstreason.h b/include/net/rstreason.h
> > index bbf20d0bbde7..739ad1db4212 100644
> > --- a/include/net/rstreason.h
> > +++ b/include/net/rstreason.h
> > @@ -21,6 +21,7 @@
> > FN(TCP_ABORT_ON_LINGER) \
> > FN(TCP_ABORT_ON_MEMORY) \
> > FN(TCP_STATE) \
> > + FN(TCP_TIMEOUT) \
> > FN(MPTCP_RST_EUNSPEC) \
> > FN(MPTCP_RST_EMPTCP) \
> > FN(MPTCP_RST_ERESOURCE) \
> > @@ -108,6 +109,13 @@ enum sk_rst_reason {
> > * Please see RFC 9293 for all possible reset conditions
> > */
> > SK_RST_REASON_TCP_STATE,
> > + /**
> > + * @SK_RST_REASON_TCP_TIMEOUT: time to timeout
> > + * Whether user sets TCP_USER_TIMEOUT options or not, when we
> > + * have already run out of all the chances, we have to reset the
> > + * connection
> > + */
> > + SK_RST_REASON_TCP_TIMEOUT,
> >
> > /* Copy from include/uapi/linux/mptcp.h.
> > * These reset fields will not be changed since they adhere to
> > diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> > index 3910f6d8614e..bd403300e4c4 100644
> > --- a/net/ipv4/tcp_timer.c
> > +++ b/net/ipv4/tcp_timer.c
> > @@ -807,7 +807,7 @@ static void tcp_keepalive_timer (struct timer_list *t)
> > (user_timeout == 0 &&
> > icsk->icsk_probes_out >= keepalive_probes(tp))) {
> > tcp_send_active_reset(sk, GFP_ATOMIC,
> > - SK_RST_REASON_NOT_SPECIFIED);
> > + SK_RST_REASON_TCP_TIMEOUT);
> > tcp_write_err(sk);
> > goto out;
> > }
> > --
> > 2.37.3
> >
>
> This is more about keepalive really. You should use a better name
> reflecting that it is a keepalive timeout.
I think you're right. Let me use 'TCP_KEEPALIVE_TIMEOUT' then.
Thanks,
Jason
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH net-next v2 6/6] tcp: rstreason: let it work finally in tcp_send_active_reset()
2024-07-31 12:09 [PATCH net-next v2 0/6] tcp: completely support active reset Jason Xing
` (4 preceding siblings ...)
2024-07-31 12:09 ` [PATCH net-next v2 5/6] tcp: rstreason: introduce SK_RST_REASON_TCP_TIMEOUT " Jason Xing
@ 2024-07-31 12:09 ` Jason Xing
5 siblings, 0 replies; 16+ messages in thread
From: Jason Xing @ 2024-07-31 12:09 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, dsahern, kuniyu; +Cc: netdev, Jason Xing
From: Jason Xing <kernelxing@tencent.com>
Now it's time to let it work by using the 'reason' parameter in
the trace world :)
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
net/ipv4/tcp_output.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 16c48df8df4c..cdd0def14427 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3649,7 +3649,7 @@ void tcp_send_active_reset(struct sock *sk, gfp_t priority,
/* skb of trace_tcp_send_reset() keeps the skb that caused RST,
* skb here is different to the troublesome skb, so use NULL
*/
- trace_tcp_send_reset(sk, NULL, SK_RST_REASON_NOT_SPECIFIED);
+ trace_tcp_send_reset(sk, NULL, reason);
}
/* Send a crossed SYN-ACK during socket establishment.
--
2.37.3
^ permalink raw reply related [flat|nested] 16+ messages in thread