From: Thomas Gleixner <tglx@linutronix.de>
To: ira.weiny@intel.com, Ingo Molnar <mingo@redhat.com>,
Borislav Petkov <bp@alien8.de>, Andy Lutomirski <luto@kernel.org>,
Peter Zijlstra <peterz@infradead.org>
Cc: Ira Weiny <ira.weiny@intel.com>,
Dave Hansen <dave.hansen@linux.intel.com>,
x86@kernel.org, Dan Williams <dan.j.williams@intel.com>,
Vishal Verma <vishal.l.verma@intel.com>,
Andrew Morton <akpm@linux-foundation.org>,
Fenghua Yu <fenghua.yu@intel.com>,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-nvdimm@lists.01.org, linux-fsdevel@vger.kernel.org,
linux-mm@kvack.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH RFC V2 17/17] x86/entry: Preserve PKRS MSR across exceptions
Date: Thu, 23 Jul 2020 21:53:20 +0200 [thread overview]
Message-ID: <87r1t2vwi7.fsf@nanos.tec.linutronix.de> (raw)
In-Reply-To: <20200717072056.73134-18-ira.weiny@intel.com>
Ira,
ira.weiny@intel.com writes:
> ...
> // ref == 0
> dev_access_enable() // ref += 1 ==> disable protection
> irq()
> // enable protection
> // ref = 0
> _handler()
> dev_access_enable() // ref += 1 ==> disable protection
> dev_access_disable() // ref -= 1 ==> enable protection
> // WARN_ON(ref != 0)
> // disable protection
> do_pmem_thing() // all good here
> dev_access_disable() // ref -= 1 ==> 0 ==> enable protection
...
> First I'm not sure if adding this state to idtentry_state and having
> that state copied is the right way to go.
Adding the state to idtentry_state is fine at least for most interrupts
and exceptions. Emphasis on most.
#PF does not work because #PF can schedule.
> It seems like we should start passing this by reference instead of
> value. But for now this works as an RFC. Comments?
Works as in compiles, right?
static void noinstr idt_save_pkrs(idtentry_state_t state)
{
state.foo = 1;
}
How is that supposed to change the caller state? C programming basics.
> Second, I'm not 100% happy with having to save the reference count in
> the exception handler. It seems like a very ugly layering violation but
> I don't see a way around it at the moment.
That state is strict per task, right? So why do you want to store it
anywhere else that in task/thread storage. That solves your problem of
#PF scheduling nicely.
> Third, this patch has gone through a couple of revisions as I've had
> crashes which just don't make sense to me. One particular issue I've
> had is taking a MCE during memcpy_mcsafe causing my WARN_ON() to fire.
> The code path was a pmem copy and the ref count should have been
> elevated due to dev_access_enable() but why was
> idtentry_enter()->idt_save_pkrs() not called I don't know.
Because #MC does not go through idtentry_enter(). Neither do #NMI, #DB, #BP.
> Finally, it looks like the entry/exit code is being refactored into
> common code. So likely this is best handled somewhat there. Because
> this can be predicated on CONFIG_ARCH_HAS_SUPERVISOR_PKEYS and handled
> in a generic fashion. But that is a ways off I think.
The invocation of save/restore might be placed in generic code at least
for the common exception and interrupt entries.
> +static void noinstr idt_save_pkrs(idtentry_state_t state)
*state. See above.
> +#else
> +/* Define as macros to prevent conflict of inline and noinstr */
> +#define idt_save_pkrs(state)
> +#define idt_restore_pkrs(state)
Empty inlines do not need noinstr because they are optimized out. If you
want inlines in a noinstr section then use __always_inline
> /**
> * idtentry_enter - Handle state tracking on ordinary idtentries
> * @regs: Pointer to pt_regs of interrupted context
> @@ -604,6 +671,8 @@ idtentry_state_t noinstr idtentry_enter(struct pt_regs *regs)
> return ret;
> }
>
> + idt_save_pkrs(ret);
No. This really has no business to be called before the state is
established. It's not something horribly urgent and write_pkrs() is NOT
noinstr and invokes wrmsrl() which is subject to tracing.
> +
> + idt_restore_pkrs(state);
This one is placed correctly.
Thanks,
tglx
next prev parent reply other threads:[~2020-07-23 19:53 UTC|newest]
Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-17 7:20 [PATCH RFC V2 00/17] PKS: Add Protection Keys Supervisor (PKS) support ira.weiny
2020-07-17 7:20 ` [PATCH RFC V2 01/17] x86/pkeys: Create pkeys_internal.h ira.weiny
2020-07-17 7:20 ` [PATCH RFC V2 02/17] x86/fpu: Refactor arch_set_user_pkey_access() for PKS support ira.weiny
2020-07-17 8:54 ` Peter Zijlstra
2020-07-17 20:52 ` Ira Weiny
2020-07-20 9:14 ` Peter Zijlstra
2020-07-17 22:36 ` Dave Hansen
2020-07-20 9:13 ` Peter Zijlstra
2020-07-17 7:20 ` [PATCH RFC V2 03/17] x86/pks: Enable Protection Keys Supervisor (PKS) ira.weiny
2020-07-17 7:20 ` [PATCH RFC V2 04/17] x86/pks: Preserve the PKRS MSR on context switch ira.weiny
2020-07-17 8:31 ` Peter Zijlstra
2020-07-17 21:39 ` Ira Weiny
2020-07-17 8:59 ` Peter Zijlstra
2020-07-17 22:34 ` Ira Weiny
2020-07-20 9:15 ` Peter Zijlstra
2020-07-20 18:35 ` Ira Weiny
2020-07-17 7:20 ` [PATCH RFC V2 05/17] x86/pks: Add PKS kernel API ira.weiny
2020-07-17 7:20 ` [PATCH RFC V2 06/17] x86/pks: Add a debugfs file for allocated PKS keys ira.weiny
2020-07-17 7:20 ` [PATCH RFC V2 07/17] Documentation/pkeys: Update documentation for kernel pkeys ira.weiny
2020-07-17 7:20 ` [PATCH RFC V2 08/17] x86/pks: Add PKS Test code ira.weiny
2020-07-17 7:20 ` [PATCH RFC V2 09/17] memremap: Convert devmap static branch to {inc,dec} ira.weiny
2020-07-17 7:20 ` [PATCH RFC V2 10/17] fs/dax: Remove unused size parameter ira.weiny
2020-07-17 7:20 ` [PATCH RFC V2 11/17] drivers/dax: Expand lock scope to cover the use of addresses ira.weiny
2020-07-17 7:20 ` [PATCH RFC V2 12/17] memremap: Add zone device access protection ira.weiny
2020-07-17 9:10 ` Peter Zijlstra
2020-07-18 5:06 ` Ira Weiny
2020-07-20 9:16 ` Peter Zijlstra
2020-07-17 9:17 ` Peter Zijlstra
2020-07-18 5:51 ` Ira Weiny
2020-07-17 9:20 ` Peter Zijlstra
2020-07-17 7:20 ` [PATCH RFC V2 13/17] kmap: Add stray write protection for device pages ira.weiny
2020-07-17 9:21 ` Peter Zijlstra
2020-07-19 4:13 ` Ira Weiny
2020-07-20 9:17 ` Peter Zijlstra
2020-07-21 16:31 ` Ira Weiny
2020-07-17 7:20 ` [PATCH RFC V2 14/17] dax: Stray write protection for dax_direct_access() ira.weiny
2020-07-17 9:22 ` Peter Zijlstra
2020-07-19 4:41 ` Ira Weiny
2020-07-17 7:20 ` [PATCH RFC V2 15/17] nvdimm/pmem: Stray write protection for pmem->virt_addr ira.weiny
2020-07-17 7:20 ` [PATCH RFC V2 16/17] [dax|pmem]: Enable stray write protection ira.weiny
2020-07-17 9:25 ` Peter Zijlstra
2020-07-17 7:20 ` [PATCH RFC V2 17/17] x86/entry: Preserve PKRS MSR across exceptions ira.weiny
2020-07-17 9:30 ` Peter Zijlstra
2020-07-21 18:01 ` Ira Weiny
2020-07-21 19:11 ` Peter Zijlstra
2020-07-17 9:34 ` Peter Zijlstra
2020-07-17 10:06 ` Peter Zijlstra
2020-07-22 5:27 ` Ira Weiny
2020-07-22 9:48 ` Peter Zijlstra
2020-07-22 21:24 ` Ira Weiny
2020-07-23 20:08 ` Thomas Gleixner
2020-07-23 20:15 ` Thomas Gleixner
2020-07-24 17:23 ` Ira Weiny
2020-07-24 17:29 ` Andy Lutomirski
2020-07-24 19:43 ` Ira Weiny
2020-07-22 16:21 ` Andy Lutomirski
2020-07-23 16:18 ` Fenghua Yu
2020-07-23 16:23 ` Dave Hansen
2020-07-23 16:52 ` Fenghua Yu
2020-07-23 17:08 ` Andy Lutomirski
2020-07-23 17:30 ` Dave Hansen
2020-07-23 20:23 ` Thomas Gleixner
2020-07-23 20:22 ` Thomas Gleixner
2020-07-23 21:30 ` Andy Lutomirski
2020-07-23 22:14 ` Thomas Gleixner
2020-07-23 19:53 ` Thomas Gleixner [this message]
2020-07-23 22:04 ` Ira Weiny
2020-07-23 23:41 ` Thomas Gleixner
2020-07-24 21:24 ` Thomas Gleixner
2020-07-24 21:31 ` Thomas Gleixner
2020-07-25 0:09 ` Andy Lutomirski
2020-07-27 20:59 ` Ira Weiny
2020-07-24 22:19 ` [PATCH RFC V2 00/17] PKS: Add Protection Keys Supervisor (PKS) support Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87r1t2vwi7.fsf@nanos.tec.linutronix.de \
--to=tglx@linutronix.de \
--cc=akpm@linux-foundation.org \
--cc=bp@alien8.de \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@linux.intel.com \
--cc=fenghua.yu@intel.com \
--cc=ira.weiny@intel.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-nvdimm@lists.01.org \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=vishal.l.verma@intel.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).