From patchwork Sun Aug 15 14:47:17 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Eric Dumazet X-Patchwork-Id: 61750 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 4DAC2B70A7 for ; Mon, 16 Aug 2010 01:41:26 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757752Ab0HOPkz (ORCPT ); Sun, 15 Aug 2010 11:40:55 -0400 Received: from mail-wy0-f174.google.com ([74.125.82.174]:57750 "EHLO mail-wy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757560Ab0HOPky (ORCPT ); Sun, 15 Aug 2010 11:40:54 -0400 Received: by wyb32 with SMTP id 32so4701481wyb.19 for ; Sun, 15 Aug 2010 08:40:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:subject:from:to:cc :in-reply-to:references:content-type:date:message-id:mime-version :x-mailer:content-transfer-encoding; bh=SLJVEm5Pr9th5UQvtNpcSBNs46ganjh7ZswBQzxix/Q=; b=nxKwz6eAcD4R6Xwxubtk2TwSc2UBe1xbYgrIPHAhm8VqEtSk6sKjaocUC0hqFIYbGL ez4qIJK0WikWc8XVCGAZaSsTby/QY9DZTwPIthktEyCbg+Yw7LYgx28Bcq2QvvhK5Wl8 Du5FjzFqn1q+TF9JumtMOs7wz3NB128palBVE= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=subject:from:to:cc:in-reply-to:references:content-type:date :message-id:mime-version:x-mailer:content-transfer-encoding; b=NOcLXtxMvsBg4qmUH0SpJxslfyfMlazAb664f8POmGT2J38jpUKnT6Pd8mFpYvNsw9 RKXkni8eQaYbL9Uu0uwyyJ06de69zdnWDia+jy9gb1OlyZZVPqAIT43L2hILVjwDTeUS 6dfxEvxtyOLrzegQI7epViyVu/tt2zKLxsC/E= Received: by 10.227.157.211 with SMTP id c19mr452986wbx.212.1281883642643; Sun, 15 Aug 2010 07:47:22 -0700 (PDT) Received: from [127.0.0.1] ([85.17.35.125]) by mx.google.com with ESMTPS id e31sm4428128wbe.17.2010.08.15.07.47.19 (version=SSLv3 cipher=RC4-MD5); Sun, 15 Aug 2010 07:47:21 -0700 (PDT) Subject: Re: [GIT] Networking From: Eric Dumazet To: David Miller Cc: torvalds@linux-foundation.org, akpm@linux-foundation.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Patrick McHardy In-Reply-To: <1281869722.2942.20.camel@edumazet-laptop> References: <20100812.161050.246523792.davem@davemloft.net> <20100814.220945.232761341.davem@davemloft.net> <1281869722.2942.20.camel@edumazet-laptop> Date: Sun, 15 Aug 2010 16:47:17 +0200 Message-ID: <1281883637.2942.42.camel@edumazet-laptop> 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 Le dimanche 15 août 2010 à 12:55 +0200, Eric Dumazet a écrit : > We have one lock per cpu, and only one cpu can possibly lock its > associated lock under softirq. So the usual lockdep check, warning a > lock is taken with BH enabled, while same lock was taken inside softirq > handler is triggering a false positive here. > > I believe no existing lockdep annotation can instruct lockdep this use > is OK, I guess we have following choice : > > 1) Mask BH again, using xt_info_wrlock_lockdep(cpu) instead of > xt_info_wrlock(cpu). > > xt_info_wrlock_lockdep() being a variant, that disables BH in case > CONFIG_PROVE_LOCKING=y > > 2) temporally switch off lockdep in get_counters(), using a > lockdep_off()/lockdep_on() pair, and a comment why this is necessary. > In any case, here is patch implementing the later CC Patrick, our netfilter maintainer... Maybe lockdep rules could be improved to take care of this later ? Thanks [PATCH] netfilter: {ip,ip6,arp}_tables: avoid lockdep false positive After commit 24b36f019 (netfilter: {ip,ip6,arp}_tables: dont block bottom half more than necessary), lockdep can raise a warning because we attempt to lock a spinlock with BH enabled, while the same lock is usually locked by another cpu in a softirq context. In this use case, the lockdep splat is a false positive, because the BH disabling only matters for one cpu for a given lock (we use one lock per cpu). Use lockdep_off()/lockdep_on() around the problematic section to avoid the splat. Reported-by: Linus Torvalds Diagnosed-by: David S. Miller Signed-off-by: Eric Dumazet CC: Patrick McHardy --- net/ipv4/netfilter/arp_tables.c | 3 +++ net/ipv4/netfilter/ip_tables.c | 3 +++ net/ipv6/netfilter/ip6_tables.c | 3 +++ 3 files changed, 9 insertions(+) -- 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/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c index 6bccba3..b4f7ebf 100644 --- a/net/ipv4/netfilter/arp_tables.c +++ b/net/ipv4/netfilter/arp_tables.c @@ -729,8 +729,10 @@ static void get_counters(const struct xt_table_info *t, local_bh_enable(); /* Processing counters from other cpus, we can let bottom half enabled, * (preemption is disabled) + * We must turn off lockdep to avoid a false positive. */ + lockdep_off(); for_each_possible_cpu(cpu) { if (cpu == curcpu) continue; @@ -743,6 +745,7 @@ static void get_counters(const struct xt_table_info *t, } xt_info_wrunlock(cpu); } + lockdep_on(); put_cpu(); } diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c index c439721..dc5b2fd 100644 --- a/net/ipv4/netfilter/ip_tables.c +++ b/net/ipv4/netfilter/ip_tables.c @@ -903,8 +903,10 @@ get_counters(const struct xt_table_info *t, local_bh_enable(); /* Processing counters from other cpus, we can let bottom half enabled, * (preemption is disabled) + * We must turn off lockdep to avoid a false positive. */ + lockdep_off(); for_each_possible_cpu(cpu) { if (cpu == curcpu) continue; @@ -917,6 +919,7 @@ get_counters(const struct xt_table_info *t, } xt_info_wrunlock(cpu); } + lockdep_on(); put_cpu(); } diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c index 5359ef4..fb55443 100644 --- a/net/ipv6/netfilter/ip6_tables.c +++ b/net/ipv6/netfilter/ip6_tables.c @@ -916,8 +916,10 @@ get_counters(const struct xt_table_info *t, local_bh_enable(); /* Processing counters from other cpus, we can let bottom half enabled, * (preemption is disabled) + * We must turn off lockdep to avoid a false positive. */ + lockdep_off(); for_each_possible_cpu(cpu) { if (cpu == curcpu) continue; @@ -930,6 +932,7 @@ get_counters(const struct xt_table_info *t, } xt_info_wrunlock(cpu); } + lockdep_on(); put_cpu(); }