Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1283499 > unrolled thread
| Started by | Dave Hansen <dave@sr71.net> |
|---|---|
| First post | 2015-12-04 02:20 +0100 |
| Last post | 2015-12-05 00:40 +0100 |
| Articles | 20 on this page of 55 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH 00/34] x86: Memory Protection Keys (v5) Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
[PATCH 09/34] x86, pkeys: store protection in high VMA flags Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
Re: [PATCH 09/34] x86, pkeys: store protection in high VMA flags Thomas Gleixner <tglx@linutronix.de> - 2015-12-08 15:20 +0100
[PATCH 32/34] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
[PATCH 29/34] x86: separate out LDT init from context init Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
[PATCH 31/34] x86, pkeys: allocation/free syscalls Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
[PATCH 12/34] signals, pkeys: notify userspace about protection key faults Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
[PATCH 20/34] x86, pkeys: differentiate instruction fetches Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
Re: [PATCH 20/34] x86, pkeys: differentiate instruction fetches Thomas Gleixner <tglx@linutronix.de> - 2015-12-08 19:20 +0100
[PATCH 19/34] x86, pkeys: optimize fault handling in access_error() Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
Re: [PATCH 19/34] x86, pkeys: optimize fault handling in access_error() Thomas Gleixner <tglx@linutronix.de> - 2015-12-08 19:20 +0100
[PATCH 18/34] mm: add gup flag to indicate "foreign" mm access Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
[PATCH 27/34] x86, pkeys: make mprotect_key() mask off additional vm_flags Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
Re: [PATCH 27/34] x86, pkeys: make mprotect_key() mask off additional vm_flags Thomas Gleixner <tglx@linutronix.de> - 2015-12-08 19:50 +0100
[PATCH 23/34] x86, pkeys: add Kconfig prompt to existing config option Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
Re: [PATCH 23/34] x86, pkeys: add Kconfig prompt to existing config option Thomas Gleixner <tglx@linutronix.de> - 2015-12-08 19:30 +0100
[PATCH 25/34] x86, pkeys: add arch_validate_pkey() Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
Re: [PATCH 25/34] x86, pkeys: add arch_validate_pkey() Thomas Gleixner <tglx@linutronix.de> - 2015-12-08 19:50 +0100
[PATCH 33/34] x86, pkeys: actually enable Memory Protection Keys in CPU Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
[PATCH 26/34] mm: implement new mprotect_key() system call Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
Re: [PATCH 26/34] mm: implement new mprotect_key() system call "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2015-12-05 08:00 +0100
Re: [PATCH 26/34] mm: implement new mprotect_key() system call Dave Hansen <dave@sr71.net> - 2015-12-07 17:50 +0100
Re: [PATCH 26/34] mm: implement new mprotect_key() system call "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2015-12-09 12:10 +0100
Re: [PATCH 26/34] mm: implement new mprotect_key() system call Dave Hansen <dave@sr71.net> - 2015-12-09 16:50 +0100
Re: [PATCH 26/34] mm: implement new mprotect_key() system call "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2015-12-09 17:50 +0100
Re: [PATCH 26/34] mm: implement new mprotect_key() system call Dave Hansen <dave@sr71.net> - 2015-12-09 18:10 +0100
Re: [PATCH 26/34] mm: implement new mprotect_key() system call "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2015-12-11 21:20 +0100
[PATCH 24/34] mm, multi-arch: pass a protection key in to calc_vm_flag_bits() Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
[PATCH 22/34] x86, pkeys: dump PTE pkey in /proc/pid/smaps Dave Hansen <dave@sr71.net> - 2015-12-04 02:20 +0100
[PATCH 14/34] x86, pkeys: add functions to fetch PKRU Dave Hansen <dave@sr71.net> - 2015-12-04 02:30 +0100
Re: [PATCH 14/34] x86, pkeys: add functions to fetch PKRU Thomas Gleixner <tglx@linutronix.de> - 2015-12-08 16:20 +0100
[PATCH 11/34] x86, pkeys: pass VMA down in to fault signal generation code Dave Hansen <dave@sr71.net> - 2015-12-04 02:30 +0100
[PATCH 02/34] x86, fpu: add placeholder for Processor Trace XSAVE state Dave Hansen <dave@sr71.net> - 2015-12-04 02:30 +0100
[PATCH 16/34] x86, mm: simplify get_user_pages() PTE bit handling Dave Hansen <dave@sr71.net> - 2015-12-04 02:30 +0100
Re: [PATCH 16/34] x86, mm: simplify get_user_pages() PTE bit handling Thomas Gleixner <tglx@linutronix.de> - 2015-12-08 19:10 +0100
Re: [PATCH 16/34] x86, mm: simplify get_user_pages() PTE bit handling Dave Hansen <dave@sr71.net> - 2015-12-08 19:40 +0100
[PATCH 10/34] x86, pkeys: arch-specific protection bits Dave Hansen <dave@sr71.net> - 2015-12-04 02:30 +0100
Re: [PATCH 10/34] x86, pkeys: arch-specific protection bitsy Thomas Gleixner <tglx@linutronix.de> - 2015-12-08 16:20 +0100
Re: [PATCH 10/34] x86, pkeys: arch-specific protection bitsy Dave Hansen <dave@sr71.net> - 2015-12-08 17:40 +0100
Re: [PATCH 10/34] x86, pkeys: arch-specific protection bitsy Thomas Gleixner <tglx@linutronix.de> - 2015-12-08 18:30 +0100
Re: [PATCH 10/34] x86, pkeys: arch-specific protection bitsy Dave Hansen <dave@sr71.net> - 2015-12-08 19:10 +0100
Re: [PATCH 10/34] x86, pkeys: arch-specific protection bitsy Thomas Gleixner <tglx@linutronix.de> - 2015-12-08 19:40 +0100
Re: [PATCH 10/34] x86, pkeys: arch-specific protection bitsy Thomas Gleixner <tglx@linutronix.de> - 2015-12-08 19:40 +0100
[PATCH 06/34] x86, pkeys: add PKRU xsave fields and data structure(s) Dave Hansen <dave@sr71.net> - 2015-12-04 02:30 +0100
[PATCH 05/34] x86, pkeys: define new CR4 bit Dave Hansen <dave@sr71.net> - 2015-12-04 02:30 +0100
[PATCH 01/34] mm, gup: introduce concept of "foreign" get_user_pages() Dave Hansen <dave@sr71.net> - 2015-12-04 02:30 +0100
[PATCH 17/34] x86, pkeys: check VMAs and PTEs for protection keys Dave Hansen <dave@sr71.net> - 2015-12-04 02:30 +0100
Re: [PATCH 17/34] x86, pkeys: check VMAs and PTEs for protection keys Thomas Gleixner <tglx@linutronix.de> - 2015-12-08 19:20 +0100
[PATCH 13/34] x86, pkeys: fill in pkey field in siginfo Dave Hansen <dave@sr71.net> - 2015-12-04 02:30 +0100
[PATCH 08/34] x86, pkeys: new page fault error code bit: PF_PK Dave Hansen <dave@sr71.net> - 2015-12-04 02:30 +0100
[PATCH 07/34] x86, pkeys: PTE bits for storing protection key Dave Hansen <dave@sr71.net> - 2015-12-04 02:30 +0100
[PATCH 04/34] x86, pkeys: cpuid bit definition Dave Hansen <dave@sr71.net> - 2015-12-04 02:30 +0100
Re: [PATCH 00/34] x86: Memory Protection Keys (v5) Dave Hansen <dave@sr71.net> - 2015-12-05 00:40 +0100
Re: [PATCH 00/34] x86: Memory Protection Keys (v5) Andy Lutomirski <luto@amacapital.net> - 2015-12-11 21:20 +0100
Re: [PATCH 00/34] x86: Memory Protection Keys (v5) Andy Lutomirski <luto@amacapital.net> - 2015-12-05 00:40 +0100
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
| From | "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> |
|---|---|
| Date | 2015-12-05 08:00 +0100 |
| Subject | Re: [PATCH 26/34] mm: implement new mprotect_key() system call |
| Message-ID | <qCeZ4-5ea-7@gated-at.bofh.it> |
| In reply to | #1283515 |
Dave,
On 12/04/2015 02:15 AM, Dave Hansen wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> mprotect_key() is just like mprotect, except it also takes a
> protection key as an argument. On systems that do not support
> protection keys, it still works, but requires that key=0.
> Otherwise it does exactly what mprotect does.
Is there a man page for this API?
Thanks,
Michael
> I expect it to get used like this, if you want to guarantee that
> any mapping you create can *never* be accessed without the right
> protection keys set up.
>
> pkey_deny_access(11); // random pkey
> int real_prot = PROT_READ|PROT_WRITE;
> ptr = mmap(NULL, PAGE_SIZE, PROT_NONE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
> ret = mprotect_key(ptr, PAGE_SIZE, real_prot, 11);
>
> This way, there is *no* window where the mapping is accessible
> since it was always either PROT_NONE or had a protection key set.
>
> We settled on 'unsigned long' for the type of the key here. We
> only need 4 bits on x86 today, but I figured that other
> architectures might need some more space.
>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: linux-api@vger.kernel.org
> ---
>
> b/arch/x86/include/asm/mmu_context.h | 10 +++++++--
> b/include/linux/pkeys.h | 7 +++++-
> b/mm/Kconfig | 7 ++++++
> b/mm/mprotect.c | 36 +++++++++++++++++++++++++++++------
> 4 files changed, 51 insertions(+), 9 deletions(-)
>
> diff -puN arch/x86/include/asm/mmu_context.h~pkeys-85-mprotect_pkey arch/x86/include/asm/mmu_context.h
> --- a/arch/x86/include/asm/mmu_context.h~pkeys-85-mprotect_pkey 2015-12-03 16:21:30.181877894 -0800
> +++ b/arch/x86/include/asm/mmu_context.h 2015-12-03 16:21:30.190878302 -0800
> @@ -4,6 +4,7 @@
> #include <asm/desc.h>
> #include <linux/atomic.h>
> #include <linux/mm_types.h>
> +#include <linux/pkeys.h>
>
> #include <trace/events/tlb.h>
>
> @@ -243,10 +244,14 @@ static inline void arch_unmap(struct mm_
> mpx_notify_unmap(mm, vma, start, end);
> }
>
> +#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
> +/*
> + * If the config option is off, we get the generic version from
> + * include/linux/pkeys.h.
> + */
> static inline int vma_pkey(struct vm_area_struct *vma)
> {
> u16 pkey = 0;
> -#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
> unsigned long vma_pkey_mask = VM_PKEY_BIT0 | VM_PKEY_BIT1 |
> VM_PKEY_BIT2 | VM_PKEY_BIT3;
> /*
> @@ -259,9 +264,10 @@ static inline int vma_pkey(struct vm_are
> */
> pkey = (vma->vm_flags >> vm_pkey_shift) &
> (vma_pkey_mask >> vm_pkey_shift);
> -#endif
> +
> return pkey;
> }
> +#endif
>
> static inline bool __pkru_allows_pkey(u16 pkey, bool write)
> {
> diff -puN include/linux/pkeys.h~pkeys-85-mprotect_pkey include/linux/pkeys.h
> --- a/include/linux/pkeys.h~pkeys-85-mprotect_pkey 2015-12-03 16:21:30.183877985 -0800
> +++ b/include/linux/pkeys.h 2015-12-03 16:21:30.190878302 -0800
> @@ -2,10 +2,10 @@
> #define _LINUX_PKEYS_H
>
> #include <linux/mm_types.h>
> -#include <asm/mmu_context.h>
>
> #ifdef CONFIG_ARCH_HAS_PKEYS
> #include <asm/pkeys.h>
> +#include <asm/mmu_context.h>
> #else /* ! CONFIG_ARCH_HAS_PKEYS */
>
> /*
> @@ -17,6 +17,11 @@ static inline bool arch_validate_pkey(in
> {
> return true;
> }
> +
> +static inline int vma_pkey(struct vm_area_struct *vma)
> +{
> + return 0;
> +}
> #endif /* ! CONFIG_ARCH_HAS_PKEYS */
>
> #endif /* _LINUX_PKEYS_H */
> diff -puN mm/Kconfig~pkeys-85-mprotect_pkey mm/Kconfig
> --- a/mm/Kconfig~pkeys-85-mprotect_pkey 2015-12-03 16:21:30.185878075 -0800
> +++ b/mm/Kconfig 2015-12-03 16:21:30.190878302 -0800
> @@ -673,3 +673,10 @@ config ARCH_USES_HIGH_VMA_FLAGS
> bool
> config ARCH_HAS_PKEYS
> bool
> +
> +config NR_PROTECTION_KEYS
> + int
> + # Everything supports a _single_ key, so allow folks to
> + # at least call APIs that take keys, but require that the
> + # key be 0.
> + default 1
> diff -puN mm/mprotect.c~pkeys-85-mprotect_pkey mm/mprotect.c
> --- a/mm/mprotect.c~pkeys-85-mprotect_pkey 2015-12-03 16:21:30.186878121 -0800
> +++ b/mm/mprotect.c 2015-12-03 16:21:30.191878347 -0800
> @@ -24,6 +24,7 @@
> #include <linux/migrate.h>
> #include <linux/perf_event.h>
> #include <linux/ksm.h>
> +#include <linux/pkeys.h>
> #include <asm/uaccess.h>
> #include <asm/pgtable.h>
> #include <asm/cacheflush.h>
> @@ -344,10 +345,13 @@ fail:
> return error;
> }
>
> -SYSCALL_DEFINE3(mprotect, unsigned long, start, size_t, len,
> - unsigned long, prot)
> +/*
> + * pkey=-1 when doing a legacy mprotect()
> + */
> +static int do_mprotect_pkey(unsigned long start, size_t len,
> + unsigned long prot, int pkey)
> {
> - unsigned long vm_flags, nstart, end, tmp, reqprot;
> + unsigned long nstart, end, tmp, reqprot;
> struct vm_area_struct *vma, *prev;
> int error = -EINVAL;
> const int grows = prot & (PROT_GROWSDOWN|PROT_GROWSUP);
> @@ -373,8 +377,6 @@ SYSCALL_DEFINE3(mprotect, unsigned long,
> if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
> prot |= PROT_EXEC;
>
> - vm_flags = calc_vm_prot_bits(prot, 0);
> -
> down_write(¤t->mm->mmap_sem);
>
> vma = find_vma(current->mm, start);
> @@ -407,7 +409,14 @@ SYSCALL_DEFINE3(mprotect, unsigned long,
>
> /* Here we know that vma->vm_start <= nstart < vma->vm_end. */
>
> - newflags = vm_flags;
> + /*
> + * If this is a vanilla, non-pkey mprotect, inherit the
> + * pkey from the VMA we are working on.
> + */
> + if (pkey == -1)
> + newflags = calc_vm_prot_bits(prot, vma_pkey(vma));
> + else
> + newflags = calc_vm_prot_bits(prot, pkey);
> newflags |= (vma->vm_flags & ~(VM_READ | VM_WRITE | VM_EXEC));
>
> /* newflags >> 4 shift VM_MAY% in place of VM_% */
> @@ -443,3 +452,18 @@ out:
> up_write(¤t->mm->mmap_sem);
> return error;
> }
> +
> +SYSCALL_DEFINE3(mprotect, unsigned long, start, size_t, len,
> + unsigned long, prot)
> +{
> + return do_mprotect_pkey(start, len, prot, -1);
> +}
> +
> +SYSCALL_DEFINE4(pkey_mprotect, unsigned long, start, size_t, len,
> + unsigned long, prot, int, pkey)
> +{
> + if (!arch_validate_pkey(pkey))
> + return -EINVAL;
> +
> + return do_mprotect_pkey(start, len, prot, pkey);
> +}
> _
> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Hansen <dave@sr71.net> |
|---|---|
| Date | 2015-12-07 17:50 +0100 |
| Subject | Re: [PATCH 26/34] mm: implement new mprotect_key() system call |
| Message-ID | <qD799-75n-25@gated-at.bofh.it> |
| In reply to | #1284470 |
[Multipart message — attachments visible in raw view] — view raw
On 12/04/2015 10:50 PM, Michael Kerrisk (man-pages) wrote: > On 12/04/2015 02:15 AM, Dave Hansen wrote: >> From: Dave Hansen <dave.hansen@linux.intel.com> >> >> mprotect_key() is just like mprotect, except it also takes a >> protection key as an argument. On systems that do not support >> protection keys, it still works, but requires that key=0. >> Otherwise it does exactly what mprotect does. > > Is there a man page for this API? Yep. Patch to man-pages source is attached. I actually broke it up in to a few separate pages. I was planning on submitting these after the patches themselves go upstream.
[toc] | [prev] | [next] | [standalone]
| From | "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> |
|---|---|
| Date | 2015-12-09 12:10 +0100 |
| Subject | Re: [PATCH 26/34] mm: implement new mprotect_key() system call |
| Message-ID | <qDKNb-7Bs-7@gated-at.bofh.it> |
| In reply to | #1285796 |
Hi Dave,
On 7 December 2015 at 17:44, Dave Hansen <dave@sr71.net> wrote:
> On 12/04/2015 10:50 PM, Michael Kerrisk (man-pages) wrote:
>> On 12/04/2015 02:15 AM, Dave Hansen wrote:
>>> From: Dave Hansen <dave.hansen@linux.intel.com>
>>>
>>> mprotect_key() is just like mprotect, except it also takes a
>>> protection key as an argument. On systems that do not support
>>> protection keys, it still works, but requires that key=0.
>>> Otherwise it does exactly what mprotect does.
>>
>> Is there a man page for this API?
>
> Yep.
Thanks!
> Patch to man-pages source is attached.
Better as inline, for review purposes.
> I actually broke it up in
> to a few separate pages.
Seems the right approach to me.
> I was planning on submitting these after the
> patches themselves go upstream.
Not a good idea. Reading and creating man pages has helped
me (and others) find a heap of design and implementation
bugs in APIs. Best that that happens before things hit
upstream.
Would you be willing to revise your man page (and possibly
your kernel patches) in the light of my comments below?
It would be better to do this sooner than later, since
I suspect I'll have a few more API comments as I review
future drafts of the page.
> commit ebb12643876810931ed23992f92b7c77c2c36883
> Author: Dave Hansen <dave.hansen@intel.com>
> Date: Mon Dec 7 08:42:57 2015 -0800
>
> pkeys
>
> diff --git a/man2/mprotect.2 b/man2/mprotect.2
> index ae305f6..a3c1e62 100644
> --- a/man2/mprotect.2
> +++ b/man2/mprotect.2
> @@ -38,16 +38,19 @@
> .\"
> .TH MPROTECT 2 2015-07-23 "Linux" "Linux Programmer's Manual"
> .SH NAME
> -mprotect \- set protection on a region of memory
> +mprotect, mprotect_key \- set protection on a region of memory
Elsewhere in your patch series (in a mail with subject
"mm: implement new mprotect_key() system call") I see:
+SYSCALL_DEFINE4(pkey_mprotect, unsigned long, start, size_t, len,
+ unsigned long, prot, int, pkey)
+{
+ if (!arch_validate_pkey(pkey))
+ return -EINVAL;
+
+ return do_mprotect_pkey(start, len, prot, pkey);
+}
And lower down in this patch series, I see "mprotect_pkey"!
What is the name of this system call supposed to be?
For what it's worth, I think "mprotect_pkey()" is the best
name (and secretly, you seem to as well, since we have at
the bottom of it all the internal function "do_mprotect_pkey()".
It signifies that this is a modified version of the base
functionality provided my mprotect(), and "pkey" is
consistent with the remainder of the APIs.
But, whatever name you do choose, please fix it in all
of your commit messages, otherwise reading the git
history gets very confusing.
> .SH SYNOPSIS
> .nf
> .B #include <sys/mman.h>
> .sp
> .BI "int mprotect(void *" addr ", size_t " len ", int " prot );
> +.BI "int mprotect_key(void *" addr ", size_t " len ", int " prot , " int " key);
> .fi
> .SH DESCRIPTION
> .BR mprotect ()
> -changes protection for the calling process's memory page(s)
> +and
> +.BR mprotect_key ()
> +change protection for the calling process's memory page(s)
> containing any part of the address range in the
> interval [\fIaddr\fP,\ \fIaddr\fP+\fIlen\fP\-1].
> .I addr
> @@ -74,10 +77,17 @@ The memory can be modified.
> .TP
> .B PROT_EXEC
> The memory can be executed.
> +.PP
> +.I key
> +is the protection or storage key to assign to the memory.
Why "protection or storage key" here? This phrasing seems a
little ambiguous to me, given that we also have a 'prot'
argument. I think it would be clearer just to say
"protection key". But maybe I'm missing something.
> +A key must be allocated with pkey_alloc () before it is
Please format syscall cross references as
.BR pkey_alloc (2)
> +passed to pkey_mprotect ().
> .SH RETURN VALUE
> On success,
> .BR mprotect ()
> -returns zero.
> +and
> +.BR mprotect_key ()
> +return zero.
> On error, \-1 is returned, and
> .I errno
> is set appropriately.
Are there no errors specific to mprotect_key()? Is there
an error if pkey is invalid? I see now that there is. That
EINVAL error needs documenting.
> diff --git a/man2/pkey_alloc.2 b/man2/pkey_alloc.2
> new file mode 100644
> index 0000000..980ce3e
> --- /dev/null
> +++ b/man2/pkey_alloc.2
> @@ -0,0 +1,72 @@
> +.\" Copyright (C) 2007 Michael Kerrisk <mtk.manpages@gmail.com>
> +.\" and Copyright (C) 1995 Michael Shields <shields@tembel.org>.
Michaels have many talents, but documenting kernel APIs
20 years ahead of their creation is not one of them, I believe.
Better replace this with the actual copyright holder and author
name.
> +.\" %%%LICENSE_START(VERBATIM)
> +.\" Permission is granted to make and distribute verbatim copies of this
> +.\" manual provided the copyright notice and this permission notice are
> +.\" preserved on all copies.
> +.\"
> +.\" Permission is granted to copy and distribute modified versions of this
> +.\" manual under the conditions for verbatim copying, provided that the
> +.\" entire resulting derived work is distributed under the terms of a
> +.\" permission notice identical to this one.
> +.\"
> +.\" Since the Linux kernel and libraries are constantly changing, this
> +.\" manual page may be incorrect or out-of-date. The author(s) assume no
> +.\" responsibility for errors or omissions, or for damages resulting from
> +.\" the use of the information contained herein. The author(s) may not
> +.\" have taken the same level of care in the production of this manual,
> +.\" which is licensed free of charge, as they might when working
> +.\" professionally.
> +.\"
> +.\" Formatted or processed versions of this manual, if unaccompanied by
> +.\" the source, must acknowledge the copyright and author of this work.
> +.\" %%%LICENSE_END
> +.\"
> +.\" Modified 2015-12-04 by Dave Hansen <dave@sr71.net>
This info should be in the copyright notice above.
> +.\"
> +.\"
> +.TH PKEY_ALLOC 2 2015-12-04 "Linux" "Linux Programmer's Manual"
> +.SH NAME
> +pkey_alloc, pkey_free \- allocate or free a protection key
> +.SH SYNOPSIS
> +.nf
> +.B #include <sys/mman.h>
> +.sp
> +.BI "int pkey_alloc(unsigned long" flags ", unsigned long " init_val);
If I understand correctly, 'init_val' is a mask of access rights
as per pkey_set(). If so, let's name the argument in this man page
"access_rights" also (or perhaps "init_access_rights" if you must,
but I think the shorter name is better. This helps the reader
to understand that we're talking about the same thing. It would be
good also to make the same change in the kernel code.
> +.BI "int pkey_free(int " pkey);
> +.fi
> +.SH DESCRIPTION
> +.BR pkey_alloc ()
> +and
> +.BR pkey_free ()
> +allow or disallow the calling process's to use the given
s/process's/process/
But should this actually be "thread"?
> +protection key for all protection-key-related operations.
> +
> +.PP
> +.I flags
> +is may contain zero or more disable operation:
s/is may/may/
> +.B PKEY_DISABLE_ACCESS
> +and/or
> +.B PKEY_DISABLE_WRITE
For the above two, please format as
.TP
.B PKEY_DISABLE_ACCESS
<Explanation of this flag>
.TP
.B PKEY_DISABLE_WRITE
<Explanation of this flag>
> +.SH RETURN VALUE
> +On success,
> +.BR pkey_alloc ()
> +and
> +.BR pkey_free ()
> +return zero.
The description of the success return for pkey_alloc() can't
be right. Doesn't it return a protection key?
> +On error, \-1 is returned, and
> +.I errno
> +is set appropriately.
> +.SH ERRORS
> +.TP
> +.B EINVAL
> +An invalid protection key, flag, or init_val was specified.
Better to write that last line as:
[[
.IR pkey ,
.IR flags ,
or
.I init_val [Or: access_rights]
is invalid.
]]
> +.TP
> +.B ENOSPC
> +All protection keys available for the current process have
> +been allocated.
So it seems to me that this page needs a discussion of the
limit that is involved here.
> +.SH SEE ALSO
> +.BR mprotect_pkey (2),
> +.BR pkey_get (2),
> +.BR pkey_set (2),
Remove trailing comma.
> diff --git a/man2/pkey_get.2 b/man2/pkey_get.2
> new file mode 100644
> index 0000000..4cfdea9
> --- /dev/null
> +++ b/man2/pkey_get.2
> @@ -0,0 +1,76 @@
> +.\" Copyright (C) 2007 Michael Kerrisk <mtk.manpages@gmail.com>
> +.\" and Copyright (C) 1995 Michael Shields <shields@tembel.org>.
Again, the copyright notice needs fixing.
> +.\" %%%LICENSE_START(VERBATIM)
> +.\" Permission is granted to make and distribute verbatim copies of this
> +.\" manual provided the copyright notice and this permission notice are
> +.\" preserved on all copies.
> +.\"
> +.\" Permission is granted to copy and distribute modified versions of this
> +.\" manual under the conditions for verbatim copying, provided that the
> +.\" entire resulting derived work is distributed under the terms of a
> +.\" permission notice identical to this one.
> +.\"
> +.\" Since the Linux kernel and libraries are constantly changing, this
> +.\" manual page may be incorrect or out-of-date. The author(s) assume no
> +.\" responsibility for errors or omissions, or for damages resulting from
> +.\" the use of the information contained herein. The author(s) may not
> +.\" have taken the same level of care in the production of this manual,
> +.\" which is licensed free of charge, as they might when working
> +.\" professionally.
> +.\"
> +.\" Formatted or processed versions of this manual, if unaccompanied by
> +.\" the source, must acknowledge the copyright and author of this work.
> +.\" %%%LICENSE_END
> +.\"
> +.\" Modified 2015-12-04 by Dave Hansen <dave@sr71.net>
> +.\"
> +.\"
> +.TH PKEY_GET 2 2015-12-04 "Linux" "Linux Programmer's Manual"
> +.SH NAME
> +pkey_get, pkey_set \- manage protection key access permissions
> +.SH SYNOPSIS
> +.nf
> +.B #include <sys/mman.h>
> +.sp
> +.BI "int pkey_get(int " pkey);
> +.BI "int pkey_set(int " pkey ", unsigned long " access_rights);
> +.fi
> +.SH DESCRIPTION
> +.BR pkey_get ()
> +and
> +.BR pkey_set ()
> +query or set the current set of rights for the calling
> +task for the given protection key.
Change "task" to "thread".
> +When rights for a key are disabled, any future access
> +to any memory region with that key set will generate
> +a SIGSEGV. The rights are local to the calling thread and
> +do not affect any other threads.
I think the last sentence could be simpler ("Access rights are
private to each thread."), or even removed, since you already
say above that these operations are per task (should be "per thread").
> +.PP
> +Upon entering any signal handler, the process is given a
> +default set of protection key rights which are separate from
> +the main thread's. Any calls to pkey_set () in a signal
s/signal/signal hander/
Format the reference to this system call as:
.BR pkey_set ()
> +will not persist upon a return to the calling process.
So, the preceding paragraph leaves me confused. And I'm wondering
if that confusion reflects some weirdness in the API design. But
I can't tell until I understand it better. These are my problems:
* You throw "process" and "thread" together in the explanation.
Is this simply a mistake? If it is not, the distinction
you are trying to draw by using the two different terms
is not made clear in the text.
* Your text ("separate from the main thread's") makes it
sound as though a signal handler is somehow invoked in a
different thread, which makes no sense. I suspect you
want to say something like this:
[[
When a signal handler is invoked, the thread is temporarily
given a default set of protection key rights. The thread's
protection key rights are restored when the signal handler
returns.
]]
Is that close to the truth?
* Change "a return to the calling process" to "when the
signal handler returns". Signal handlers are not "called"
by the program.
* There needs to be some explanation in this page of *why*
this special behavior occurs when signal handlers are
invoked.
And I have a question (and the answer probably should
be documented in the manual page). What happens when
one signal handler interrupts the execution of another?
Do pkey_set() calls in the first handler persist into the
second handler? I presume not, but it would be good to
be a little more explicit about this.
> +.PP
> +.I access_rights
> +is may contain zero or more disable operation:
s/is may/may/
> +.B PKEY_DISABLE_ACCESS
> +and/or
> +.B PKEY_DISABLE_WRITE
For the above two, please format as
[[
.TP
.B PKEY_DISABLE_ACCESS
<Explanation of this flag>
.TP
.B PKEY_DISABLE_WRITE
<Explanation of this flag>
]]
In various commit messages you use two alternative names:
PKEY_DENY_ACCESS and PKEY_DENY_WRITE. I assume bit rot here
as the the API has evolved. But please fix all of those
commit messages, so that the git history is more sensible.
> +.SH RETURN VALUE
> +On success,
> +.BR pkey_get ()
> +and
> +.BR pkey_set ()
> +return zero.
The success return value of pkey_get() is not correct.
Doesn't it return an access rights mask?
> +On error, \-1 is returned, and
> +.I errno
> +is set appropriately.
> +.SH ERRORS
> +.TP
> +.B EINVAL
> +An invalid protection key or access_rights was specified.
[[
.I access_rights
or
.I pkey
is invalid.
]]
> +.SH SEE ALSO
> +.BR mprotect_pkey (2),
> +.BR pkey_alloc (2),
> +.BR pkey_free (2),
Remove trailing comma.
So at the end of reading these pages, and delving
a little through the commit messages, I still don't
feel convinced that I understand what these APIs are
about. There's several things that I think still need
to be added to these pages:
* A general overview of why this functionality is useful.
* A note on which architectures support/will support
this functionality.
* Explanation of what a protection domain is.
* Explanation of how a process (thread?) changes its
protection domain.
* Explanation of the relationship between page permission
bits (PROT_READ/PROT_WRITE/PROTE_EXEC) and
PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE.
It's still not clear to me. Do the PKEY_* bits
override the PROT_* bits. Or, something else?
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Hansen <dave@sr71.net> |
|---|---|
| Date | 2015-12-09 16:50 +0100 |
| Subject | Re: [PATCH 26/34] mm: implement new mprotect_key() system call |
| Message-ID | <qDPa9-1NH-3@gated-at.bofh.it> |
| In reply to | #1287418 |
Hi Michael, Thanks for all the comments! I'll fix most of it when I post a new version of the manpage, but I have a few general questions. On 12/09/2015 03:08 AM, Michael Kerrisk (man-pages) wrote: >> >> +is the protection or storage key to assign to the memory. > > Why "protection or storage key" here? This phrasing seems a > little ambiguous to me, given that we also have a 'prot' > argument. I think it would be clearer just to say > "protection key". But maybe I'm missing something. x86 calls it a "protection key" while powerpc calls it a "storage key". They're called "protection keys" consistently inside the kernel. Should we just stick to one name in the manpages? > * A general overview of why this functionality is useful. Any preference on a central spot to do the general overview? Does it go in one of the manpages I'm already modifying, or a new one? > * A note on which architectures support/will support > this functionality. x86 only for now. We might get powerpc support down the road somewhere. > * Explanation of what a protection domain is. A protection domain is a unique view of memory and is represented by the value in the PKRU register. > * Explanation of how a process (thread?) changes its > protection domain. Changing protection domains is done by pkey_set() system call, or by using the WRPKRU instruction. The system call is preferred and less error-prone since it enforces that a protection is allocated before its access protection can be modified. > * Explanation of the relationship between page permission > bits (PROT_READ/PROT_WRITE/PROTE_EXEC) and > PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE. > It's still not clear to me. Do the PKEY_* bits > override the PROT_* bits. Or, something else? Protection keys add access restrictions in addition to existing page permissions. They can only take away access; they never grant additional access. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> |
|---|---|
| Date | 2015-12-09 17:50 +0100 |
| Subject | Re: [PATCH 26/34] mm: implement new mprotect_key() system call |
| Message-ID | <qDQ6e-2qX-11@gated-at.bofh.it> |
| In reply to | #1287583 |
Hi Dave, On 9 December 2015 at 16:48, Dave Hansen <dave@sr71.net> wrote: > Hi Michael, > > Thanks for all the comments! I'll fix most of it when I post a new > version of the manpage, but I have a few general questions. > > On 12/09/2015 03:08 AM, Michael Kerrisk (man-pages) wrote: >>> >>> +is the protection or storage key to assign to the memory. >> >> Why "protection or storage key" here? This phrasing seems a >> little ambiguous to me, given that we also have a 'prot' >> argument. I think it would be clearer just to say >> "protection key". But maybe I'm missing something. > > x86 calls it a "protection key" while powerpc calls it a "storage key". > They're called "protection keys" consistently inside the kernel. > > Should we just stick to one name in the manpages? Yes. But perhaps you could note the alternate name in the pkey(7) page. >> * A general overview of why this functionality is useful. > > Any preference on a central spot to do the general overview? Does it go > in one of the manpages I'm already modifying, or a new one? How about we add one more page, pkey(7) that gives the overview and also summarizes the APIs. >> * A note on which architectures support/will support >> this functionality. > > x86 only for now. We might get powerpc support down the road somewhere. Supported architectures can be listed in pkey(7). >> * Explanation of what a protection domain is. > > A protection domain is a unique view of memory and is represented by the > value in the PKRU register. Out something about this in pkey(7), but explain what you mean by a "unique view of memory". >> * Explanation of how a process (thread?) changes its >> protection domain. > > Changing protection domains is done by pkey_set() system call, or by > using the WRPKRU instruction. The system call is preferred and less > error-prone since it enforces that a protection is allocated before its > access protection can be modified. Details (perhaps not the WRPKRU bit) that should go in pkey(7). >> * Explanation of the relationship between page permission >> bits (PROT_READ/PROT_WRITE/PROTE_EXEC) and >> PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE. >> It's still not clear to me. Do the PKEY_* bits >> override the PROT_* bits. Or, something else? > > Protection keys add access restrictions in addition to existing page > permissions. They can only take away access; they never grant > additional access. This belongs in pkey(7) :-). Cheers, Michael -- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Linux/UNIX System Programming Training: http://man7.org/training/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Hansen <dave@sr71.net> |
|---|---|
| Date | 2015-12-09 18:10 +0100 |
| Subject | Re: [PATCH 26/34] mm: implement new mprotect_key() system call |
| Message-ID | <qDQpA-2N1-19@gated-at.bofh.it> |
| In reply to | #1287637 |
On 12/09/2015 08:45 AM, Michael Kerrisk (man-pages) wrote: >>> >> * Explanation of what a protection domain is. >> > >> > A protection domain is a unique view of memory and is represented by the >> > value in the PKRU register. > Out something about this in pkey(7), but explain what you mean by a > "unique view of memory". Let's say there are only two protection keys: 0 and 1. There are two disable bits per protection key (Access and Write Disable), so a two-key PKRU looks like: | PKEY0 | PKEY1 | | AD0 | WD0 | AD1 | WD1 | In this example, there are 16 possible protection domains, one for each possible combination of the 4 rights-disable bits. "Changing a protection domain" would mean changing (setting or clearing) the value of any of those 4 bits. Each unique value of PKRU represents a view of memory, or unique protection domain. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> |
|---|---|
| Date | 2015-12-11 21:20 +0100 |
| Subject | Re: [PATCH 26/34] mm: implement new mprotect_key() system call |
| Message-ID | <qECkx-LZ-1@gated-at.bofh.it> |
| In reply to | #1287662 |
On 12/09/2015 06:05 PM, Dave Hansen wrote: > On 12/09/2015 08:45 AM, Michael Kerrisk (man-pages) wrote: >>>>>> * Explanation of what a protection domain is. >>>> >>>> A protection domain is a unique view of memory and is represented by the >>>> value in the PKRU register. >> Out something about this in pkey(7), but explain what you mean by a >> "unique view of memory". > > Let's say there are only two protection keys: 0 and 1. There are two > disable bits per protection key (Access and Write Disable), so a two-key > PKRU looks like: > > | PKEY0 | PKEY1 | > | AD0 | WD0 | AD1 | WD1 | > > In this example, there are 16 possible protection domains, one for each > possible combination of the 4 rights-disable bits. > > "Changing a protection domain" would mean changing (setting or clearing) > the value of any of those 4 bits. Each unique value of PKRU represents > a view of memory, or unique protection domain. Again, some of this could make its way into pkey(7). And I guess there are useful nuggets for that page to be found in Jon's article at https://lwn.net/Articles/667156/ Thanks, Michael -- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Linux/UNIX System Programming Training: http://man7.org/training/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Hansen <dave@sr71.net> |
|---|---|
| Date | 2015-12-04 02:20 +0100 |
| Subject | [PATCH 24/34] mm, multi-arch: pass a protection key in to calc_vm_flag_bits() |
| Message-ID | <qBNcv-4nR-55@gated-at.bofh.it> |
| In reply to | #1283499 |
From: Dave Hansen <dave.hansen@linux.intel.com>
This plumbs a protection key through calc_vm_flag_bits(). We
could have done this in calc_vm_prot_bits(), but I did not feel
super strongly which way to go. It was pretty arbitrary which
one to use.
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: linux-api@vger.kernel.org
Cc: linux-arch@vger.kernel.org
---
b/arch/powerpc/include/asm/mman.h | 5 +++--
b/drivers/char/agp/frontend.c | 2 +-
b/drivers/staging/android/ashmem.c | 4 ++--
b/include/linux/mman.h | 6 +++---
b/mm/mmap.c | 2 +-
b/mm/mprotect.c | 2 +-
b/mm/nommu.c | 2 +-
7 files changed, 12 insertions(+), 11 deletions(-)
diff -puN arch/powerpc/include/asm/mman.h~pkeys-84-calc_vm_prot_bits arch/powerpc/include/asm/mman.h
--- a/arch/powerpc/include/asm/mman.h~pkeys-84-calc_vm_prot_bits 2015-12-03 16:21:29.142830772 -0800
+++ b/arch/powerpc/include/asm/mman.h 2015-12-03 16:21:29.155831361 -0800
@@ -18,11 +18,12 @@
* This file is included by linux/mman.h, so we can't use cacl_vm_prot_bits()
* here. How important is the optimization?
*/
-static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot)
+static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
+ unsigned long pkey)
{
return (prot & PROT_SAO) ? VM_SAO : 0;
}
-#define arch_calc_vm_prot_bits(prot) arch_calc_vm_prot_bits(prot)
+#define arch_calc_vm_prot_bits(prot, pkey) arch_calc_vm_prot_bits(prot, pkey)
static inline pgprot_t arch_vm_get_page_prot(unsigned long vm_flags)
{
diff -puN drivers/char/agp/frontend.c~pkeys-84-calc_vm_prot_bits drivers/char/agp/frontend.c
--- a/drivers/char/agp/frontend.c~pkeys-84-calc_vm_prot_bits 2015-12-03 16:21:29.143830817 -0800
+++ b/drivers/char/agp/frontend.c 2015-12-03 16:21:29.155831361 -0800
@@ -156,7 +156,7 @@ static pgprot_t agp_convert_mmap_flags(i
{
unsigned long prot_bits;
- prot_bits = calc_vm_prot_bits(prot) | VM_SHARED;
+ prot_bits = calc_vm_prot_bits(prot, 0) | VM_SHARED;
return vm_get_page_prot(prot_bits);
}
diff -puN drivers/staging/android/ashmem.c~pkeys-84-calc_vm_prot_bits drivers/staging/android/ashmem.c
--- a/drivers/staging/android/ashmem.c~pkeys-84-calc_vm_prot_bits 2015-12-03 16:21:29.145830908 -0800
+++ b/drivers/staging/android/ashmem.c 2015-12-03 16:21:29.156831407 -0800
@@ -372,8 +372,8 @@ static int ashmem_mmap(struct file *file
}
/* requested protection bits must match our allowed protection mask */
- if (unlikely((vma->vm_flags & ~calc_vm_prot_bits(asma->prot_mask)) &
- calc_vm_prot_bits(PROT_MASK))) {
+ if (unlikely((vma->vm_flags & ~calc_vm_prot_bits(asma->prot_mask, 0)) &
+ calc_vm_prot_bits(PROT_MASK, 0))) {
ret = -EPERM;
goto out;
}
diff -puN include/linux/mman.h~pkeys-84-calc_vm_prot_bits include/linux/mman.h
--- a/include/linux/mman.h~pkeys-84-calc_vm_prot_bits 2015-12-03 16:21:29.147830999 -0800
+++ b/include/linux/mman.h 2015-12-03 16:21:29.156831407 -0800
@@ -35,7 +35,7 @@ static inline void vm_unacct_memory(long
*/
#ifndef arch_calc_vm_prot_bits
-#define arch_calc_vm_prot_bits(prot) 0
+#define arch_calc_vm_prot_bits(prot, pkey) 0
#endif
#ifndef arch_vm_get_page_prot
@@ -70,12 +70,12 @@ static inline int arch_validate_prot(uns
* Combine the mmap "prot" argument into "vm_flags" used internally.
*/
static inline unsigned long
-calc_vm_prot_bits(unsigned long prot)
+calc_vm_prot_bits(unsigned long prot, unsigned long pkey)
{
return _calc_vm_trans(prot, PROT_READ, VM_READ ) |
_calc_vm_trans(prot, PROT_WRITE, VM_WRITE) |
_calc_vm_trans(prot, PROT_EXEC, VM_EXEC) |
- arch_calc_vm_prot_bits(prot);
+ arch_calc_vm_prot_bits(prot, pkey);
}
/*
diff -puN mm/mmap.c~pkeys-84-calc_vm_prot_bits mm/mmap.c
--- a/mm/mmap.c~pkeys-84-calc_vm_prot_bits 2015-12-03 16:21:29.148831044 -0800
+++ b/mm/mmap.c 2015-12-03 16:21:29.157831452 -0800
@@ -1309,7 +1309,7 @@ unsigned long do_mmap(struct file *file,
* to. we assume access permissions have been handled by the open
* of the memory object, so we don't do any here.
*/
- vm_flags |= calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
+ vm_flags |= calc_vm_prot_bits(prot, 0) | calc_vm_flag_bits(flags) |
mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
if (flags & MAP_LOCKED)
diff -puN mm/mprotect.c~pkeys-84-calc_vm_prot_bits mm/mprotect.c
--- a/mm/mprotect.c~pkeys-84-calc_vm_prot_bits 2015-12-03 16:21:29.150831135 -0800
+++ b/mm/mprotect.c 2015-12-03 16:21:29.158831497 -0800
@@ -373,7 +373,7 @@ SYSCALL_DEFINE3(mprotect, unsigned long,
if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
prot |= PROT_EXEC;
- vm_flags = calc_vm_prot_bits(prot);
+ vm_flags = calc_vm_prot_bits(prot, 0);
down_write(¤t->mm->mmap_sem);
diff -puN mm/nommu.c~pkeys-84-calc_vm_prot_bits mm/nommu.c
--- a/mm/nommu.c~pkeys-84-calc_vm_prot_bits 2015-12-03 16:21:29.152831225 -0800
+++ b/mm/nommu.c 2015-12-03 16:21:29.158831497 -0800
@@ -1090,7 +1090,7 @@ static unsigned long determine_vm_flags(
{
unsigned long vm_flags;
- vm_flags = calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags);
+ vm_flags = calc_vm_prot_bits(prot, 0) | calc_vm_flag_bits(flags);
/* vm_flags |= mm->def_flags; */
if (!(capabilities & NOMMU_MAP_DIRECT)) {
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Hansen <dave@sr71.net> |
|---|---|
| Date | 2015-12-04 02:20 +0100 |
| Subject | [PATCH 22/34] x86, pkeys: dump PTE pkey in /proc/pid/smaps |
| Message-ID | <qBNcv-4nR-53@gated-at.bofh.it> |
| In reply to | #1283499 |
From: Dave Hansen <dave.hansen@linux.intel.com>
The protection key can now be just as important as read/write
permissions on a VMA. We need some debug mechanism to help
figure out if it is in play. smaps seems like a logical
place to expose it.
arch/x86/kernel/setup.c is a bit of a weirdo place to put
this code, but it already had seq_file.h and there was not
a much better existing place to put it.
We also use no #ifdef. If protection keys is .config'd out
we will get the same function as if we used the weak generic
function.
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---
b/arch/x86/kernel/setup.c | 9 +++++++++
b/fs/proc/task_mmu.c | 5 +++++
2 files changed, 14 insertions(+)
diff -puN arch/x86/kernel/setup.c~pkeys-40-smaps arch/x86/kernel/setup.c
--- a/arch/x86/kernel/setup.c~pkeys-40-smaps 2015-12-03 16:21:28.284791859 -0800
+++ b/arch/x86/kernel/setup.c 2015-12-03 16:21:28.289792086 -0800
@@ -112,6 +112,7 @@
#include <asm/alternative.h>
#include <asm/prom.h>
#include <asm/microcode.h>
+#include <asm/mmu_context.h>
/*
* max_low_pfn_mapped: highest direct mapped pfn under 4GB
@@ -1282,3 +1283,11 @@ static int __init register_kernel_offset
return 0;
}
__initcall(register_kernel_offset_dumper);
+
+void arch_show_smap(struct seq_file *m, struct vm_area_struct *vma)
+{
+ if (!boot_cpu_has(X86_FEATURE_OSPKE))
+ return;
+
+ seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma));
+}
diff -puN fs/proc/task_mmu.c~pkeys-40-smaps fs/proc/task_mmu.c
--- a/fs/proc/task_mmu.c~pkeys-40-smaps 2015-12-03 16:21:28.285791904 -0800
+++ b/fs/proc/task_mmu.c 2015-12-03 16:21:28.290792131 -0800
@@ -657,6 +657,10 @@ static int smaps_hugetlb_range(pte_t *pt
}
#endif /* HUGETLB_PAGE */
+void __weak arch_show_smap(struct seq_file *m, struct vm_area_struct *vma)
+{
+}
+
static int show_smap(struct seq_file *m, void *v, int is_pid)
{
struct vm_area_struct *vma = v;
@@ -713,6 +717,7 @@ static int show_smap(struct seq_file *m,
(vma->vm_flags & VM_LOCKED) ?
(unsigned long)(mss.pss >> (10 + PSS_SHIFT)) : 0);
+ arch_show_smap(m, vma);
show_smap_vma_flags(m, vma);
m_cache_vma(m, vma);
return 0;
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Hansen <dave@sr71.net> |
|---|---|
| Date | 2015-12-04 02:30 +0100 |
| Subject | [PATCH 14/34] x86, pkeys: add functions to fetch PKRU |
| Message-ID | <qBNma-4rj-3@gated-at.bofh.it> |
| In reply to | #1283499 |
From: Dave Hansen <dave.hansen@linux.intel.com>
This adds the raw instruction to access PKRU as well as some
accessor functions that correctly handle when the CPU does not
support the instruction. We don't use it here, but we will use
read_pkru() in the next patch.
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---
b/arch/x86/include/asm/pgtable.h | 8 ++++++++
b/arch/x86/include/asm/special_insns.h | 22 ++++++++++++++++++++++
2 files changed, 30 insertions(+)
diff -puN arch/x86/include/asm/pgtable.h~pkeys-13-kernel-pkru-instructions arch/x86/include/asm/pgtable.h
--- a/arch/x86/include/asm/pgtable.h~pkeys-13-kernel-pkru-instructions 2015-12-03 16:21:24.298611081 -0800
+++ b/arch/x86/include/asm/pgtable.h 2015-12-03 16:21:24.303611308 -0800
@@ -102,6 +102,14 @@ static inline int pte_dirty(pte_t pte)
return pte_flags(pte) & _PAGE_DIRTY;
}
+
+static inline u32 read_pkru(void)
+{
+ if (boot_cpu_has(X86_FEATURE_OSPKE))
+ return __read_pkru();
+ return 0;
+}
+
static inline int pte_young(pte_t pte)
{
return pte_flags(pte) & _PAGE_ACCESSED;
diff -puN arch/x86/include/asm/special_insns.h~pkeys-13-kernel-pkru-instructions arch/x86/include/asm/special_insns.h
--- a/arch/x86/include/asm/special_insns.h~pkeys-13-kernel-pkru-instructions 2015-12-03 16:21:24.300611172 -0800
+++ b/arch/x86/include/asm/special_insns.h 2015-12-03 16:21:24.303611308 -0800
@@ -98,6 +98,28 @@ static inline void native_write_cr8(unsi
}
#endif
+#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
+static inline u32 __read_pkru(void)
+{
+ unsigned int ecx = 0;
+ unsigned int edx, pkru;
+
+ /*
+ * "rdpkru" instruction. Places PKRU contents in to EAX,
+ * clears EDX and requires that ecx=0.
+ */
+ asm volatile(".byte 0x0f,0x01,0xee\n\t"
+ : "=a" (pkru), "=d" (edx)
+ : "c" (ecx));
+ return pkru;
+}
+#else
+static inline u32 __read_pkru(void)
+{
+ return 0;
+}
+#endif
+
static inline void native_wbinvd(void)
{
asm volatile("wbinvd": : :"memory");
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2015-12-08 16:20 +0100 |
| Subject | Re: [PATCH 14/34] x86, pkeys: add functions to fetch PKRU |
| Message-ID | <qDsdA-41I-17@gated-at.bofh.it> |
| In reply to | #1283520 |
On Thu, 3 Dec 2015, Dave Hansen wrote:
> +#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
> +static inline u32 __read_pkru(void)
> +{
> + unsigned int ecx = 0;
> + unsigned int edx, pkru;
u32 please.
Other than that: Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> +
> + /*
> + * "rdpkru" instruction. Places PKRU contents in to EAX,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Hansen <dave@sr71.net> |
|---|---|
| Date | 2015-12-04 02:30 +0100 |
| Subject | [PATCH 11/34] x86, pkeys: pass VMA down in to fault signal generation code |
| Message-ID | <qBNma-4rj-5@gated-at.bofh.it> |
| In reply to | #1283499 |
From: Dave Hansen <dave.hansen@linux.intel.com>
During a page fault, we look up the VMA to ensure that the fault
is in a region with a valid mapping. But, in the top-level page
fault code we don't need the VMA for much else. Once we have
decided that an access is bad, we are going to send a signal no
matter what and do not need the VMA any more. So we do not pass
it down in to the signal generation code.
But, for protection keys, we need the VMA. It tells us *which*
protection key we violated if we get a PF_PK. So, we need to
pass the VMA down and fill in siginfo->si_pkey.
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---
b/arch/x86/mm/fault.c | 50 ++++++++++++++++++++++++++++----------------------
1 file changed, 28 insertions(+), 22 deletions(-)
diff -puN arch/x86/mm/fault.c~pkeys-08-pass-down-vma arch/x86/mm/fault.c
--- a/arch/x86/mm/fault.c~pkeys-08-pass-down-vma 2015-12-03 16:21:22.995551986 -0800
+++ b/arch/x86/mm/fault.c 2015-12-03 16:21:22.998552122 -0800
@@ -171,7 +171,8 @@ is_prefetch(struct pt_regs *regs, unsign
static void
force_sig_info_fault(int si_signo, int si_code, unsigned long address,
- struct task_struct *tsk, int fault)
+ struct task_struct *tsk, struct vm_area_struct *vma,
+ int fault)
{
unsigned lsb = 0;
siginfo_t info;
@@ -656,6 +657,8 @@ no_context(struct pt_regs *regs, unsigne
struct task_struct *tsk = current;
unsigned long flags;
int sig;
+ /* No context means no VMA to pass down */
+ struct vm_area_struct *vma = NULL;
/* Are we prepared to handle this kernel fault? */
if (fixup_exception(regs)) {
@@ -679,7 +682,8 @@ no_context(struct pt_regs *regs, unsigne
tsk->thread.cr2 = address;
/* XXX: hwpoison faults will set the wrong code. */
- force_sig_info_fault(signal, si_code, address, tsk, 0);
+ force_sig_info_fault(signal, si_code, address,
+ tsk, vma, 0);
}
/*
@@ -756,7 +760,8 @@ show_signal_msg(struct pt_regs *regs, un
static void
__bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
- unsigned long address, int si_code)
+ unsigned long address, struct vm_area_struct *vma,
+ int si_code)
{
struct task_struct *tsk = current;
@@ -799,7 +804,7 @@ __bad_area_nosemaphore(struct pt_regs *r
tsk->thread.error_code = error_code;
tsk->thread.trap_nr = X86_TRAP_PF;
- force_sig_info_fault(SIGSEGV, si_code, address, tsk, 0);
+ force_sig_info_fault(SIGSEGV, si_code, address, tsk, vma, 0);
return;
}
@@ -812,14 +817,14 @@ __bad_area_nosemaphore(struct pt_regs *r
static noinline void
bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
- unsigned long address)
+ unsigned long address, struct vm_area_struct *vma)
{
- __bad_area_nosemaphore(regs, error_code, address, SEGV_MAPERR);
+ __bad_area_nosemaphore(regs, error_code, address, vma, SEGV_MAPERR);
}
static void
__bad_area(struct pt_regs *regs, unsigned long error_code,
- unsigned long address, int si_code)
+ unsigned long address, struct vm_area_struct *vma, int si_code)
{
struct mm_struct *mm = current->mm;
@@ -829,25 +834,25 @@ __bad_area(struct pt_regs *regs, unsigne
*/
up_read(&mm->mmap_sem);
- __bad_area_nosemaphore(regs, error_code, address, si_code);
+ __bad_area_nosemaphore(regs, error_code, address, vma, si_code);
}
static noinline void
bad_area(struct pt_regs *regs, unsigned long error_code, unsigned long address)
{
- __bad_area(regs, error_code, address, SEGV_MAPERR);
+ __bad_area(regs, error_code, address, NULL, SEGV_MAPERR);
}
static noinline void
bad_area_access_error(struct pt_regs *regs, unsigned long error_code,
- unsigned long address)
+ unsigned long address, struct vm_area_struct *vma)
{
- __bad_area(regs, error_code, address, SEGV_ACCERR);
+ __bad_area(regs, error_code, address, vma, SEGV_ACCERR);
}
static void
do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
- unsigned int fault)
+ struct vm_area_struct *vma, unsigned int fault)
{
struct task_struct *tsk = current;
int code = BUS_ADRERR;
@@ -874,12 +879,13 @@ do_sigbus(struct pt_regs *regs, unsigned
code = BUS_MCEERR_AR;
}
#endif
- force_sig_info_fault(SIGBUS, code, address, tsk, fault);
+ force_sig_info_fault(SIGBUS, code, address, tsk, vma, fault);
}
static noinline void
mm_fault_error(struct pt_regs *regs, unsigned long error_code,
- unsigned long address, unsigned int fault)
+ unsigned long address, struct vm_area_struct *vma,
+ unsigned int fault)
{
if (fatal_signal_pending(current) && !(error_code & PF_USER)) {
no_context(regs, error_code, address, 0, 0);
@@ -903,9 +909,9 @@ mm_fault_error(struct pt_regs *regs, uns
} else {
if (fault & (VM_FAULT_SIGBUS|VM_FAULT_HWPOISON|
VM_FAULT_HWPOISON_LARGE))
- do_sigbus(regs, error_code, address, fault);
+ do_sigbus(regs, error_code, address, vma, fault);
else if (fault & VM_FAULT_SIGSEGV)
- bad_area_nosemaphore(regs, error_code, address);
+ bad_area_nosemaphore(regs, error_code, address, vma);
else
BUG();
}
@@ -1119,7 +1125,7 @@ __do_page_fault(struct pt_regs *regs, un
* Don't take the mm semaphore here. If we fixup a prefetch
* fault we could otherwise deadlock:
*/
- bad_area_nosemaphore(regs, error_code, address);
+ bad_area_nosemaphore(regs, error_code, address, NULL);
return;
}
@@ -1132,7 +1138,7 @@ __do_page_fault(struct pt_regs *regs, un
pgtable_bad(regs, error_code, address);
if (unlikely(smap_violation(error_code, regs))) {
- bad_area_nosemaphore(regs, error_code, address);
+ bad_area_nosemaphore(regs, error_code, address, NULL);
return;
}
@@ -1141,7 +1147,7 @@ __do_page_fault(struct pt_regs *regs, un
* in a region with pagefaults disabled then we must not take the fault
*/
if (unlikely(faulthandler_disabled() || !mm)) {
- bad_area_nosemaphore(regs, error_code, address);
+ bad_area_nosemaphore(regs, error_code, address, NULL);
return;
}
@@ -1185,7 +1191,7 @@ __do_page_fault(struct pt_regs *regs, un
if (unlikely(!down_read_trylock(&mm->mmap_sem))) {
if ((error_code & PF_USER) == 0 &&
!search_exception_tables(regs->ip)) {
- bad_area_nosemaphore(regs, error_code, address);
+ bad_area_nosemaphore(regs, error_code, address, NULL);
return;
}
retry:
@@ -1233,7 +1239,7 @@ retry:
*/
good_area:
if (unlikely(access_error(error_code, vma))) {
- bad_area_access_error(regs, error_code, address);
+ bad_area_access_error(regs, error_code, address, vma);
return;
}
@@ -1271,7 +1277,7 @@ good_area:
up_read(&mm->mmap_sem);
if (unlikely(fault & VM_FAULT_ERROR)) {
- mm_fault_error(regs, error_code, address, fault);
+ mm_fault_error(regs, error_code, address, vma, fault);
return;
}
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Hansen <dave@sr71.net> |
|---|---|
| Date | 2015-12-04 02:30 +0100 |
| Subject | [PATCH 02/34] x86, fpu: add placeholder for Processor Trace XSAVE state |
| Message-ID | <qBNma-4rj-9@gated-at.bofh.it> |
| In reply to | #1283499 |
From: Dave Hansen <dave.hansen@linux.intel.com>
x86 Maintainers,
I submitted this independently, but it must be applied before adding
subsequent patches. Please drop this if it has already been applied.
---
From: Dave Hansen <dave.hansen@linux.intel.com>
There is an XSAVE state component for Intel Processor Trace. But,
we do not use it and do not expect to ever use it.
We add a placeholder in the code for it so it is not a mystery and
also so we do not need an explicit enum initialization for Protection
Keys in a moment.
Why will we never use it? According to Andi Kleen:
The XSAVE support assumes that there is a single buffer
for each thread. But perf generally doesn't work this
way, it usually has only a single perf event per CPU per
user, and when tracing multiple threads on that CPU it
inherits perf event buffers between different threads. So
XSAVE per thread cannot handle this inheritance case
directly.
Using multiple XSAVE areas (another one per perf event)
would defeat some of the state caching that the CPUs do.
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---
b/arch/x86/include/asm/fpu/types.h | 1 +
b/arch/x86/kernel/fpu/xstate.c | 10 ++++++++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff -puN arch/x86/include/asm/fpu/types.h~pt-xstate-bit arch/x86/include/asm/fpu/types.h
--- a/arch/x86/include/asm/fpu/types.h~pt-xstate-bit 2015-12-03 16:21:19.003370936 -0800
+++ b/arch/x86/include/asm/fpu/types.h 2015-12-03 16:21:19.008371163 -0800
@@ -108,6 +108,7 @@ enum xfeature {
XFEATURE_OPMASK,
XFEATURE_ZMM_Hi256,
XFEATURE_Hi16_ZMM,
+ XFEATURE_PT_UNIMPLEMENTED_SO_FAR,
XFEATURE_MAX,
};
diff -puN arch/x86/kernel/fpu/xstate.c~pt-xstate-bit arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~pt-xstate-bit 2015-12-03 16:21:19.004370981 -0800
+++ b/arch/x86/kernel/fpu/xstate.c 2015-12-03 16:21:19.008371163 -0800
@@ -13,6 +13,11 @@
#include <asm/tlbflush.h>
+/*
+ * Although we spell it out in here, the Processor Trace
+ * xfeature is completely unused. We use other mechanisms
+ * to save/restore PT state in Linux.
+ */
static const char *xfeature_names[] =
{
"x87 floating point registers" ,
@@ -23,7 +28,7 @@ static const char *xfeature_names[] =
"AVX-512 opmask" ,
"AVX-512 Hi256" ,
"AVX-512 ZMM_Hi256" ,
- "unknown xstate feature" ,
+ "Processor Trace (unused)" ,
};
/*
@@ -469,7 +474,8 @@ static void check_xstate_against_struct(
* numbers.
*/
if ((nr < XFEATURE_YMM) ||
- (nr >= XFEATURE_MAX)) {
+ (nr >= XFEATURE_MAX) ||
+ (nr == XFEATURE_PT_UNIMPLEMENTED_SO_FAR)) {
WARN_ONCE(1, "no structure for xstate: %d\n", nr);
XSTATE_WARN_ON(1);
}
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Hansen <dave@sr71.net> |
|---|---|
| Date | 2015-12-04 02:30 +0100 |
| Subject | [PATCH 16/34] x86, mm: simplify get_user_pages() PTE bit handling |
| Message-ID | <qBNma-4rj-17@gated-at.bofh.it> |
| In reply to | #1283499 |
From: Dave Hansen <dave.hansen@linux.intel.com>
The current get_user_pages() code is a wee bit more complicated
than it needs to be for pte bit checking. Currently, it establishes
a mask of required pte _PAGE_* bits and ensures that the pte it
goes after has all those bits.
This consolidates the three identical copies of this code.
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---
b/arch/x86/mm/gup.c | 45 ++++++++++++++++++++++++++++-----------------
1 file changed, 28 insertions(+), 17 deletions(-)
diff -puN arch/x86/mm/gup.c~pkeys-16-gup-swizzle arch/x86/mm/gup.c
--- a/arch/x86/mm/gup.c~pkeys-16-gup-swizzle 2015-12-03 16:21:25.148649631 -0800
+++ b/arch/x86/mm/gup.c 2015-12-03 16:21:25.151649767 -0800
@@ -63,6 +63,30 @@ retry:
#endif
}
+static inline int pte_allows_gup(pte_t pte, int write)
+{
+ /*
+ * 'pte' can reall be a pte, pmd or pud. We only check
+ * _PAGE_PRESENT, _PAGE_USER, and _PAGE_RW in here which
+ * are the same value on all 3 types.
+ */
+ if (!(pte_flags(pte) & (_PAGE_PRESENT|_PAGE_USER)))
+ return 0;
+ if (write && !(pte_write(pte)))
+ return 0;
+ return 1;
+}
+
+static inline int pmd_allows_gup(pmd_t pmd, int write)
+{
+ return pte_allows_gup(*(pte_t *)&pmd, write);
+}
+
+static inline int pud_allows_gup(pud_t pud, int write)
+{
+ return pte_allows_gup(*(pte_t *)&pud, write);
+}
+
/*
* The performance critical leaf functions are made noinline otherwise gcc
* inlines everything into a single function which results in too much
@@ -71,13 +95,8 @@ retry:
static noinline int gup_pte_range(pmd_t pmd, unsigned long addr,
unsigned long end, int write, struct page **pages, int *nr)
{
- unsigned long mask;
pte_t *ptep;
- mask = _PAGE_PRESENT|_PAGE_USER;
- if (write)
- mask |= _PAGE_RW;
-
ptep = pte_offset_map(&pmd, addr);
do {
pte_t pte = gup_get_pte(ptep);
@@ -88,8 +107,8 @@ static noinline int gup_pte_range(pmd_t
pte_unmap(ptep);
return 0;
}
-
- if ((pte_flags(pte) & (mask | _PAGE_SPECIAL)) != mask) {
+ if (!pte_allows_gup(pte, write) ||
+ pte_special(pte)) {
pte_unmap(ptep);
return 0;
}
@@ -117,14 +136,10 @@ static inline void get_head_page_multipl
static noinline int gup_huge_pmd(pmd_t pmd, unsigned long addr,
unsigned long end, int write, struct page **pages, int *nr)
{
- unsigned long mask;
struct page *head, *page;
int refs;
- mask = _PAGE_PRESENT|_PAGE_USER;
- if (write)
- mask |= _PAGE_RW;
- if ((pmd_flags(pmd) & mask) != mask)
+ if (!pmd_allows_gup(pmd, write))
return 0;
/* hugepages are never "special" */
VM_BUG_ON(pmd_flags(pmd) & _PAGE_SPECIAL);
@@ -193,14 +208,10 @@ static int gup_pmd_range(pud_t pud, unsi
static noinline int gup_huge_pud(pud_t pud, unsigned long addr,
unsigned long end, int write, struct page **pages, int *nr)
{
- unsigned long mask;
struct page *head, *page;
int refs;
- mask = _PAGE_PRESENT|_PAGE_USER;
- if (write)
- mask |= _PAGE_RW;
- if ((pud_flags(pud) & mask) != mask)
+ if (!pud_allows_gup(pud, write))
return 0;
/* hugepages are never "special" */
VM_BUG_ON(pud_flags(pud) & _PAGE_SPECIAL);
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2015-12-08 19:10 +0100 |
| Subject | Re: [PATCH 16/34] x86, mm: simplify get_user_pages() PTE bit handling |
| Message-ID | <qDuS5-5IQ-9@gated-at.bofh.it> |
| In reply to | #1283523 |
On Thu, 3 Dec 2015, Dave Hansen wrote:
>
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> The current get_user_pages() code is a wee bit more complicated
> than it needs to be for pte bit checking. Currently, it establishes
> a mask of required pte _PAGE_* bits and ensures that the pte it
> goes after has all those bits.
>
> This consolidates the three identical copies of this code.
>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> ---
>
> b/arch/x86/mm/gup.c | 45 ++++++++++++++++++++++++++++-----------------
> 1 file changed, 28 insertions(+), 17 deletions(-)
>
> diff -puN arch/x86/mm/gup.c~pkeys-16-gup-swizzle arch/x86/mm/gup.c
> --- a/arch/x86/mm/gup.c~pkeys-16-gup-swizzle 2015-12-03 16:21:25.148649631 -0800
> +++ b/arch/x86/mm/gup.c 2015-12-03 16:21:25.151649767 -0800
> @@ -63,6 +63,30 @@ retry:
> #endif
> }
>
> +static inline int pte_allows_gup(pte_t pte, int write)
> +{
> + /*
> + * 'pte' can reall be a pte, pmd or pud. We only check
> + * _PAGE_PRESENT, _PAGE_USER, and _PAGE_RW in here which
> + * are the same value on all 3 types.
> + */
> + if (!(pte_flags(pte) & (_PAGE_PRESENT|_PAGE_USER)))
> + return 0;
> + if (write && !(pte_write(pte)))
> + return 0;
> + return 1;
> +}
> +
> +static inline int pmd_allows_gup(pmd_t pmd, int write)
> +{
> + return pte_allows_gup(*(pte_t *)&pmd, write);
> +}
> +
> +static inline int pud_allows_gup(pud_t pud, int write)
> +{
> + return pte_allows_gup(*(pte_t *)&pud, write);
> +}
This still puzzles me. And the only reason it compiles is because we
have -fno-strict-aliasing set ...
All this operates on the pteval or even just on the pte_flags(). Even
the new arch_pte_access_permitted() thingy which you add later is only
interrested in pte_flags() and its just a wrapper around
__pkru_allows_pkey().
So for readability and simplicity sake, can we please do something
like this (pkey check already added):
/*
* 'pteval' can reall be a pte, pmd or pud. We only check
* _PAGE_PRESENT, _PAGE_USER, and _PAGE_RW in here which
* are the same value on all 3 types.
*/
static inline int pte_allows_gup(unsigned long pteval, int write)
{
unsigned long mask = _PAGE_PRESENT|_PAGE_USER;
if (write)
mask |= _PAGE_RW;
if ((pteval & mask) != mask)
return 0;
if (!__pkru_allows_pkey(pte_flags_pkey(pteval), write))
return 0;
return 1;
}
and at the callsites do:
if (pte_allows_gup(pte_val(pte, write))
if (pte_allows_gup(pmd_val(pmd, write))
if (pte_allows_gup(pud_val(pud, write))
Hmm?
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Hansen <dave@sr71.net> |
|---|---|
| Date | 2015-12-08 19:40 +0100 |
| Subject | Re: [PATCH 16/34] x86, mm: simplify get_user_pages() PTE bit handling |
| Message-ID | <qDvl9-5U7-51@gated-at.bofh.it> |
| In reply to | #1286729 |
On 12/08/2015 10:01 AM, Thomas Gleixner wrote:
> static inline int pte_allows_gup(unsigned long pteval, int write)
> {
> unsigned long mask = _PAGE_PRESENT|_PAGE_USER;
>
> if (write)
> mask |= _PAGE_RW;
>
> if ((pteval & mask) != mask)
> return 0;
>
> if (!__pkru_allows_pkey(pte_flags_pkey(pteval), write))
> return 0;
> return 1;
> }
>
> and at the callsites do:
>
> if (pte_allows_gup(pte_val(pte, write))
>
> if (pte_allows_gup(pmd_val(pmd, write))
>
> if (pte_allows_gup(pud_val(pud, write))
>
> Hmm?
Looks fine to me. I'll do that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Hansen <dave@sr71.net> |
|---|---|
| Date | 2015-12-04 02:30 +0100 |
| Subject | [PATCH 10/34] x86, pkeys: arch-specific protection bits |
| Message-ID | <qBNma-4rj-19@gated-at.bofh.it> |
| In reply to | #1283499 |
From: Dave Hansen <dave.hansen@linux.intel.com>
Lots of things seem to do:
vma->vm_page_prot = vm_get_page_prot(flags);
and the ptes get created right from things we pull out
of ->vm_page_prot. So it is very convenient if we can
store the protection key in flags and vm_page_prot, just
like the existing permission bits (_PAGE_RW/PRESENT). It
greatly reduces the amount of plumbing and arch-specific
hacking we have to do in generic code.
This also takes the new PROT_PKEY{0,1,2,3} flags and
turns *those* in to VM_ flags for vma->vm_flags.
The protection key values are stored in 4 places:
1. "prot" argument to system calls
2. vma->vm_flags, filled from the mmap "prot"
3. vma->vm_page prot, filled from vma->vm_flags
4. the PTE itself.
The pseudocode for these for steps are as follows:
mmap(PROT_PKEY*)
vma->vm_flags = ... | arch_calc_vm_prot_bits(mmap_prot);
vma->vm_page_prot = ... | arch_vm_get_page_prot(vma->vm_flags);
pte = pfn | vma->vm_page_prot
Note that this provides a new definitions for x86:
arch_vm_get_page_prot()
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---
b/arch/x86/include/asm/mmu_context.h | 20 ++++++++++++++++++++
b/arch/x86/include/asm/pgtable_types.h | 12 ++++++++++--
b/arch/x86/include/uapi/asm/mman.h | 16 ++++++++++++++++
b/include/linux/mm.h | 6 ++++++
4 files changed, 52 insertions(+), 2 deletions(-)
diff -puN arch/x86/include/asm/mmu_context.h~pkeys-08-store-pkey-in-vma arch/x86/include/asm/mmu_context.h
--- a/arch/x86/include/asm/mmu_context.h~pkeys-08-store-pkey-in-vma 2015-12-03 16:21:22.505529763 -0800
+++ b/arch/x86/include/asm/mmu_context.h 2015-12-03 16:21:22.514530171 -0800
@@ -243,4 +243,24 @@ static inline void arch_unmap(struct mm_
mpx_notify_unmap(mm, vma, start, end);
}
+static inline int vma_pkey(struct vm_area_struct *vma)
+{
+ u16 pkey = 0;
+#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
+ unsigned long vma_pkey_mask = VM_PKEY_BIT0 | VM_PKEY_BIT1 |
+ VM_PKEY_BIT2 | VM_PKEY_BIT3;
+ /*
+ * ffs is one-based, not zero-based, so bias back down by 1.
+ */
+ int vm_pkey_shift = __builtin_ffsl(vma_pkey_mask) - 1;
+ /*
+ * gcc generates better code if we do this rather than:
+ * pkey = (flags & mask) >> shift
+ */
+ pkey = (vma->vm_flags >> vm_pkey_shift) &
+ (vma_pkey_mask >> vm_pkey_shift);
+#endif
+ return pkey;
+}
+
#endif /* _ASM_X86_MMU_CONTEXT_H */
diff -puN arch/x86/include/asm/pgtable_types.h~pkeys-08-store-pkey-in-vma arch/x86/include/asm/pgtable_types.h
--- a/arch/x86/include/asm/pgtable_types.h~pkeys-08-store-pkey-in-vma 2015-12-03 16:21:22.507529853 -0800
+++ b/arch/x86/include/asm/pgtable_types.h 2015-12-03 16:21:22.514530171 -0800
@@ -111,7 +111,12 @@
#define _KERNPG_TABLE (_PAGE_PRESENT | _PAGE_RW | _PAGE_ACCESSED | \
_PAGE_DIRTY)
-/* Set of bits not changed in pte_modify */
+/*
+ * Set of bits not changed in pte_modify. The pte's
+ * protection key is treated like _PAGE_RW, for
+ * instance, and is *not* included in this mask since
+ * pte_modify() does modify it.
+ */
#define _PAGE_CHG_MASK (PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT | \
_PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY | \
_PAGE_SOFT_DIRTY)
@@ -227,7 +232,10 @@ enum page_cache_mode {
/* Extracts the PFN from a (pte|pmd|pud|pgd)val_t of a 4KB page */
#define PTE_PFN_MASK ((pteval_t)PHYSICAL_PAGE_MASK)
-/* Extracts the flags from a (pte|pmd|pud|pgd)val_t of a 4KB page */
+/*
+ * Extracts the flags from a (pte|pmd|pud|pgd)val_t
+ * This includes the protection key value.
+ */
#define PTE_FLAGS_MASK (~PTE_PFN_MASK)
typedef struct pgprot { pgprotval_t pgprot; } pgprot_t;
diff -puN arch/x86/include/uapi/asm/mman.h~pkeys-08-store-pkey-in-vma arch/x86/include/uapi/asm/mman.h
--- a/arch/x86/include/uapi/asm/mman.h~pkeys-08-store-pkey-in-vma 2015-12-03 16:21:22.509529944 -0800
+++ b/arch/x86/include/uapi/asm/mman.h 2015-12-03 16:21:22.514530171 -0800
@@ -6,6 +6,22 @@
#define MAP_HUGE_2MB (21 << MAP_HUGE_SHIFT)
#define MAP_HUGE_1GB (30 << MAP_HUGE_SHIFT)
+#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
+/*
+ * Take the 4 protection key bits out of the vma->vm_flags
+ * value and turn them in to the bits that we can put in
+ * to a pte.
+ *
+ * Only override these if Protection Keys are available
+ * (which is only on 64-bit).
+ */
+#define arch_vm_get_page_prot(vm_flags) __pgprot( \
+ ((vm_flags) & VM_PKEY_BIT0 ? _PAGE_PKEY_BIT0 : 0) | \
+ ((vm_flags) & VM_PKEY_BIT1 ? _PAGE_PKEY_BIT1 : 0) | \
+ ((vm_flags) & VM_PKEY_BIT2 ? _PAGE_PKEY_BIT2 : 0) | \
+ ((vm_flags) & VM_PKEY_BIT3 ? _PAGE_PKEY_BIT3 : 0))
+#endif
+
#include <asm-generic/mman.h>
#endif /* _ASM_X86_MMAN_H */
diff -puN include/linux/mm.h~pkeys-08-store-pkey-in-vma include/linux/mm.h
--- a/include/linux/mm.h~pkeys-08-store-pkey-in-vma 2015-12-03 16:21:22.510529990 -0800
+++ b/include/linux/mm.h 2015-12-03 16:21:22.515530216 -0800
@@ -167,6 +167,12 @@ extern unsigned int kobjsize(const void
#if defined(CONFIG_X86)
# define VM_PAT VM_ARCH_1 /* PAT reserves whole VMA at once (x86) */
+#if defined (CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS)
+# define VM_PKEY_BIT0 VM_HIGH_ARCH_0 /* A protection key is a 4-bit value */
+# define VM_PKEY_BIT1 VM_HIGH_ARCH_1
+# define VM_PKEY_BIT2 VM_HIGH_ARCH_2
+# define VM_PKEY_BIT3 VM_HIGH_ARCH_3
+#endif
#elif defined(CONFIG_PPC)
# define VM_SAO VM_ARCH_1 /* Strong Access Ordering (powerpc) */
#elif defined(CONFIG_PARISC)
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2015-12-08 16:20 +0100 |
| Subject | Re: [PATCH 10/34] x86, pkeys: arch-specific protection bitsy |
| Message-ID | <qDsdz-41I-1@gated-at.bofh.it> |
| In reply to | #1283524 |
Dave,
On Thu, 3 Dec 2015, Dave Hansen wrote:
>
> +static inline int vma_pkey(struct vm_area_struct *vma)
Shouldn't this return something unsigned?
> +{
> + u16 pkey = 0;
> +#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
> + unsigned long vma_pkey_mask = VM_PKEY_BIT0 | VM_PKEY_BIT1 |
> + VM_PKEY_BIT2 | VM_PKEY_BIT3;
> + /*
> + * ffs is one-based, not zero-based, so bias back down by 1.
> + */
> + int vm_pkey_shift = __builtin_ffsl(vma_pkey_mask) - 1;
Took me some time to figure out that this will resolve to a compile
time constant (hopefully). Is there a reason why we don't have a
VM_PKEY_SHIFT constant in the header file which makes that code just
simple and intuitive?
> + /*
> + * gcc generates better code if we do this rather than:
> + * pkey = (flags & mask) >> shift
> + */
> + pkey = (vma->vm_flags >> vm_pkey_shift) &
> + (vma_pkey_mask >> vm_pkey_shift);
My gcc (4.9) does it the other way round for whatever reason.
I really prefer to have this as simple as:
#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
#define VM_PKEY_MASK (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | VM_PKEY_BIT3)
#define VM_PKEY_SHIFT
#else
#define VM_PKEY_MASK 0UL
#define VM_PKEY_SHIFT 0
#endif
static inline unsigned int vma_pkey(struct vm_area_struct *vma)
{
return (vma->vm_flags & VM_PKEY_MASK) >> VM_PKEY_SHIFT;
}
or
#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
#define VM_PKEY_MASK (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | VM_PKEY_BIT3)
#define VM_PKEY_SHIFT
static inline unsigned int vma_pkey(struct vm_area_struct *vma)
{
return (vma->vm_flags & VM_PKEY_MASK) >> VM_PKEY_SHIFT;
}
#else
static inline unsigned int vma_pkey(struct vm_area_struct *vma)
{
return 0;
}
#endif
Hmm?
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Hansen <dave@sr71.net> |
|---|---|
| Date | 2015-12-08 17:40 +0100 |
| Subject | Re: [PATCH 10/34] x86, pkeys: arch-specific protection bitsy |
| Message-ID | <qDtsZ-4J4-11@gated-at.bofh.it> |
| In reply to | #1286570 |
On 12/08/2015 07:15 AM, Thomas Gleixner wrote:
> On Thu, 3 Dec 2015, Dave Hansen wrote:
>>
>> +static inline int vma_pkey(struct vm_area_struct *vma)
>
> Shouldn't this return something unsigned?
Ingo had asked that we use 'int' in the syscalls at some point. We also
use a -1 to mean "no pkey set" (to differentiate it from pkey=0) at
least at the very top of the syscall level.
>> +{
>> + u16 pkey = 0;
>> +#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
>> + unsigned long vma_pkey_mask = VM_PKEY_BIT0 | VM_PKEY_BIT1 |
>> + VM_PKEY_BIT2 | VM_PKEY_BIT3;
>> + /*
>> + * ffs is one-based, not zero-based, so bias back down by 1.
>> + */
>> + int vm_pkey_shift = __builtin_ffsl(vma_pkey_mask) - 1;
>
> Took me some time to figure out that this will resolve to a compile
> time constant (hopefully). Is there a reason why we don't have a
> VM_PKEY_SHIFT constant in the header file which makes that code just
> simple and intuitive?
All of the VM_* flags are #defined as bitmaps directly and don't define
shifts:
#define VM_MAYWRITE 0x00000020
#define VM_MAYEXEC 0x00000040
#define VM_MAYSHARE 0x00000080
...
So to get a shift we've either got to do a ffs somewhere, or we have to
define the VM_PKEY_BIT*'s differently from all of the other VM_* flags.
Or, we do something along the lines of:
#define VM_PKEY_BIT0 0x100000000UL
#define __VM_PKEY_SHIFT (32)
and we run a small risk that somebody will desynchronize the shift and
the bit definition.
We only need this shift in this *one* place, so that's why I opted for
the local variable and ffs.
>> + /*
>> + * gcc generates better code if we do this rather than:
>> + * pkey = (flags & mask) >> shift
>> + */
>> + pkey = (vma->vm_flags >> vm_pkey_shift) &
>> + (vma_pkey_mask >> vm_pkey_shift);
>
> My gcc (4.9) does it the other way round for whatever reason.
I'll go recheck.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2015-12-08 18:30 +0100 |
| Subject | Re: [PATCH 10/34] x86, pkeys: arch-specific protection bitsy |
| Message-ID | <qDufo-5gg-19@gated-at.bofh.it> |
| In reply to | #1286669 |
Dave,
On Tue, 8 Dec 2015, Dave Hansen wrote:
> On 12/08/2015 07:15 AM, Thomas Gleixner wrote:
> > On Thu, 3 Dec 2015, Dave Hansen wrote:
> >>
> >> +static inline int vma_pkey(struct vm_area_struct *vma)
> >
> > Shouldn't this return something unsigned?
>
> Ingo had asked that we use 'int' in the syscalls at some point. We also
> use a -1 to mean "no pkey set" (to differentiate it from pkey=0) at
> least at the very top of the syscall level.
Ok.
> >> +{
> >> + u16 pkey = 0;
> >> +#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
> >> + unsigned long vma_pkey_mask = VM_PKEY_BIT0 | VM_PKEY_BIT1 |
> >> + VM_PKEY_BIT2 | VM_PKEY_BIT3;
> >> + /*
> >> + * ffs is one-based, not zero-based, so bias back down by 1.
> >> + */
> >> + int vm_pkey_shift = __builtin_ffsl(vma_pkey_mask) - 1;
> >
> > Took me some time to figure out that this will resolve to a compile
> > time constant (hopefully). Is there a reason why we don't have a
> > VM_PKEY_SHIFT constant in the header file which makes that code just
> > simple and intuitive?
>
> All of the VM_* flags are #defined as bitmaps directly and don't define
> shifts:
>
> #define VM_MAYWRITE 0x00000020
> #define VM_MAYEXEC 0x00000040
> #define VM_MAYSHARE 0x00000080
> ...
>
> So to get a shift we've either got to do a ffs somewhere, or we have to
> define the VM_PKEY_BIT*'s differently from all of the other VM_* flags.
> Or, we do something along the lines of:
>
> #define VM_PKEY_BIT0 0x100000000UL
> #define __VM_PKEY_SHIFT (32)
Well, yes. But these are the new "high" bits so we really can do it:
#define VM_KEY_BIT_SHIFT 32
#define VM_KEY_BIT0 BIT(VM_KEY_BIT_SHIFT);
...
> and we run a small risk that somebody will desynchronize the shift and
> the bit definition.
>
> We only need this shift in this *one* place, so that's why I opted for
> the local variable and ffs.
>
> >> + /*
> >> + * gcc generates better code if we do this rather than:
> >> + * pkey = (flags & mask) >> shift
> >> + */
> >> + pkey = (vma->vm_flags >> vm_pkey_shift) &
> >> + (vma_pkey_mask >> vm_pkey_shift);
> >
> > My gcc (4.9) does it the other way round for whatever reason.
>
> I'll go recheck.
It's one instruction difference and that even depends on the offset of
vm_flags in the struct. So we really can go for the readable version :)
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
Back to top | Article view | linux.kernel
csiph-web