[PATCH 3/10]: Dedicated auxiliary states to support passive-close

[PATCH 3/10]: Dedicated auxiliary states to support passive-close

[Gerrit Renker]

[DCCP]: Dedicated auxiliary states to support passive-close

This adds two auxiliary states to deal with passive closes:
  * PASSIVE_1 (reached from OPEN via reception of Close) and
  * PASSIVE_2 (reached from OPEN via reception of CloseReq)
to the internal state machine.

The PASSIVE_1 and PASSIVE_2 states represent the two ways a passive-close
can happen in DCCP.  The addition of these states is not merely to
increase clarity. These states are required by the implementation to
allow a receiver to process unread data before acknowledging the received
connection-termination-request (i.e. the Close/CloseReq).

Fix:
----
 This patch uses the PASSIVE_1 and PASSIVE_2 states to explicitly refer to
 passive-closing states and to protect against external wipeout of internal
 receive queues. The macroscopic behaviour is compatible with RFC 4340, but
 without the auxiliary states, buggy, absurd and abnormal behaviour of the
 socket API will continue. Which is to say, without auxiliary states it does
 not work.

 As a consequence, the count of DCCP_STATE_MASK has been increased, to account
 for the number of new states. 


Implementation Note:
--------------------
 To keep compatibility with sk_stream_wait_connect():
 * DCCP_CLOSING continues to map into TCP_CLOSING (since this state can be
   either passive- or active-close)
 * DCCP_CLOSEREQ maps into TCP_FIN_WAIT1 (since it is always active-close)

 It is tempting to keep the clever merge of the CLOSEREQ and CLOSING states.
 However, with the number of possible state transitions, this would require:

 * quite a number of `if' statements to distinguish all predecessors of
   the CLOSING state (server/client, active/passive, server timewait yes/no);

 * two different branches from the CLOSING state:
   - to TIMEWAIT if it is not an active server-close without keeping timewait state
   - to CLOSED otherwise (and requiring to receive a Close instead of a Reset).
 
 In light of this, I think it is cleaner to implement separate CLOSEREQ and CLOSING
 states (this is done by the subsequent patches).

Further documentation is on http://www.erg.abdn.ac.uk/users/gerrit/dccp/docs/closing_states/


                    Why this is necessary [can be removed]
                    --------------------------------------

 The two states are not mentioned in the DCCP specification [RFC 4340, 8.4].
 In fact, RFC 4340 is silent about passive-close. In a nutshell, suppose that
 the CLOSE_WAIT and LAST_ACK states were removed from TCP, i.e. a FIN triggers
 direct transition from ESTABLISHED to CLOSED - similar problem. 
 The detailed account is as follows.

 The first problem lies in using inet_stream_connect():

 An absurd case arises if we let the protocol machinery, not the application,
 go through the states. If the protocol machinery allows to proceed to DCCP_CLOSED
 (which corresponds to TCP_CLOSE), connect() returns with -ECONNABORTED, even if
 there was data in the receive queue. 

 Therefore, if we want to keep the useful abstraction of inet_stream_connect(), we
 need to make sure that a passive close can not lead to DCCP_CLOSE via the protocol
 machinery alone. 

 The second problem is also related to entering DCCP_CLOSED too early: 

 Even if connect() were fixed, if a passive-close can directly trigger a state 
 transition from OPEN to DCCP_CLOSED, the receiver may not be able to read data
 from its input queue. The reason is that the input queue has already been wiped, 
 DCCP_CLOSED state has been entered, while the SOCK_DONE flag has not yet been set.
 Consequently, any subsequent call to dccp_recvmsg terminates in -ENOTCONN, 
 and this despite of a potentially full receive queue.


Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 include/linux/dccp.h |   22 +++++++++++++++++++++-
 net/dccp/proto.c     |    3 +++
 2 files changed, 24 insertions(+), 1 deletion(-)

--- a/include/linux/dccp.h
+++ b/include/linux/dccp.h
@@ -230,16 +230,35 @@ enum dccp_state {
 	DCCP_REQUESTING	= TCP_SYN_SENT,
 	DCCP_LISTEN	= TCP_LISTEN,
 	DCCP_RESPOND	= TCP_SYN_RECV,
+	/*
+	 *			Close states:
+	 *
+	 * CLOSEREQ is active-server close only.
+	 * CLOSING can have three different meanings [RFC 4340, 8.3]:
+	 *  a. Client has performed active-close, sent a Close to peer from
+	 *     state OPEN or PARTOPEN, waiting for the final Reset
+	 *     (in this case, SOCK_DONE = 1).
+	 *  b. Client performs passive-Close, by receiving an CloseReq in OPEN
+	 *     or PARTOPEN state. It sends a Close and waits for final Reset
+	 *     (in this case, SOCK_DONE = 0).
+	 *  c. Server decides to hold TIMEWAIT state & performs an active-close.
+	 * To avoid erasing receive queues too early, the transitional states
+	 * PASSIVE_1 (from OPEN => CLOSED) and PASSIVE_2 (from (PART)OPEN to
+	 * CLOSING, corresponds to (b) above) are used.
+	 */
+	DCCP_CLOSEREQ	= TCP_FIN_WAIT1,
 	DCCP_CLOSING	= TCP_CLOSING,
 	DCCP_TIME_WAIT	= TCP_TIME_WAIT,
 	DCCP_CLOSED	= TCP_CLOSE,
 	/* Everything below here is specific to DCCP only */
 	DCCP_INTRINSICS = TCP_MAX_STATES,
 	DCCP_PARTOPEN,
+	DCCP_PASSIVE_1,		/* any node receiving a Close  */
+	DCCP_PASSIVE_2,		/* client receiving a CloseReq */
 	DCCP_MAX_STATES
 };
 
-#define DCCP_STATE_MASK 0xf
+#define DCCP_STATE_MASK 0x1f
 #define DCCP_ACTION_FIN (1<<7)
 
 enum {
@@ -247,6 +266,7 @@ enum {
 	DCCPF_REQUESTING = TCPF_SYN_SENT,
 	DCCPF_LISTEN	 = TCPF_LISTEN,
 	DCCPF_RESPOND	 = TCPF_SYN_RECV,
+	DCCPF_CLOSEREQ	 = TCPF_FIN_WAIT1,
 	DCCPF_CLOSING	 = TCPF_CLOSING,
 	DCCPF_TIME_WAIT	 = TCPF_TIME_WAIT,
 	DCCPF_CLOSED	 = TCPF_CLOSE,
--- a/net/dccp/proto.c
+++ b/net/dccp/proto.c
@@ -139,6 +139,9 @@ const char *dccp_state_name(const int st
 	[DCCP_LISTEN]	  = "LISTEN",
 	[DCCP_RESPOND]	  = "RESPOND",
 	[DCCP_CLOSING]	  = "CLOSING",
+	[DCCP_CLOSEREQ]	  = "CLOSEREQ",
+	[DCCP_PASSIVE_1]  = "PASSIVE_1",
+	[DCCP_PASSIVE_2]  = "PASSIVE_2",
 	[DCCP_TIME_WAIT]  = "TIME_WAIT",
 	[DCCP_CLOSED]	  = "CLOSED",
 	};

Re: [PATCH 3/10]: Dedicated auxiliary states to support passive-close

[Ian McDonald]

On 7/13/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> [DCCP]: Dedicated auxiliary states to support passive-close
>
> Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Signed-off-by: Ian McDonald <ian.mcdonald@jandi.co.nz>
-- 
Web1: http://wand.net.nz/~iam4/
Web2: http://www.jandi.co.nz
Blog: http://iansblog.jandi.co.nz

Re: [PATCH 3/10]: Dedicated auxiliary states to support

[Arnaldo Carvalho de Melo]

Em Thu, Jul 12, 2007 at 03:23:39PM +0100, Gerrit Renker escreveu:
> [DCCP]: Dedicated auxiliary states to support passive-close
> 
> This adds two auxiliary states to deal with passive closes:
>   * PASSIVE_1 (reached from OPEN via reception of Close) and
>   * PASSIVE_2 (reached from OPEN via reception of CloseReq)


Perhaps we should rename PASSIVE_1 (magic number) to PASSIVE_CLOSEREQ
and PASSIVE_2 to PASSIVE_CLOSE, and also CLOSEREQ to ACTIVE_CLOSEREQ?

I'll defer these patches till we get some more discussion. There are
many more unrelated patches to work on after all :)

- Arnaldo

> to the internal state machine.
> 
> The PASSIVE_1 and PASSIVE_2 states represent the two ways a passive-close
> can happen in DCCP.  The addition of these states is not merely to
> increase clarity. These states are required by the implementation to
> allow a receiver to process unread data before acknowledging the received
> connection-termination-request (i.e. the Close/CloseReq).
> 
> Fix:
> ----
>  This patch uses the PASSIVE_1 and PASSIVE_2 states to explicitly refer to
>  passive-closing states and to protect against external wipeout of internal
>  receive queues. The macroscopic behaviour is compatible with RFC 4340, but
>  without the auxiliary states, buggy, absurd and abnormal behaviour of the
>  socket API will continue. Which is to say, without auxiliary states it does
>  not work.
> 
>  As a consequence, the count of DCCP_STATE_MASK has been increased, to account
>  for the number of new states. 
> 
> 
> Implementation Note:
> --------------------
>  To keep compatibility with sk_stream_wait_connect():
>  * DCCP_CLOSING continues to map into TCP_CLOSING (since this state can be
>    either passive- or active-close)
>  * DCCP_CLOSEREQ maps into TCP_FIN_WAIT1 (since it is always active-close)
> 
>  It is tempting to keep the clever merge of the CLOSEREQ and CLOSING states.
>  However, with the number of possible state transitions, this would require:
> 
>  * quite a number of `if' statements to distinguish all predecessors of
>    the CLOSING state (server/client, active/passive, server timewait yes/no);
> 
>  * two different branches from the CLOSING state:
>    - to TIMEWAIT if it is not an active server-close without keeping timewait state
>    - to CLOSED otherwise (and requiring to receive a Close instead of a Reset).
>  
>  In light of this, I think it is cleaner to implement separate CLOSEREQ and CLOSING
>  states (this is done by the subsequent patches).
> 
> Further documentation is on http://www.erg.abdn.ac.uk/users/gerrit/dccp/docs/closing_states/
> 
> 
>                     Why this is necessary [can be removed]
>                     --------------------------------------
> 
>  The two states are not mentioned in the DCCP specification [RFC 4340, 8.4].
>  In fact, RFC 4340 is silent about passive-close. In a nutshell, suppose that
>  the CLOSE_WAIT and LAST_ACK states were removed from TCP, i.e. a FIN triggers
>  direct transition from ESTABLISHED to CLOSED - similar problem. 
>  The detailed account is as follows.
> 
>  The first problem lies in using inet_stream_connect():
> 
>  An absurd case arises if we let the protocol machinery, not the application,
>  go through the states. If the protocol machinery allows to proceed to DCCP_CLOSED
>  (which corresponds to TCP_CLOSE), connect() returns with -ECONNABORTED, even if
>  there was data in the receive queue. 
> 
>  Therefore, if we want to keep the useful abstraction of inet_stream_connect(), we
>  need to make sure that a passive close can not lead to DCCP_CLOSE via the protocol
>  machinery alone. 
> 
>  The second problem is also related to entering DCCP_CLOSED too early: 
> 
>  Even if connect() were fixed, if a passive-close can directly trigger a state 
>  transition from OPEN to DCCP_CLOSED, the receiver may not be able to read data
>  from its input queue. The reason is that the input queue has already been wiped, 
>  DCCP_CLOSED state has been entered, while the SOCK_DONE flag has not yet been set.
>  Consequently, any subsequent call to dccp_recvmsg terminates in -ENOTCONN, 
>  and this despite of a potentially full receive queue.
> 
> 
> Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> ---
>  include/linux/dccp.h |   22 +++++++++++++++++++++-
>  net/dccp/proto.c     |    3 +++
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> --- a/include/linux/dccp.h
> +++ b/include/linux/dccp.h
> @@ -230,16 +230,35 @@ enum dccp_state {
>  	DCCP_REQUESTING	= TCP_SYN_SENT,
>  	DCCP_LISTEN	= TCP_LISTEN,
>  	DCCP_RESPOND	= TCP_SYN_RECV,
> +	/*
> +	 *			Close states:
> +	 *
> +	 * CLOSEREQ is active-server close only.
> +	 * CLOSING can have three different meanings [RFC 4340, 8.3]:
> +	 *  a. Client has performed active-close, sent a Close to peer from
> +	 *     state OPEN or PARTOPEN, waiting for the final Reset
> +	 *     (in this case, SOCK_DONE = 1).
> +	 *  b. Client performs passive-Close, by receiving an CloseReq in OPEN
> +	 *     or PARTOPEN state. It sends a Close and waits for final Reset
> +	 *     (in this case, SOCK_DONE = 0).
> +	 *  c. Server decides to hold TIMEWAIT state & performs an active-close.
> +	 * To avoid erasing receive queues too early, the transitional states
> +	 * PASSIVE_1 (from OPEN => CLOSED) and PASSIVE_2 (from (PART)OPEN to
> +	 * CLOSING, corresponds to (b) above) are used.
> +	 */
> +	DCCP_CLOSEREQ	= TCP_FIN_WAIT1,
>  	DCCP_CLOSING	= TCP_CLOSING,
>  	DCCP_TIME_WAIT	= TCP_TIME_WAIT,
>  	DCCP_CLOSED	= TCP_CLOSE,
>  	/* Everything below here is specific to DCCP only */
>  	DCCP_INTRINSICS = TCP_MAX_STATES,
>  	DCCP_PARTOPEN,
> +	DCCP_PASSIVE_1,		/* any node receiving a Close  */
> +	DCCP_PASSIVE_2,		/* client receiving a CloseReq */
>  	DCCP_MAX_STATES
>  };
>  
> -#define DCCP_STATE_MASK 0xf
> +#define DCCP_STATE_MASK 0x1f
>  #define DCCP_ACTION_FIN (1<<7)
>  
>  enum {
> @@ -247,6 +266,7 @@ enum {
>  	DCCPF_REQUESTING = TCPF_SYN_SENT,
>  	DCCPF_LISTEN	 = TCPF_LISTEN,
>  	DCCPF_RESPOND	 = TCPF_SYN_RECV,
> +	DCCPF_CLOSEREQ	 = TCPF_FIN_WAIT1,
>  	DCCPF_CLOSING	 = TCPF_CLOSING,
>  	DCCPF_TIME_WAIT	 = TCPF_TIME_WAIT,
>  	DCCPF_CLOSED	 = TCPF_CLOSE,
> --- a/net/dccp/proto.c
> +++ b/net/dccp/proto.c
> @@ -139,6 +139,9 @@ const char *dccp_state_name(const int st
>  	[DCCP_LISTEN]	  = "LISTEN",
>  	[DCCP_RESPOND]	  = "RESPOND",
>  	[DCCP_CLOSING]	  = "CLOSING",
> +	[DCCP_CLOSEREQ]	  = "CLOSEREQ",
> +	[DCCP_PASSIVE_1]  = "PASSIVE_1",
> +	[DCCP_PASSIVE_2]  = "PASSIVE_2",
>  	[DCCP_TIME_WAIT]  = "TIME_WAIT",
>  	[DCCP_CLOSED]	  = "CLOSED",
>  	};
> -
> To unsubscribe from this list: send the line "unsubscribe dccp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/10]: Dedicated auxiliary states to support passive-close

[Gerrit Renker]

Quoting Arnaldo Carvalho de Melo:
|  > [DCCP]: Dedicated auxiliary states to support passive-close
|  > 
|  > This adds two auxiliary states to deal with passive closes:
|  >   * PASSIVE_1 (reached from OPEN via reception of Close) and
|  >   * PASSIVE_2 (reached from OPEN via reception of CloseReq)
|  
|  
|  Perhaps we should rename PASSIVE_1 (magic number) to PASSIVE_CLOSEREQ
|  and PASSIVE_2 to PASSIVE_CLOSE, and also CLOSEREQ to ACTIVE_CLOSEREQ?
No problems with the first two. With the third I was initially thinking
`this name is from the RFC', but the RFC didn't help much here and your
scheme makes the state much clearer - so, yes, I agree fully with that.
 
|  I'll defer these patches till we get some more discussion. There are
|  many more unrelated patches to work on after all :)
What kind of discussion were you thinking of: in a previous thread Ian and I
reached some agreement already that there is a problem here.

The problem really is in taking the RFC 4340 state machine at face value: if this
is done literally, short-lived connections will wipe the receive queue before the
userland application had a chance to read data; as documented on
	http://www.erg.abdn.ac.uk/users/gerrit/dccp/notes/closing_states/
If you don't believe this, it can readily be tested by writing a small test app which 
sends something like `hello world' to the server.
All data is seen on the wire, but never by the application.

A test app can be found in the directory misc/hello_world of
http://www.erg.abdn.ac.uk/users/gerrit/dccp/apps/dccp_applications_lib.tar.gz

Re: [PATCH 3/10]: Dedicated auxiliary states to support passive-close

[Ian McDonald]

On 7/13/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> [DCCP]: Dedicated auxiliary states to support passive-close
>
> This adds two auxiliary states to deal with passive closes:
>   * PASSIVE_1 (reached from OPEN via reception of Close) and
>   * PASSIVE_2 (reached from OPEN via reception of CloseReq)
> to the internal state machine.
>
> The PASSIVE_1 and PASSIVE_2 states represent the two ways a passive-close
> can happen in DCCP.  The addition of these states is not merely to
> increase clarity. These states are required by the implementation to
> allow a receiver to process unread data before acknowledging the received
> connection-termination-request (i.e. the Close/CloseReq).
>
> Fix:
> ----
>  This patch uses the PASSIVE_1 and PASSIVE_2 states to explicitly refer to
>  passive-closing states and to protect against external wipeout of internal
>  receive queues. The macroscopic behaviour is compatible with RFC 4340, but
>  without the auxiliary states, buggy, absurd and abnormal behaviour of the
>  socket API will continue. Which is to say, without auxiliary states it does
>  not work.
>
>  As a consequence, the count of DCCP_STATE_MASK has been increased, to account
>  for the number of new states.
>
>
> Implementation Note:
> --------------------
>  To keep compatibility with sk_stream_wait_connect():
>  * DCCP_CLOSING continues to map into TCP_CLOSING (since this state can be
>    either passive- or active-close)
>  * DCCP_CLOSEREQ maps into TCP_FIN_WAIT1 (since it is always active-close)
>
>  It is tempting to keep the clever merge of the CLOSEREQ and CLOSING states.
>  However, with the number of possible state transitions, this would require:
>
>  * quite a number of `if' statements to distinguish all predecessors of
>    the CLOSING state (server/client, active/passive, server timewait yes/no);
>
>  * two different branches from the CLOSING state:
>    - to TIMEWAIT if it is not an active server-close without keeping timewait state
>    - to CLOSED otherwise (and requiring to receive a Close instead of a Reset).
>
>  In light of this, I think it is cleaner to implement separate CLOSEREQ and CLOSING
>  states (this is done by the subsequent patches).
>
> Further documentation is on http://www.erg.abdn.ac.uk/users/gerrit/dccp/docs/closing_states/
>
>
> Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>

This was signed off by me on Sep 6th but seems to have got lost so:
Signed-off-by: Ian McDonald <ian.mcdonald@jandi.co.nz>