[PATCH 3/3]: Convert Reset code into socket error number

[PATCH 3/3]: Convert Reset code into socket error number

[Gerrit Renker]

[DCCP]: Convert Reset code into socket error number

This adds support for converting the 11 currently defined Reset codes into system
error numbers, which are stored in sk_err for further interpretation.

This makes the externally visible API behaviour similar to TCP, since a client
connecting to a non-existing port will experience ECONNREFUSED.
 
 * Code 0, Unspecified, is interpreted as non-error (0);
 * Code 1, Closed (normal termination), also maps into 0;
 * Code 2, Aborted, maps into "Connection reset by peer" (ECONNRESET);
 * Code 3, No Connection and
   Code 7, Connection Refused, map into "Connection refused" (ECONNREFUSED);
 * Code 4, Packet Error, maps into "No message of desired type" (ENOMSG);
 * Code 5, Option Error, maps into "Illegal byte sequence" (EILSEQ);
 * Code 6, Mandatory Error, maps into "Operation not supported on transport endpoint" (EOPNOTSUPP);
 * Code 8, Bad Service Code, maps into "Invalid request code" (EBADRQC);
 * Code 9, Too Busy, maps into "Too many users" (EUSERS);
 * Code 10, Bad Init Cookie, maps into "Invalid request descriptor" (EBADR);
 * Code 11, Aggression Penalty, maps into "Quota exceeded" (EDQUOT)
   which makes sense in terms of using more than the `fair share' of bandwidth.

The patch was found to solve the problem reported by Rémi Denis-Courmont - many thanks.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>   
---
 net/dccp/input.c |   48 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

--- a/net/dccp/input.c
+++ b/net/dccp/input.c
@@ -55,6 +55,42 @@ static void dccp_rcv_closereq(struct soc
 	dccp_send_close(sk, 0);
 }
 
+static u8 dccp_reset_code_convert(const u8 code)
+{
+	const u8 error_code[] = {
+	[DCCP_RESET_CODE_CLOSED]	     = 0,	/* normal termination */
+	[DCCP_RESET_CODE_UNSPECIFIED]	     = 0,	/* nothing known */
+	[DCCP_RESET_CODE_ABORTED]	     = ECONNRESET,
+
+	[DCCP_RESET_CODE_NO_CONNECTION]	     = ECONNREFUSED,
+	[DCCP_RESET_CODE_CONNECTION_REFUSED] = ECONNREFUSED,
+	[DCCP_RESET_CODE_TOO_BUSY]	     = EUSERS,
+	[DCCP_RESET_CODE_AGGRESSION_PENALTY] = EDQUOT,
+
+	[DCCP_RESET_CODE_PACKET_ERROR]	     = ENOMSG,
+	[DCCP_RESET_CODE_BAD_INIT_COOKIE]    = EBADR,
+	[DCCP_RESET_CODE_BAD_SERVICE_CODE]   = EBADRQC,
+	[DCCP_RESET_CODE_OPTION_ERROR]	     = EILSEQ,
+	[DCCP_RESET_CODE_MANDATORY_ERROR]    = EOPNOTSUPP,
+	};
+
+	return code <= DCCP_RESET_CODE_AGGRESSION_PENALTY? error_code[code] : 0;
+}
+
+static void dccp_rcv_reset(struct sock *sk, struct sk_buff *skb)
+{
+	u8 err = dccp_reset_code_convert(dccp_hdr_reset(skb)->dccph_reset_code);
+
+	sk->sk_err = err;
+
+	/* Queue the equivalent of TCP fin so that dccp_recvmsg exits the loop */
+	dccp_fin(sk, skb);
+
+	if (err && !sock_flag(sk, SOCK_DEAD))
+		sk_wake_async(sk, 0, POLL_ERR);
+	dccp_time_wait(sk, DCCP_TIME_WAIT, 0);
+}
+
 static void dccp_event_ack_recv(struct sock *sk, struct sk_buff *skb)
 {
 	struct dccp_sock *dp = dccp_sk(sk);
@@ -175,9 +211,8 @@ static int __dccp_rcv_established(struct
 		 *		S.state := TIMEWAIT
 		 *		Set TIMEWAIT timer
 		 *		Drop packet and return
-		*/
-		dccp_fin(sk, skb);
-		dccp_time_wait(sk, DCCP_TIME_WAIT, 0);
+		 */
+		dccp_rcv_reset(sk, skb);
 		return 0;
 	case DCCP_PKT_CLOSEREQ:
 		dccp_rcv_closereq(sk, skb);
@@ -505,12 +540,7 @@ int dccp_rcv_state_process(struct sock *
 	 *		Drop packet and return
 	*/
 	if (dh->dccph_type = DCCP_PKT_RESET) {
-		/*
-		 * Queue the equivalent of TCP fin so that dccp_recvmsg
-		 * exits the loop
-		 */
-		dccp_fin(sk, skb);
-		dccp_time_wait(sk, DCCP_TIME_WAIT, 0);
+		dccp_rcv_reset(sk, skb);
 		return 0;
 		/*
 		 *   Step 7: Check for unexpected packet types

Re: [PATCH 3/3]: Convert Reset code into socket error number

[Ian McDonald]

On 9/30/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> [DCCP]: Convert Reset code into socket error number
>
> This adds support for converting the 11 currently defined Reset codes into system
> error numbers, which are stored in sk_err for further interpretation.
>
> This makes the externally visible API behaviour similar to TCP, since a client
> connecting to a non-existing port will experience ECONNREFUSED.
>
>  * Code 0, Unspecified, is interpreted as non-error (0);
>  * Code 1, Closed (normal termination), also maps into 0;
>  * Code 2, Aborted, maps into "Connection reset by peer" (ECONNRESET);
>  * Code 3, No Connection and
>    Code 7, Connection Refused, map into "Connection refused" (ECONNREFUSED);
>  * Code 4, Packet Error, maps into "No message of desired type" (ENOMSG);
>  * Code 5, Option Error, maps into "Illegal byte sequence" (EILSEQ);
>  * Code 6, Mandatory Error, maps into "Operation not supported on transport endpoint" (EOPNOTSUPP);
>  * Code 8, Bad Service Code, maps into "Invalid request code" (EBADRQC);
>  * Code 9, Too Busy, maps into "Too many users" (EUSERS);
>  * Code 10, Bad Init Cookie, maps into "Invalid request descriptor" (EBADR);
>  * Code 11, Aggression Penalty, maps into "Quota exceeded" (EDQUOT)
>    which makes sense in terms of using more than the `fair share' of bandwidth.
>
> The patch was found to solve the problem reported by Rémi Denis-Courmont - many thanks.
>
> Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> ---
>  net/dccp/input.c |   48 +++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 9 deletions(-)
>
> --- a/net/dccp/input.c
> +++ b/net/dccp/input.c
> @@ -55,6 +55,42 @@ static void dccp_rcv_closereq(struct soc
>         dccp_send_close(sk, 0);
>  }
>
> +static u8 dccp_reset_code_convert(const u8 code)
> +{
> +       const u8 error_code[] = {
> +       [DCCP_RESET_CODE_CLOSED]             = 0,       /* normal termination */
> +       [DCCP_RESET_CODE_UNSPECIFIED]        = 0,       /* nothing known */
> +       [DCCP_RESET_CODE_ABORTED]            = ECONNRESET,
> +
> +       [DCCP_RESET_CODE_NO_CONNECTION]      = ECONNREFUSED,
> +       [DCCP_RESET_CODE_CONNECTION_REFUSED] = ECONNREFUSED,
> +       [DCCP_RESET_CODE_TOO_BUSY]           = EUSERS,
> +       [DCCP_RESET_CODE_AGGRESSION_PENALTY] = EDQUOT,
> +
> +       [DCCP_RESET_CODE_PACKET_ERROR]       = ENOMSG,
> +       [DCCP_RESET_CODE_BAD_INIT_COOKIE]    = EBADR,
> +       [DCCP_RESET_CODE_BAD_SERVICE_CODE]   = EBADRQC,
> +       [DCCP_RESET_CODE_OPTION_ERROR]       = EILSEQ,
> +       [DCCP_RESET_CODE_MANDATORY_ERROR]    = EOPNOTSUPP,
> +       };
> +

This array is inside the function so is local.

> +       return code <= DCCP_RESET_CODE_AGGRESSION_PENALTY? error_code[code] : 0;

and then you basically don't use half of it.
> +}

So I presume the reason for doing this is that half the codes don't
apply to reset.

However this looks ugly as you're only using half the array. I suspect
that you want this for other purposes too (it would make sense too).
As such then the array should be shifted out of the local function
into the main part of the file.

Ian

Re: [PATCH 3/3]: Convert Reset code into socket error number

[Arnaldo Carvalho de Melo]

Em Tue, Oct 02, 2007 at 08:38:31AM +1300, Ian McDonald escreveu:
> On 9/30/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> > [DCCP]: Convert Reset code into socket error number
> >
> > This adds support for converting the 11 currently defined Reset codes into system
> > error numbers, which are stored in sk_err for further interpretation.
> >
> > This makes the externally visible API behaviour similar to TCP, since a client
> > connecting to a non-existing port will experience ECONNREFUSED.
> >
> >  * Code 0, Unspecified, is interpreted as non-error (0);
> >  * Code 1, Closed (normal termination), also maps into 0;
> >  * Code 2, Aborted, maps into "Connection reset by peer" (ECONNRESET);
> >  * Code 3, No Connection and
> >    Code 7, Connection Refused, map into "Connection refused" (ECONNREFUSED);
> >  * Code 4, Packet Error, maps into "No message of desired type" (ENOMSG);
> >  * Code 5, Option Error, maps into "Illegal byte sequence" (EILSEQ);
> >  * Code 6, Mandatory Error, maps into "Operation not supported on transport endpoint" (EOPNOTSUPP);
> >  * Code 8, Bad Service Code, maps into "Invalid request code" (EBADRQC);
> >  * Code 9, Too Busy, maps into "Too many users" (EUSERS);
> >  * Code 10, Bad Init Cookie, maps into "Invalid request descriptor" (EBADR);
> >  * Code 11, Aggression Penalty, maps into "Quota exceeded" (EDQUOT)
> >    which makes sense in terms of using more than the `fair share' of bandwidth.
> >
> > The patch was found to solve the problem reported by Rémi Denis-Courmont - many thanks.
> >
> > Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> > ---
> >  net/dccp/input.c |   48 +++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 39 insertions(+), 9 deletions(-)
> >
> > --- a/net/dccp/input.c
> > +++ b/net/dccp/input.c
> > @@ -55,6 +55,42 @@ static void dccp_rcv_closereq(struct soc
> >         dccp_send_close(sk, 0);
> >  }
> >
> > +static u8 dccp_reset_code_convert(const u8 code)
> > +{
> > +       const u8 error_code[] = {
> > +       [DCCP_RESET_CODE_CLOSED]             = 0,       /* normal termination */
> > +       [DCCP_RESET_CODE_UNSPECIFIED]        = 0,       /* nothing known */
> > +       [DCCP_RESET_CODE_ABORTED]            = ECONNRESET,
> > +
> > +       [DCCP_RESET_CODE_NO_CONNECTION]      = ECONNREFUSED,
> > +       [DCCP_RESET_CODE_CONNECTION_REFUSED] = ECONNREFUSED,
> > +       [DCCP_RESET_CODE_TOO_BUSY]           = EUSERS,
> > +       [DCCP_RESET_CODE_AGGRESSION_PENALTY] = EDQUOT,
> > +
> > +       [DCCP_RESET_CODE_PACKET_ERROR]       = ENOMSG,
> > +       [DCCP_RESET_CODE_BAD_INIT_COOKIE]    = EBADR,
> > +       [DCCP_RESET_CODE_BAD_SERVICE_CODE]   = EBADRQC,
> > +       [DCCP_RESET_CODE_OPTION_ERROR]       = EILSEQ,
> > +       [DCCP_RESET_CODE_MANDATORY_ERROR]    = EOPNOTSUPP,
> > +       };
> > +
> 
> This array is inside the function so is local.

Is that a problem?
 
> > +       return code <= DCCP_RESET_CODE_AGGRESSION_PENALTY? error_code[code] : 0;
> 
> and then you basically don't use half of it.

Not really, the code is correct, look at the definition of those codes:

enum dccp_reset_codes {
        DCCP_RESET_CODE_UNSPECIFIED = 0,
        DCCP_RESET_CODE_CLOSED,
        DCCP_RESET_CODE_ABORTED,
        DCCP_RESET_CODE_NO_CONNECTION,
        DCCP_RESET_CODE_PACKET_ERROR,
        DCCP_RESET_CODE_OPTION_ERROR,
        DCCP_RESET_CODE_MANDATORY_ERROR,
        DCCP_RESET_CODE_CONNECTION_REFUSED,
        DCCP_RESET_CODE_BAD_SERVICE_CODE,
        DCCP_RESET_CODE_TOO_BUSY,
        DCCP_RESET_CODE_BAD_INIT_COOKIE,
        DCCP_RESET_CODE_AGGRESSION_PENALTY,
};

He used a "designated initializer", i.e. he said at which index in the
array it the value is to be set. So it really doesn't matter the order.
And when checking if the code was within the valid range he tested
against the last entry in the enum. Correct code. :-)

- Arnaldo

Re: [PATCH 3/3]: Convert Reset code into socket error number

[Ian McDonald]

On 10/2/07, Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:
> He used a "designated initializer", i.e. he said at which index in the
> array it the value is to be set. So it really doesn't matter the order.
> And when checking if the code was within the valid range he tested
> against the last entry in the enum. Correct code. :-)
>
> - Arnaldo
>
Yes you are correct. I misread the code.

However - I'd prefer another constant to match last in enum at the
same point as the declaration of the enum. This way if values are
added other code doesn't break. I've seen this done before quite a few
times. It is dangerous on relying on the last value when it's not
explicitly marked as the last value.

Ian
-- 
Web1: http://wand.net.nz/~iam4/
Web2: http://www.jandi.co.nz
Blog: http://iansblog.jandi.co.nz

Re: [PATCH 3/3]: Convert Reset code into socket error number

[Arnaldo Carvalho de Melo]

Em Tue, Oct 02, 2007 at 09:07:24AM +1300, Ian McDonald escreveu:
> On 10/2/07, Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:
> > He used a "designated initializer", i.e. he said at which index in the
> > array it the value is to be set. So it really doesn't matter the order.
> > And when checking if the code was within the valid range he tested
> > against the last entry in the enum. Correct code. :-)
> >
> > - Arnaldo
> >
> Yes you are correct. I misread the code.
> 
> However - I'd prefer another constant to match last in enum at the
> same point as the declaration of the enum. This way if values are
> added other code doesn't break. I've seen this done before quite a few
> times. It is dangerous on relying on the last value when it's not
> explicitly marked as the last value.

Agreed, TCP_MAX_STATES, etc :-)

- Arnaldo

Re: [PATCH 3/3]: Convert Reset code into socket error number

[Gerrit Renker]

|  > +static u8 dccp_reset_code_convert(const u8 code)
|  > +{
|  > +       const u8 error_code[] = {
|  > +       [DCCP_RESET_CODE_CLOSED]             = 0,       /* normal termination */
|  > +       [DCCP_RESET_CODE_UNSPECIFIED]        = 0,       /* nothing known */
|  > +       [DCCP_RESET_CODE_ABORTED]            = ECONNRESET,
|  > +
|  > +       [DCCP_RESET_CODE_NO_CONNECTION]      = ECONNREFUSED,
|  > +       [DCCP_RESET_CODE_CONNECTION_REFUSED] = ECONNREFUSED,
|  > +       [DCCP_RESET_CODE_TOO_BUSY]           = EUSERS,
|  > +       [DCCP_RESET_CODE_AGGRESSION_PENALTY] = EDQUOT,
|  > +
|  > +       [DCCP_RESET_CODE_PACKET_ERROR]       = ENOMSG,
|  > +       [DCCP_RESET_CODE_BAD_INIT_COOKIE]    = EBADR,
|  > +       [DCCP_RESET_CODE_BAD_SERVICE_CODE]   = EBADRQC,
|  > +       [DCCP_RESET_CODE_OPTION_ERROR]       = EILSEQ,
|  > +       [DCCP_RESET_CODE_MANDATORY_ERROR]    = EOPNOTSUPP,
|  > +       };
|  > +
|  
|  This array is inside the function so is local.
|  
|  > +       return code <= DCCP_RESET_CODE_AGGRESSION_PENALTY? error_code[code] : 0;
|  
|  and then you basically don't use half of it.
|  > +}
|
There is no problem declaring a local array here and I don't understand what you mean by
"basically don't use half of it" -- all are valid reset codes and all can appear on a
DCCP connection. 

|  So I presume the reason for doing this is that half the codes don't
|  apply to reset.
Please have a look at RFC 4340, 5.6.
 
|  As such then the array should be shifted out of the local function
|  into the main part of the file.
For a standalone table one would have to perform the index-bounds test each time which I think
is pretty bad, as the caller should not have to know about internals and bounds of Reset Codes.
Also we are not yet at the end of the story of reset codes - have you thought about dealing with
Data Dropped options - this is a different way of generating error codes: Drop code 1, "Application 
Not Listening", for instance has an effect similar to EPIPE (11.7). 
By using separate functions for translating the results, one can keep this separate.

NB: In the test tree I have changed the index-bounds test to 

 return code > DCCP_RESET_CODE_AGGRESSION_PENALTY ?  0 : error_code[code];

due to
	(a) coding conventions (Arnaldos email)
	(b) the reset codes 12-127 (reserved) and 128-255, CCID-specific codes, are
            to be interpreted as `0', "Unspecified" and is now clearer in the above.

Re: [PATCH 3/3]: Convert Reset code into socket error number

[Gerrit Renker]

|  Yes you are correct. I misread the code.
|  
|  However - I'd prefer another constant to match last in enum at the
|  same point as the declaration of the enum. This way if values are
|  added other code doesn't break. I've seen this done before quite a few
|  times. It is dangerous on relying on the last value when it's not
|  explicitly marked as the last value.
Sorry I was replying to emails in-order and this one was later in the pile, so
please disregard most of the previous email.
Your suggestion is a useful improvement, which means that I will rework and the
resubmit.