Re: Sequence Number Validation Bug Fixes 0/2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Samuel,

| This tool converts DCCP CCID 2 connections to TCP connections which can
| then be analyzed with Tcptrace (http://tcptrace.org). (I can provide a
| link to the tool if people are interested).
| 
Please do -- such a tool has been missing for a long time, and the idea to hook
it up with tcptrace is both original and efficient. Analysing Ack Vectors using
tcpdump/wireshark is very tedious, so anything that can help in debugging Ack
Vector traces is a great help.

| It turns out this behavior is due to a nasty bug in the dccp sequence
| number validation code (dccp_check_seqno in input.c). What happens is
| that the receiver legitimately stops incrementing it's ack number
| because packets from the sender are outside the valid Ack window (why
| this happens is another problem I'm still working on). DCCP should send
| a sync when this happens, but this sync needs to be rate-limited. If it
| isn't time to send a sync, the DCCP code currently returns 0 which
| indicates that the packet is good!! 
| 
Thanks a lot, you have found a real bug. I have edited your patch since
Evolution squeezed tabs into whitespaces, and added your Signed-off --
please check the attached result, as I would like to submit this soon.


With regard to not updating the Ack Number, I remember having seen something
related earlier. There is a relationship between the current sending rate, the
Ack Ratio, the cwnd, and the Sequence Window. The Ack window is advanced by the
GSS and depends on the local value of the Sequence Window. RFC 4340, 7.5.2
recommends 5 * max_packets_per_RTT as guideline for Sequence Window.

When I looked at this earlier I noted that the dynamic update of Ack Ratio,
cwnd, and Sequence Window (which are all inter-related, but are controlled
by different code paths) does not produce the expected interaction, therefore
Ack Ratio is currently always 1 and Sequence Window is also static and does
not change throughout the connection. 

It may be that both these shortcuts cause the behaviour you observed. The
Ack Ratio update is disabled in net/dccp/feat.c:dccp_hdlr_ack_ratio(), but
there is currently no code to predict (guess) what the sending rate will
be for the next 5 RTTs -- if such a heuristic is possible.

I am interested in suggestions to improve this.

| In examining this section of code, I also noticed that the the GSR is
| updated by any packet in the valid sequence number window. I believe
| this represents a minor bug, because it would allow the GSR to move
| backward as a result of an out of order packet that is still in the
| valid window. I am also sending a patch for that as a separate email.
|
Excellent catch, there is a second bug here. Apart from the clerical edits
(please consult Documentation/SubmittingPatches), there is a small
modification, sent in response to the other patch. Please check this also -
both are very useful bug fixes, I have already put them into the test tree at

    git://eden-feed.erg.abdn.ac.uk/dccp_exp	[subtree 'dccp']

    http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p=dccp_exp.git;a=commitdiff;h=adc0984a00912f2297f8272d3756a279900f6ba9
    http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p=dccp_exp.git;a=commitdiff;h=9eb39613b014cee24b0b8e390bd221df6737954c

Gerrit
dccp: fix return value for sequence-invalid packets

Currently dccp_check_seqno returns 0 (indicating a valid packet) if the
acknowledgment number is out of bounds and the sync that RFC 4340 mandates at
this point is currently being rate-limited. This function should return -1,
indicating an invalid packet.

Signed-off-by: Samuel Jero <sj323707@xxxxxxxx>
Acked-by: Gerrit Renker <gerrit@xxxxxxxxxxxxxx>
---
 net/dccp/input.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/net/dccp/input.c
+++ b/net/dccp/input.c
@@ -260,7 +260,7 @@ static int dccp_check_seqno(struct sock 
 		 */
 		if (time_before(now, (dp->dccps_rate_last +
 				      sysctl_dccp_sync_ratelimit)))
-			return 0;
+			return -1;
 
 		DCCP_WARN("Step 6 failed for %s packet, "
 			  "(LSWL(%llu) <= P.seqno(%llu) <= S.SWH(%llu)) and "

[Index of Archives]     [Linux Kernel]     [IETF DCCP]     [Linux Networking]     [Git]     [Security]     [Linux Assembly]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]

  Powered by Linux