From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752314AbdJFQeV (ORCPT ); Fri, 6 Oct 2017 12:34:21 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:43000 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751817AbdJFQeT (ORCPT ); Fri, 6 Oct 2017 12:34:19 -0400 Date: Fri, 6 Oct 2017 09:34:14 -0700 From: "Paul E. McKenney" To: Paolo Abeni Cc: linux-kernel@vger.kernel.org, Josh Triplett , Steven Rostedt , "David S. Miller" , Eric Dumazet , Hannes Frederic Sowa , netdev@vger.kernel.org Subject: Re: [PATCH 0/4] RCU: introduce noref debug Reply-To: paulmck@linux.vnet.ibm.com References: <20171006133436.GY3521@linux.vnet.ibm.com> <1507302609.2793.16.camel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1507302609.2793.16.camel@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-GCONF: 00 x-cbid: 17100616-2213-0000-0000-000002271B2F X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007850; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000235; SDB=6.00927333; UDB=6.00466609; IPR=6.00707591; BA=6.00005623; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017422; XFM=3.00000015; UTC=2017-10-06 16:34:16 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17100616-2214-0000-0000-000057C5579B Message-Id: <20171006163414.GC3521@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-10-06_04:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1710060230 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Oct 06, 2017 at 05:10:09PM +0200, Paolo Abeni wrote: > Hi, > > On Fri, 2017-10-06 at 06:34 -0700, Paul E. McKenney wrote: > > On Fri, Oct 06, 2017 at 02:57:45PM +0200, Paolo Abeni wrote: > > > The networking subsystem is currently using some kind of long-lived > > > RCU-protected, references to avoid the overhead of full book-keeping. > > > > > > Such references - skb_dst() noref - are stored inside the skbs and can be > > > moved across relevant slices of the network stack, with the users > > > being in charge of properly clearing the relevant skb - or properly refcount > > > the related dst references - before the skb escapes the RCU section. > > > > > > We currently don't have any deterministic debug infrastructure to check > > > the dst noref usages - and the introduction of others noref artifact is > > > currently under discussion. > > > > > > This series tries to tackle the above introducing an RCU debug infrastructure > > > aimed at spotting incorrect noref pointer usage, in patch one. The > > > infrastructure is small and must be explicitly enabled via a newly introduced > > > build option. > > > > > > Patch two uses such infrastructure to track dst noref usage in the networking > > > stack. > > > > > > Patch 3 and 4 are bugfixes for small buglet found running this infrastructure > > > on basic scenarios. > > Thank you for the prompt reply! > > > > This patchset does not look like it handles rcu_read_lock() nesting. > > For example, given code like this: > > > > void foo(void) > > { > > rcu_read_lock(); > > rcu_track_noref(&key2, &noref2, true); > > do_something(); > > rcu_track_noref(&key2, &noref2, false); > > rcu_read_unlock(); > > } > > > > void bar(void) > > { > > rcu_read_lock(); > > rcu_track_noref(&key1, &noref1, true); > > do_something_more(); > > foo(); > > do_something_else(); > > rcu_track_noref(&key1, &noref1, false); > > rcu_read_unlock(); > > } > > > > void grill(void) > > { > > foo(); > > } > > > > It looks like foo()'s rcu_read_unlock() will complain about key1. > > You could remove foo()'s rcu_read_lock() and rcu_read_unlock(), but > > that will break the call from grill(). > > Actually the code should cope correctly with your example; when foo()'s > rcu_read_unlock() is called, 'cache' contains: > > { { &key1, &noref1, 1}, // ... > > and when the related __rcu_check_noref() is invoked preempt_count() is > 2 - because the check is called before decreasing the preempt counter. > > In the main loop inside __rcu_check_noref() we will hit always the > 'continue' statement because 'cache->store[i].nesting != nesting', so > no warn will be triggered. You are right, it was too early, and my example wasn't correct. How about this one? void foo(void (*f)(struct s *sp), struct s **spp) { rcu_read_lock(); rcu_track_noref(&key2, &noref2, true); f(spp); rcu_track_noref(&key2, &noref2, false); rcu_read_unlock(); } void barcb(struct s **spp) { *spp = &noref3; rcu_track_noref(&key3, *spp, true); } void bar(void) { struct s *sp; rcu_read_lock(); rcu_track_noref(&key1, &noref1, true); do_something_more(); foo(barcb, &sp); do_something_else(sp); rcu_track_noref(&key3, sp, false); rcu_track_noref(&key1, &noref1, false); rcu_read_unlock(); } void grillcb(struct s **spp) { *spp } void grill(void) { foo(); } How does the user select the key argument? It looks like someone can choose to just pass in NULL -- is that the intent, or am I confused about this as well? > > Or am I missing something subtle here? Given patch 3/4, I suspect not... > > The problem with the code in patch 3/4 is different; currently > ip_route_input_noref() is basically doing: > > rcu_read_lock(); > > rcu_track_noref(&key1, &noref1, true); > > rcu_read_unlock(); > > So the rcu lock there silence any RCU based check inside > ip_route_input_noref() but does not really protect the noref dst. > > Please let me know if the above clarify the scenario. OK. I am also concerned about false negatives when the user invokes rcu_track_noref(..., false) but then leaks the pointer anyway. Or is there something you are doing that catches this that I am missing? Thanx, Paul