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: Wed, 4 May 2016 11:51:50 -0500 From: Josh Poimboeuf <jpoimboe@...hat.com> To: Peter Zijlstra <peterz@...radead.org> Cc: Petr Mladek <pmladek@...e.com>, Jessica Yu <jeyu@...hat.com>, Jiri Kosina <jikos@...nel.org>, Miroslav Benes <mbenes@...e.cz>, Ingo Molnar <mingo@...hat.com>, Michael Ellerman <mpe@...erman.id.au>, Heiko Carstens <heiko.carstens@...ibm.com>, live-patching@...r.kernel.org, linux-kernel@...r.kernel.org, x86@...nel.org, linuxppc-dev@...ts.ozlabs.org, linux-s390@...r.kernel.org, Vojtech Pavlik <vojtech@...e.com>, Jiri Slaby <jslaby@...e.cz>, Chris J Arges <chris.j.arges@...onical.com>, Andy Lutomirski <luto@...nel.org> Subject: Re: barriers: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model On Wed, May 04, 2016 at 03:53:29PM +0200, Peter Zijlstra wrote: > On Wed, May 04, 2016 at 02:39:40PM +0200, Petr Mladek wrote: > > > + * This barrier also ensures that if another CPU goes through the > > > + * syscall barrier, sees the TIF_PATCH_PENDING writes in > > > + * klp_start_transition(), and calls klp_patch_task(), it also sees the > > > + * above write to the target state. Otherwise it can put the task in > > > + * the wrong universe. (oops, missed a "universe" -> "patch state" rename) > > > + */ > > > > By other words, it makes sure that klp_patch_task() will assign the > > right patch_state. Where klp_patch_task() could not be called > > before we set TIF_PATCH_PENDING in klp_start_transition(). > > > > > + smp_wmb(); > > > +} > > So I've not read the patch; but ending a function with an smp_wmb() > feels wrong. > > A wmb orders two stores, and I feel both stores should be well visible > in the same function. Yeah, I would agree with that. And also, it's probably a red flag that the barrier needs *three* paragraphs to describe the various cases its needed for. However, there are some complications: 1) The stores are in separate functions (which is a generally a good thing as it greatly helps the readability of the code). 2) Which stores are being ordered depends on whether the function is called in the enable path or the disable path. 3) Either way it actually orders *two* separate pairs of stores. Anyway I'm thinking I should move that barrier out of klp_init_transition() and into its callers. The stores will still be in separate functions but at least there will be better visibility of where the stores are occurring, and the comments can be a little more focused. -- Josh
Powered by blists - more mailing lists