From patchwork Fri May 29 13:03:45 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Ilpo_J=C3=A4rvinen?= X-Patchwork-Id: 27844 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@bilbo.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from ozlabs.org (ozlabs.org [203.10.76.45]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "mx.ozlabs.org", Issuer "CA Cert Signing Authority" (verified OK)) by bilbo.ozlabs.org (Postfix) with ESMTPS id 78FF1B7080 for ; Fri, 29 May 2009 23:03:56 +1000 (EST) Received: by ozlabs.org (Postfix) id 63B13DDF8C; Fri, 29 May 2009 23:03:56 +1000 (EST) Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by ozlabs.org (Postfix) with ESMTP id 01D2BDDF85 for ; Fri, 29 May 2009 23:03:55 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758870AbZE2NDr (ORCPT ); Fri, 29 May 2009 09:03:47 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1758795AbZE2NDq (ORCPT ); Fri, 29 May 2009 09:03:46 -0400 Received: from courier.cs.helsinki.fi ([128.214.9.1]:59513 "EHLO mail.cs.helsinki.fi" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758617AbZE2NDp (ORCPT ); Fri, 29 May 2009 09:03:45 -0400 Received: from wrl-59.cs.helsinki.fi (wrl-59.cs.helsinki.fi [128.214.166.179]) (AUTH: PLAIN cs-relay, TLS: TLSv1/SSLv3,256bits,AES256-SHA) by mail.cs.helsinki.fi with esmtp; Fri, 29 May 2009 16:03:46 +0300 id 00063DE3.4A1FDD32.0000464E Received: by wrl-59.cs.helsinki.fi (Postfix, from userid 50795) id 115CAA0096; Fri, 29 May 2009 16:03:46 +0300 (EEST) Received: from localhost (localhost [127.0.0.1]) by wrl-59.cs.helsinki.fi (Postfix) with ESMTP id 020E0A0091; Fri, 29 May 2009 16:03:45 +0300 (EEST) Date: Fri, 29 May 2009 16:03:45 +0300 (EEST) From: "=?ISO-8859-1?Q?Ilpo_J=E4rvinen?=" X-X-Sender: ijjarvin@wrl-59.cs.helsinki.fi To: David Miller cc: Netdev Subject: [PATCH net-next] tcp: fix loop in ofo handling code and reduce its complexity Message-ID: MIME-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Somewhat luckily, I was looking into these parts with very fine comb because I've made somewhat similar changes on the same area (conflicts that arose weren't that lucky though). The loop was very much overengineered recently in commit 915219441d566 (tcp: Use SKB queue and list helpers instead of doing it by-hand), while it basically just wants to know if there are skbs after 'skb'. Also it got broken because skb1 = skb->next got translated into skb1 = skb1->next (though abstracted) improperly. Note that 'skb1' is pointing to previous sk_buff than skb or NULL if at head. Two things went wrong: - We'll kfree 'skb' on the first iteration instead of the skbuff following 'skb' (it would require required SACK reneging to recover I think). - The list head case where 'skb1' is NULL is checked too early and the loop won't execute whereas it previously did. Conclusion, mostly revert the recent changes which makes the cset very messy looking but using proper accessor in the previous-like version. The effective changes against the original can be viewed with: git-diff 915219441d566f1da0caa0e262be49b666159e17^ \ net/ipv4/tcp_input.c | sed -n -e '57,70 p' Signed-off-by: Ilpo Järvinen --- net/ipv4/tcp_input.c | 28 +++++++++++----------------- 1 files changed, 11 insertions(+), 17 deletions(-) diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index ba34a23..2bdb0da 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -4481,26 +4481,20 @@ drop: __skb_queue_after(&tp->out_of_order_queue, skb1, skb); /* And clean segments covered by new one as whole. */ - if (skb1 && !skb_queue_is_last(&tp->out_of_order_queue, skb1)) { - struct sk_buff *n; + while (!skb_queue_is_last(&tp->out_of_order_queue, skb)) { + skb1 = skb_queue_next(&tp->out_of_order_queue, skb); - skb1 = skb_queue_next(&tp->out_of_order_queue, skb1); - skb_queue_walk_from_safe(&tp->out_of_order_queue, - skb1, n) { - if (!after(end_seq, TCP_SKB_CB(skb1)->seq)) - break; - if (before(end_seq, - TCP_SKB_CB(skb1)->end_seq)) { - tcp_dsack_extend(sk, - TCP_SKB_CB(skb1)->seq, - end_seq); - break; - } - __skb_unlink(skb1, &tp->out_of_order_queue); + if (!after(end_seq, TCP_SKB_CB(skb1)->seq)) + break; + if (before(end_seq, TCP_SKB_CB(skb1)->end_seq)) { tcp_dsack_extend(sk, TCP_SKB_CB(skb1)->seq, - TCP_SKB_CB(skb1)->end_seq); - __kfree_skb(skb1); + end_seq); + break; } + __skb_unlink(skb1, &tp->out_of_order_queue); + tcp_dsack_extend(sk, TCP_SKB_CB(skb1)->seq, + TCP_SKB_CB(skb1)->end_seq); + __kfree_skb(skb1); } add_sack: