lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 13 Sep 2015 12:06:33 +0200 From: Patrick Marlier <patrick.marlier@...il.com> To: paulmck@...ux.vnet.ibm.com Cc: Steven Rostedt <rostedt@...dmis.org>, NeilBrown <neilb@...e.de>, linux-kernel@...r.kernel.org, mingo@...nel.org, laijs@...fujitsu.com, dipankar@...ibm.com, akpm@...ux-foundation.org, mathieu.desnoyers@...icios.com, josh@...htriplett.org, tglx@...utronix.de, peterz@...radead.org, dhowells@...hat.com, edumazet@...gle.com, dvhart@...ux.intel.com, fweisbec@...il.com, oleg@...hat.com, bobby.prani@...il.com, wangyun@...ux.vnet.ibm.com Subject: Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage On 09/12/2015 01:05 AM, Paul E. McKenney wrote: > On Tue, May 19, 2015 at 03:07:25PM -0700, Paul E. McKenney wrote: >> On Mon, May 18, 2015 at 09:43:21AM -0400, Steven Rostedt wrote: >>> On Mon, 18 May 2015 12:06:47 +1000 >>> NeilBrown <neilb@...e.de> wrote: >>> >>> >>>>> struct mddev { >>>>> ... >>>>> struct list_head disks; >>>>> ...} >>>>> >>>>> struct list_head { >>>>> struct list_head *next, *prev; >>>>> }; >>>>> >>>>> The tricky thing is that "list_entry_rcu" before and after the patch is >>>>> reading the same thing. >>>> >>>> No it isn't. >>>> Before the patch it is passed the address of the 'next' field. After the >>>> patch it is passed the contents of the 'next' field. >>> >>> Right. >>> >>>> >>>> >>>>> >>>>> However in your case, the change I proposed is probably wrong I trust >>>>> you on this side. :) What's your proposal to fix it with the rculist patch? >>>> >>>> What needs fixing? I don't see anything broken. >>>> >>>> Maybe there is something in this "rculist patch" that I'm missing. Can you >>>> point me at it? >>>> >>> >>> Probably some debugging tool like sparse notices that the assignment >>> isn't a true list entry and complains about it. In other words, I think >>> the real fix is to fix the debugging tool to ignore this, because the >>> code is correct, and this is a false positive failure, and is causing >>> more harm than good, because people are sending out broken patches due >>> to it. >> >> OK, finally did the history trawling that I should have done to begin with. >> >> Back in 2010, Arnd added the __rcu pointer checking in sparse. >> But the RCU list primitives were used on non-RCU-protected lists, so >> some casting pain was required to avoid sparse complaints. (Keep in >> mind that the list_head structure does not mark ->next with __rcu.) >> Arnd's workaround was to copy the pointer to the local stack, casting >> it to an __rcu pointer, then use rcu_dereference_raw() to do the needed >> traversal of an RCU-protected pointer. >> >> This of course resulted in an extraneous load from the stack, which >> Patrick noticed in his performance work, and which motivated him to send >> the patches. >> >> Perhaps what I should do is create an rcu_dereference_nocheck() for use >> in list traversals, that omits the sparse checking. That should get rid >> of both the sparse warnings and the strange casts. >> >> The code in md probably needs to change in any case, as otherwise we are >> invoking rcu_dereference_whatever() on a full struct list_head rather >> than on a single pointer. Or am I missing something here? > > Finally getting back to this one... > > I switched to lockless_dereference() instead of rcu_dereference_raw(), > and am running it through the testing gamut. Patrick, are you OK with > this change? Paul, This sounds good to me. It should fix the performance issue (will check with my benchmark). I think for drivers/md/bitmap.c:next_active_rdev() the problem was fixed but do you know if it also fixed for net/netfilter/core.c:nf_hook_slow()? Thanks. -- Pat -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists