Wei,
thank you for reporting this. There is a differentiation here.
> This patch fix two problem in REQUEST state while received invalid packets.
>
> 1. If invalid Dccp-DataAck packets is received in the REQUEST state, the
> socket just send back Dccp-Rest with invalid packet error, but the
> socket is not reset, Dccp-Request will be continue retranmitted. The
> procedure is like this:
>
> Endpoint A Endpint B
> <----------------- Request
> DataAck -----------------> <----------------- Reset (invalid
> packet)
> <----------------- Request (retranmit)
>
This is not a bug but as far as I know correct behaviour. The DataAck
could be from a previous incarnation of the same connection, or it could
be an erratic packet. RFC 4340, 5.6 says about this error code
"Packet Error"
A valid packet arrived with unexpected type. For example, a
DCCP-Data packet with valid header checksum and sequence numbers
arrived at a connection in the REQUEST state.
The client keeps on retransmitting the Request since this is a
requirement from 8.1.3; the number of retransmissions is limited
by net.dccp.default.request_retries.
Hence this case is in principle correct.
> 2. If sequence-invalid Dccp-Response is received in the REQUEST state,
> kernel will panic. This is because that after send reset when received
> sequence-invalid Dccp-Response, the state of socket not be changed. The
> procedure is like this:
>
> Endpoint A Endpint B
> <----------------- Request
> Response -----------------> (sequence-invalid)
> <----------------- Reset (invalid packet)
> Response -----------------> kernel panic
>
> Pid: 0, comm: swapper Not tainted (2.6.26 #1)
> EIP: 0060:[<c05b426d>] EFLAGS: 00010282 CPU: 0
> EIP is at skb_release_all+0x6/0x7e
> EAX: 00000000 EBX: 00000000 ECX: 00000046 EDX: c4fdd000
> ESI: c79aad80 EDI: c4fdd000 EBP: c077ee0c ESP: c077ee08
> DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> Process swapper (pid: 0, ti=c077e000 task=c0700340 task.ti=c0734000)
> Stack: 00000000 c077ee18 c05b3bf3 c79a9234 c077ee58 c8ab1db5 c79aad80 c4fdd000
> 02000015 c79aada0 00000c3c 00000854 c79aad80 c077ee50 c04260db c8ac176e
> 0059b20e c4fdd000 c79aad80 c4fdd000 c077ee78 c8ac06e3 0000001c c79a9234
> Call Trace:
> [<c05b3bf3>] ? __kfree_skb+0xb/0x66
<snip>
This is clearly a bug. From your analysis above and from looking at the code
the cause is in all likelihood:
1. sequence-invalid Response arrives;
2. retransmit timer is stopped and sk->sk_send_head is freed;
3. Reset (Packet Error) is sent as per the above diagram;
4. Next Response (valid or invalid) arrives
5. dccp_rcv_request_sent_state_process tries to __kfree_skb() the
sk_send_head (which had already been freed in (2))
==> panic
In your approach the socket is closed for each error case. This seems to
work, but there is a problem with it.
First, invalid or unexpected packets may arrive (as in the first
scenario). It is even possible that for example a DataAck stems from a
previous incarnation of the same connection. Closing the socket here
will kill the new connection.
Secondly, it also opens the door to an easy denial-of-service attack:
all an attacker needs to do is to send an unexpected packet type to the
client in state REQUEST, whereupon the client closes its socket.
The `unable_to_proceed' jump label was intended for the following
(single) purpose: if feature negotiation fails between endpoints (e.g.
if a CCID-3 only sender tries to connect to a CCID-2 only receiver).
Thus, I would like to suggest a different strategy: rather than closing
the socket whenever a (valid or invalid) packet is received in the
client-state REQUEST(ING), to move the chunk of code which
* stops the retransmit timer for the DCCP-Request and
* frees the sk_send_head containing the original skb
further below, after the error checking has been done.
This will ensure that this code is only executed when a valid Response
arrives. If no valid Response arrives within the time budget set by the
request_retries, the retransmit timer and send_head will eventually
be cleared via the timer code (dccp_write_err()).
I attach a sketch which I think will fix the bug. Could you give this a
try please? During the next two weeks it will be difficult for me to
do actual testing (writing this while travelling), but I will do my
best to respond.
--- a/net/dccp/input.c
+++ b/net/dccp/input.c
@@ -404,12 +404,6 @@ static int dccp_rcv_request_sent_state_process(struct sock *sk,
struct dccp_sock *dp = dccp_sk(sk);
long tstamp = dccp_timestamp();
- /* Stop the REQUEST timer */
- inet_csk_clear_xmit_timer(sk, ICSK_TIME_RETRANS);
- WARN_ON(sk->sk_send_head == NULL);
- __kfree_skb(sk->sk_send_head);
- sk->sk_send_head = NULL;
-
if (!between48(DCCP_SKB_CB(skb)->dccpd_ack_seq,
dp->dccps_awl, dp->dccps_awh)) {
dccp_pr_debug("invalid ackno: S.AWL=%llu, "
@@ -428,6 +422,12 @@ static int dccp_rcv_request_sent_state_process(struct sock *sk,
if (dccp_parse_options(sk, NULL, skb))
return 1;
+ /* Stop the REQUEST timer */
+ inet_csk_clear_xmit_timer(sk, ICSK_TIME_RETRANS);
+ WARN_ON(sk->sk_send_head == NULL);
+ __kfree_skb(sk->sk_send_head);
+ sk->sk_send_head = NULL;
+
/* Obtain usec RTT sample from SYN exchange (used by CCID 3) */
if (likely(dp->dccps_options_received.dccpor_timestamp_echo))
dp->dccps_syn_rtt = dccp_sample_rtt(sk, 10 * (tstamp -
