From patchwork Sat Jun 23 05:37:31 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Dumazet X-Patchwork-Id: 166730 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 87797B6EEB for ; Sat, 23 Jun 2012 15:38:20 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757280Ab2FWFhj (ORCPT ); Sat, 23 Jun 2012 01:37:39 -0400 Received: from mail-we0-f174.google.com ([74.125.82.174]:36223 "EHLO mail-we0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751947Ab2FWFhh (ORCPT ); Sat, 23 Jun 2012 01:37:37 -0400 Received: by weyu7 with SMTP id u7so1635167wey.19 for ; Fri, 22 Jun 2012 22:37:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:from:to:cc:in-reply-to:references:content-type:date :message-id:mime-version:x-mailer:content-transfer-encoding; bh=yQWTrKbkfZKSj6zgJnWc0FhjkfZbpm+AQ9BqVbo5/ro=; b=MSO6veCTFQZSVrOuo09Kjs8eXPbUrSiAhVVYAqqtqFFBtolYMap82DOMO7bXaElljJ s4C/ICE+8bWXBlUUT9pyMc2xI9TNP5NvptQRHDovreClq+BtC6yfuMCBMlgGOc8P7YLT FJrIWRq3BrI8NLlzfaGLFzEKfSYyA2fRfWGgNL3DA+ao/msaMu6pvyOXScy/YH5BPVY8 rurqdEQj7mFXKoPSJCWbBjTbtVZlQWLaRDd4FjrWTpAVH+rn/p7VILCySjZfRMgQhHv6 B/bB5qs/7arHUUIves+LrHATOBAHdQJf0ao77IGOqb9WspJEclURRu7opBzDd+VVZ/j8 dLEQ== Received: by 10.180.80.74 with SMTP id p10mr9500198wix.10.1340429855856; Fri, 22 Jun 2012 22:37:35 -0700 (PDT) Received: from [172.28.88.117] ([74.125.122.49]) by mx.google.com with ESMTPS id eb8sm1101811wib.11.2012.06.22.22.37.32 (version=SSLv3 cipher=OTHER); Fri, 22 Jun 2012 22:37:34 -0700 (PDT) Subject: Re: Bug in net/ipv6/ip6_fib.c:fib6_dump_table() From: Eric Dumazet To: David Miller Cc: johunt@akamai.com, kaber@trash.net, dbavatar@gmail.com, netdev@vger.kernel.org, yoshfuji@linux-ipv6.org, jmorris@namei.org, pekkas@netcore.fi, kuznet@ms2.inr.ac.ru, linux-kernel@vger.kernel.org, Ben Greear In-Reply-To: <20120622.170237.1504103690155447356.davem@davemloft.net> References: <1340353746.4604.9502.camel@edumazet-glaptop> <4FE476A6.1050209@akamai.com> <1340388785.4604.11442.camel@edumazet-glaptop> <20120622.170237.1504103690155447356.davem@davemloft.net> Date: Sat, 23 Jun 2012 07:37:31 +0200 Message-ID: <1340429851.4604.11942.camel@edumazet-glaptop> Mime-Version: 1.0 X-Mailer: Evolution 2.28.3 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org From: Eric Dumazet > 1) Patrick McHardy has been inactive for a while, so do not expect > any insight from him. > > 2) Ben Greear isn't even on the CC: list of this discussion yet he > appears to be the person who reproduced the crash way back then > and is listed in the Tested-by tag of the commit. > > As a result we aren't likely to get any insight from the one person > who actually could hit the crash. > > I'm inclined to just revert simply because we have people active who > can reproduce regressions introduced by this change and nobody can > understand why the change is even necessary. Well, except that : I spent 3 hours trying to understand Alexey code and failed. All other /proc/net files don't have a such sophisticated walkers aware mechanism (easily DOSable by the way, if some guy opens 10.000 handles and suspend in the middle the dumps). cat /proc/net/tcp for example can display same socket twice or miss a socket, because a 'suspend/restart' remembers offsets/counts in a hash chain, not a pointer to 'next socket' The fix I submitted is a real one, based on my analysis and tests. Patrick patch was restarting the dump at the root of the tree, and setting skip = count was doing nothing at all, since all entries were dumped again. This is more a stable candidate fix. If someones smarter than me can find the real bug, then we certainly can revert Patrick patch ? [PATCH] ipv6: fib: fix fib dump restart Commit 2bec5a369ee79576a3 (ipv6: fib: fix crash when changing large fib while dumping it) introduced ability to restart the dump at tree root, but failed to skip correctly a count of already dumped entries. Code didn't match Patrick intent. We must skip exactly the number of already dumped entries. Note that like other /proc/net files or netlink producers, we could still dump some duplicates entries. Reported-by: Debabrata Banerjee Reported-by: Josh Hunt Signed-off-by: Eric Dumazet Cc: Patrick McHardy Cc: Ben Greear Cc: Alexey Kuznetsov --- net/ipv6/ip6_fib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c index 74c21b9..6083276 100644 --- a/net/ipv6/ip6_fib.c +++ b/net/ipv6/ip6_fib.c @@ -1349,8 +1349,8 @@ static int fib6_walk_continue(struct fib6_walker_t *w) if (w->leaf && fn->fn_flags & RTN_RTINFO) { int err; - if (w->count < w->skip) { - w->count++; + if (w->skip) { + w->skip--; continue; }