: Convert Reset code into socket error number

[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.

---
 net/dccp/input.c |   48 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

@@ -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

Em Tue, Oct 02, 2007 at 08:38:31AM +1300, Ian McDonald escreveu:

Is that a problem?

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

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

Em Tue, Oct 02, 2007 at 09:07:24AM +1300, Ian McDonald escreveu:

Agreed, TCP_MAX_STATES, etc :-)

- Arnaldo

|  > +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.

|  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.