Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1231972 > unrolled thread

Re: [PATCH 10/26] x86, pkeys: notify userspace about protection key faults

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 10/26] x86, pkeys: notify userspace about protection key  faults Ingo Molnar <mingo@kernel.org> - 2015-09-24 11:30 +0200
    Re: [PATCH 10/26] x86, pkeys: notify userspace about protection key  faults Ingo Molnar <mingo@kernel.org> - 2015-09-24 11:40 +0200
      Re: [PATCH 10/26] x86, pkeys: notify userspace about protection key  faults Dave Hansen <dave@sr71.net> - 2015-09-24 19:50 +0200
        Re: [PATCH 10/26] x86, pkeys: notify userspace about protection key  faults Ingo Molnar <mingo@kernel.org> - 2015-09-25 09:20 +0200
          Re: [PATCH 10/26] x86, pkeys: notify userspace about protection key  faults Dave Hansen <dave@sr71.net> - 2015-09-26 01:20 +0200
            Re: [PATCH 10/26] x86, pkeys: notify userspace about protection key  faults Ingo Molnar <mingo@kernel.org> - 2015-09-26 08:30 +0200
              Re: [PATCH 10/26] x86, pkeys: notify userspace about protection key  faults Dave Hansen <dave@sr71.net> - 2015-09-28 00:50 +0200
                Re: [PATCH 10/26] x86, pkeys: notify userspace about protection key  faults Ingo Molnar <mingo@kernel.org> - 2015-09-28 08:00 +0200
    Re: [PATCH 10/26] x86, pkeys: notify userspace about protection key  faults Dave Hansen <dave@sr71.net> - 2015-09-24 19:20 +0200
      Re: [PATCH 10/26] x86, pkeys: notify userspace about protection key  faults Christian Borntraeger <borntraeger@de.ibm.com> - 2015-09-28 21:30 +0200
        Re: [PATCH 10/26] x86, pkeys: notify userspace about protection key  faults Dave Hansen <dave@sr71.net> - 2015-09-28 21:40 +0200

#1231972 — Re: [PATCH 10/26] x86, pkeys: notify userspace about protection key faults

* Dave Hansen <dave@sr71.net> wrote:

> A protection key fault is very similar to any other access
> error.  There must be a VMA, etc...  We even want to take
> the same action (SIGSEGV) that we do with a normal access
> fault.
> 
> However, we do need to let userspace know that something
> is different.  We do this the same way what we did with
> SEGV_BNDERR with Memory Protection eXtensions (MPX):
> define a new SEGV code: SEGV_PKUERR.
> 
> We also add a siginfo field: si_pkey that reveals to
> userspace which protection key was set on the PTE that
> we faulted on.  There is no other easy way for
> userspace to figure this out.  They could parse smaps
> but that would be a bit cruel.

> diff -puN arch/x86/mm/fault.c~pkeys-09-siginfo arch/x86/mm/fault.c
> --- a/arch/x86/mm/fault.c~pkeys-09-siginfo	2015-09-16 10:48:15.580161678 -0700
> +++ b/arch/x86/mm/fault.c	2015-09-16 10:48:15.591162177 -0700
> @@ -15,12 +15,14 @@
>  #include <linux/context_tracking.h>	/* exception_enter(), ...	*/
>  #include <linux/uaccess.h>		/* faulthandler_disabled()	*/
>  
> +#include <asm/cpufeature.h>		/* boot_cpu_has, ...		*/
>  #include <asm/traps.h>			/* dotraplinkage, ...		*/
>  #include <asm/pgalloc.h>		/* pgd_*(), ...			*/
>  #include <asm/kmemcheck.h>		/* kmemcheck_*(), ...		*/
>  #include <asm/fixmap.h>			/* VSYSCALL_ADDR		*/
>  #include <asm/vsyscall.h>		/* emulate_vsyscall		*/
>  #include <asm/vm86.h>			/* struct vm86			*/
> +#include <asm/mmu_context.h>		/* vma_pkey()			*/
>  
>  #define CREATE_TRACE_POINTS
>  #include <asm/trace/exceptions.h>
> @@ -169,6 +171,45 @@ is_prefetch(struct pt_regs *regs, unsign
>  	return prefetch;
>  }
>  
> +static u16 fetch_pkey(unsigned long address, struct task_struct *tsk)
> +{
> +	u16 ret;
> +	spinlock_t *ptl;
> +	pte_t *ptep;
> +	pte_t pte;
> +	int follow_ret;
> +
> +	if (!boot_cpu_has(X86_FEATURE_OSPKE))
> +		return 0;
> +
> +	follow_ret = follow_pte(tsk->mm, address, &ptep, &ptl);
> +	if (!follow_ret) {
> +		/*
> +		 * On a successful follow, make sure to
> +		 * drop the lock.
> +		 */
> +		pte = *ptep;
> +		pte_unmap_unlock(ptep, ptl);
> +		ret = pte_pkey(pte);
> +	} else {
> +		/*
> +		 * There is no PTE.  Go looking for the pkey in
> +		 * the VMA.  If we did not find a pkey violation
> +		 * from either the PTE or the VMA, then it must
> +		 * have been a fault from the hardware.  Perhaps
> +		 * the PTE got zapped before we got in here.
> +		 */
> +		struct vm_area_struct *vma = find_vma(tsk->mm, address);
> +		if (vma) {
> +			ret = vma_pkey(vma);
> +		} else {
> +			WARN_ONCE(1, "no PTE or VMA @ %lx\n", address);
> +			ret = 0;
> +		}
> +	}
> +	return ret;

Yeah, so I have three observations:

1)

I don't think this warning is entirely right, because this is a fundamentally racy 
op.

fetch_pkey(), called by force_sign_info_fault(), can be called while not holding 
the vma - and if we race with any other thread of the mm, the vma might be gone 
already.

So any threaded app using pkeys and vmas in parallel could trigger that WARN_ON().

2)

And note that this is a somewhat new scenario: in regular page faults, 
'error_code' always carries a then-valid cause of the page fault with itself. So 
we can put that into the siginfo and can be sure that it's the reason for the 
fault.

With the above pkey code, we fetch the pte separately from the fault, and without 
synchronizing with the fault - and we cannot do that, nor do we want to.

So I think this code should just accept the fact that races may happen. Perhaps 
warn if we get here with only a single mm user. (but even that would be a bit racy 
as we don't serialize against exit())

3)

For user-space that somehow wants to handle pkeys dynamically and drive them via 
faults, this seems somewhat inefficient: we already do a find_vma() in the primary 
fault lookup - and with the typical pkey usecase it will find a vma, just with the 
wrong access permissions. But when we generate the siginfo here, why do we do a 
find_vma() again? Why not pass the vma to the siginfo generating function?

> --- a/include/uapi/asm-generic/siginfo.h~pkeys-09-siginfo	2015-09-16 10:48:15.584161859 -0700
> +++ b/include/uapi/asm-generic/siginfo.h	2015-09-16 10:48:15.592162222 -0700
> @@ -95,6 +95,13 @@ typedef struct siginfo {
>  				void __user *_lower;
>  				void __user *_upper;
>  			} _addr_bnd;
> +			int _pkey; /* FIXME: protection key value??
> +				    * Do we really need this in here?
> +				    * userspace can get the PKRU value in
> +				    * the signal handler, but they do not
> +				    * easily have access to the PKEY value
> +				    * from the PTE.
> +				    */
>  		} _sigfault;

A couple of comments:

1)

Please use our ABI types - this one should be 'u32' I think.

We could use 'u8' as well here, and mark another 3 bytes next to it as reserved 
for future flags. Right now protection keys use 4 bits, but do you really think 
they'll ever grow beyond 8 bits? PTE bits are a scarce resource in general.

2)

To answer your question in the comment: it looks useful to have some sort of 
'extended page fault error code' information here, which shows why the page fault 
happened. With the regular error_code it's easy - with protection keys there's 16 
separate keys possible and user-space might not know the actual key value in the 
pte.

3)

Please add suitable self-tests to tools/tests/selftests/x86/ that both documents 
the preferred usage of pkeys, demonstrates all implemented aspects the new ABI and 
provokes a fault and prints the resulting siginfo, etc.

> @@ -206,7 +214,8 @@ typedef struct siginfo {
>  #define SEGV_MAPERR	(__SI_FAULT|1)	/* address not mapped to object */
>  #define SEGV_ACCERR	(__SI_FAULT|2)	/* invalid permissions for mapped object */
>  #define SEGV_BNDERR	(__SI_FAULT|3)  /* failed address bound checks */
> -#define NSIGSEGV	3
> +#define SEGV_PKUERR	(__SI_FAULT|4)  /* failed address bound checks */
> +#define NSIGSEGV	4

You copy & pasted the MPX comment here, it should read something like:

   #define SEGV_PKUERR	(__SI_FAULT|4)  /* failed protection keys checks */

Thanks,

	Ingo
--
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] | [next] | [standalone]

#1231974

* Ingo Molnar <mingo@kernel.org> wrote:

> > --- a/include/uapi/asm-generic/siginfo.h~pkeys-09-siginfo	2015-09-16 10:48:15.584161859 -0700
> > +++ b/include/uapi/asm-generic/siginfo.h	2015-09-16 10:48:15.592162222 -0700
> > @@ -95,6 +95,13 @@ typedef struct siginfo {
> >  				void __user *_lower;
> >  				void __user *_upper;
> >  			} _addr_bnd;
> > +			int _pkey; /* FIXME: protection key value??
> > +				    * Do we really need this in here?
> > +				    * userspace can get the PKRU value in
> > +				    * the signal handler, but they do not
> > +				    * easily have access to the PKEY value
> > +				    * from the PTE.
> > +				    */
> >  		} _sigfault;
> 
> A couple of comments:
> 
> 1)
> 
> Please use our ABI types - this one should be 'u32' I think.
> 
> We could use 'u8' as well here, and mark another 3 bytes next to it as reserved 
> for future flags. Right now protection keys use 4 bits, but do you really think 
> they'll ever grow beyond 8 bits? PTE bits are a scarce resource in general.
> 
> 2)
> 
> To answer your question in the comment: it looks useful to have some sort of 
> 'extended page fault error code' information here, which shows why the page fault 
> happened. With the regular error_code it's easy - with protection keys there's 16 
> separate keys possible and user-space might not know the actual key value in the 
> pte.

Btw., alternatively we could also say that user-space should know what protection 
key it used when it created the mapping - there's no need to recover it for every 
page fault.

OTOH, as long as we don't do a separate find_vma(), it looks cheap enough to look 
up the pkey value of that address and give it to user-space in the signal frame.

Btw., how does pkey support interact with hugepages?

Thanks,

	Ingo
--
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]

#1232303

On 09/24/2015 02:30 AM, Ingo Molnar wrote:
>> To answer your question in the comment: it looks useful to have some sort of 
>> 'extended page fault error code' information here, which shows why the page fault 
>> happened. With the regular error_code it's easy - with protection keys there's 16 
>> separate keys possible and user-space might not know the actual key value in the 
>> pte.
> 
> Btw., alternatively we could also say that user-space should know what protection 
> key it used when it created the mapping - there's no need to recover it for every 
> page fault.

That's true.  We don't, for instance, tell userspace whether it was a
write that caused a fault.

But, other than smaps we don't have *any* way to tell userspace what
protection key a page has.  I think some mechanism is going to be
required for this to be reasonably debuggable.

> OTOH, as long as we don't do a separate find_vma(), it looks cheap enough to look 
> up the pkey value of that address and give it to user-space in the signal frame.

I still think that find_vma() in this case is pretty darn cheap,
definitely if you compare it to the cost of the entire fault path.

> Btw., how does pkey support interact with hugepages?

Surprisingly little.  I've made sure that everything works with huge
pages and that the (huge) PTEs and VMAs get set up correctly, but I'm
not sure I had to touch the huge page code at all.  I have test code to
ensure that it works the same as with small pages, but everything worked
pretty naturally.
--
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]

#1232621

* Dave Hansen <dave@sr71.net> wrote:

> On 09/24/2015 02:30 AM, Ingo Molnar wrote:
> >> To answer your question in the comment: it looks useful to have some sort of 
> >> 'extended page fault error code' information here, which shows why the page fault 
> >> happened. With the regular error_code it's easy - with protection keys there's 16 
> >> separate keys possible and user-space might not know the actual key value in the 
> >> pte.
> > 
> > Btw., alternatively we could also say that user-space should know what protection 
> > key it used when it created the mapping - there's no need to recover it for every 
> > page fault.
> 
> That's true.  We don't, for instance, tell userspace whether it was a
> write that caused a fault.

I think we do put it into the signal frame, see setup_sigcontext():

                put_user_ex(current->thread.error_code, &sc->err);

and 'error_code & PF_WRITE' tells us whether it's a write fault.

And I'm pretty sure applications like Valgrind rely on this.

> But, other than smaps we don't have *any* way to tell userspace what protection 
> key a page has.  I think some mechanism is going to be required for this to be 
> reasonably debuggable.

I think it's a conceptual extension of sigcontext::err and we need it for similar 
reasons.

> > OTOH, as long as we don't do a separate find_vma(), it looks cheap enough to 
> > look up the pkey value of that address and give it to user-space in the signal 
> > frame.
> 
> I still think that find_vma() in this case is pretty darn cheap, definitely if 
> you compare it to the cost of the entire fault path.

So where's the problem? We have already looked up the vma and know whether there's 
any vma there or not. Why not pass in that pointer and be done with it? Why 
complicate the code by looking up a second time (and exposing us to various 
races)?

> > Btw., how does pkey support interact with hugepages?
> 
> Surprisingly little.  I've made sure that everything works with huge pages and 
> that the (huge) PTEs and VMAs get set up correctly, but I'm not sure I had to 
> touch the huge page code at all.  I have test code to ensure that it works the 
> same as with small pages, but everything worked pretty naturally.

Yeah, so the reason I'm asking about expectations is that this code:

+       follow_ret = follow_pte(tsk->mm, address, &ptep, &ptl);
+       if (!follow_ret) {
+               /*
+                * On a successful follow, make sure to
+                * drop the lock.
+                */
+               pte = *ptep;
+               pte_unmap_unlock(ptep, ptl);
+               ret = pte_pkey(pte);

is visibly hugepage-unsafe: if a vma is hugepage mapped, there are no ptes, only 
pmds - and the protection key index lives in the pmd. We don't seem to recover 
that information properly.

In any case, please put those hugepage tests into tools/tests/selftests/x86/ as 
well, as part of the pkey series.

Thanks,

	Ingo
--
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]

#1233107

On 09/25/2015 12:11 AM, Ingo Molnar wrote:
>>> > > Btw., how does pkey support interact with hugepages?
>> > 
>> > Surprisingly little.  I've made sure that everything works with huge pages and 
>> > that the (huge) PTEs and VMAs get set up correctly, but I'm not sure I had to 
>> > touch the huge page code at all.  I have test code to ensure that it works the 
>> > same as with small pages, but everything worked pretty naturally.
> Yeah, so the reason I'm asking about expectations is that this code:
> 
> +       follow_ret = follow_pte(tsk->mm, address, &ptep, &ptl);
> +       if (!follow_ret) {
> +               /*
> +                * On a successful follow, make sure to
> +                * drop the lock.
> +                */
> +               pte = *ptep;
> +               pte_unmap_unlock(ptep, ptl);
> +               ret = pte_pkey(pte);
> 
> is visibly hugepage-unsafe: if a vma is hugepage mapped, there are no ptes, only 
> pmds - and the protection key index lives in the pmd. We don't seem to recover 
> that information properly.

You got me on this one.  I assumed that follow_pte() handled huge pages.
 It does not.

But, the code still worked.  Since follow_pte() fails for all huge
pages, it just falls back to pulling the protection key out of the VMA,
which _does_ work for huge pages.

I've actually removed the PTE walking and I just now use the VMA
directly.  I don't see a ton of additional value from walking the page
tables when we can get what we need from the VMA.
--
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]

#1233155

* Dave Hansen <dave@sr71.net> wrote:

> On 09/25/2015 12:11 AM, Ingo Molnar wrote:
> >>> > > Btw., how does pkey support interact with hugepages?
> >> > 
> >> > Surprisingly little.  I've made sure that everything works with huge pages and 
> >> > that the (huge) PTEs and VMAs get set up correctly, but I'm not sure I had to 
> >> > touch the huge page code at all.  I have test code to ensure that it works the 
> >> > same as with small pages, but everything worked pretty naturally.
> > Yeah, so the reason I'm asking about expectations is that this code:
> > 
> > +       follow_ret = follow_pte(tsk->mm, address, &ptep, &ptl);
> > +       if (!follow_ret) {
> > +               /*
> > +                * On a successful follow, make sure to
> > +                * drop the lock.
> > +                */
> > +               pte = *ptep;
> > +               pte_unmap_unlock(ptep, ptl);
> > +               ret = pte_pkey(pte);
> > 
> > is visibly hugepage-unsafe: if a vma is hugepage mapped, there are no ptes, only 
> > pmds - and the protection key index lives in the pmd. We don't seem to recover 
> > that information properly.
> 
> You got me on this one.  I assumed that follow_pte() handled huge pages.
>  It does not.
> 
> But, the code still worked.  Since follow_pte() fails for all huge
> pages, it just falls back to pulling the protection key out of the VMA,
> which _does_ work for huge pages.

That might be true for explicit hugetlb vmas, but what about transparent hugepages 
that can show up in regular vmas?

> I've actually removed the PTE walking and I just now use the VMA directly.  I 
> don't see a ton of additional value from walking the page tables when we can get 
> what we need from the VMA.

That's actually good, because it's also cheap, especially if we can get rid of the 
extra find_vma().

and we (thankfully) have no non-linear vmas to worry about anymore.

Thanks,

	Ingo
--
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]

#1233795

On 09/25/2015 11:20 PM, Ingo Molnar wrote:
> * Dave Hansen <dave@sr71.net> wrote:
...
>> Since follow_pte() fails for all huge
>> pages, it just falls back to pulling the protection key out of the VMA,
>> which _does_ work for huge pages.
> 
> That might be true for explicit hugetlb vmas, but what about transparent hugepages 
> that can show up in regular vmas?

All PTEs (large or small) established under a given VMA have the same
protection key.  Any change in protection key for a range will either
change or split the VMA.

So I think it's safe to rely on the VMA entirely.  Well, as least as
safe as the PTE.  It's definitely a wee bit racy, which I'll elaborate
on when I repost the patches.

>> I've actually removed the PTE walking and I just now use the VMA directly.  I 
>> don't see a ton of additional value from walking the page tables when we can get 
>> what we need from the VMA.
> 
> That's actually good, because it's also cheap, especially if we can get rid of the 
> extra find_vma().
> 
> and we (thankfully) have no non-linear vmas to worry about anymore.

Yep.
--
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]

#1233867

* Dave Hansen <dave@sr71.net> wrote:

> On 09/25/2015 11:20 PM, Ingo Molnar wrote:
> > * Dave Hansen <dave@sr71.net> wrote:
> ...
> >> Since follow_pte() fails for all huge
> >> pages, it just falls back to pulling the protection key out of the VMA,
> >> which _does_ work for huge pages.
> > 
> > That might be true for explicit hugetlb vmas, but what about transparent hugepages 
> > that can show up in regular vmas?
> 
> All PTEs (large or small) established under a given VMA have the same
> protection key. [...]

So a 'pte' is only small. The 'large' thing is called a pmd. So follow_pte() is 
not adequate. But with that removed everything should be fine as the vma 
(protection) flags are size independent.

> So I think it's safe to rely on the VMA entirely.  Well, as least as safe as the 
> PTE.  It's definitely a wee bit racy, which I'll elaborate on when I repost the 
> patches.

So the race I can see is wrt. mprotect(), and we should fix that, because the 
existing method of recovering the 'page fault reason', error_code, is not racy - 
so the extension of it (the protection key) should not be racy either.

By the time user-space processes the signal we might race with other threads, but 
at least the fault-address/error-reason information itself should be coherent.

This can be solved by getting the protection key while still under the down_read() 
of the vma - instead of your current solution of a second find_vma().

Thanks,

	Ingo
--
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]

#1232268

Christian, can you tell us how big s390's storage protection keys are?
See the discussion below about siginfo...

On 09/24/2015 02:23 AM, Ingo Molnar wrote:
>> +static u16 fetch_pkey(unsigned long address, struct task_struct *tsk)
>> +{
...
>> +		struct vm_area_struct *vma = find_vma(tsk->mm, address);
>> +		if (vma) {
>> +			ret = vma_pkey(vma);
>> +		} else {
>> +			WARN_ONCE(1, "no PTE or VMA @ %lx\n", address);
>> +			ret = 0;
>> +		}
>> +	}
>> +	return ret;
> 
> Yeah, so I have three observations:
> 
> 1)
> 
> I don't think this warning is entirely right, because this is a fundamentally racy 
> op.
> 
> fetch_pkey(), called by force_sign_info_fault(), can be called while not holding 
> the vma - and if we race with any other thread of the mm, the vma might be gone 
> already.
> 
> So any threaded app using pkeys and vmas in parallel could trigger that WARN_ON().

Agreed.  I'll remove the warning.

> 2)
> 
> And note that this is a somewhat new scenario: in regular page faults, 
> 'error_code' always carries a then-valid cause of the page fault with itself. So 
> we can put that into the siginfo and can be sure that it's the reason for the 
> fault.
> 
> With the above pkey code, we fetch the pte separately from the fault, and without 
> synchronizing with the fault - and we cannot do that, nor do we want to.
> 
> So I think this code should just accept the fact that races may happen. Perhaps 
> warn if we get here with only a single mm user. (but even that would be a bit racy 
> as we don't serialize against exit())

Good point.

> 3)
> 
> For user-space that somehow wants to handle pkeys dynamically and drive them via 
> faults, this seems somewhat inefficient: we already do a find_vma() in the primary 
> fault lookup - and with the typical pkey usecase it will find a vma, just with the 
> wrong access permissions. But when we generate the siginfo here, why do we do a 
> find_vma() again? Why not pass the vma to the siginfo generating function?

My assumption was that the signal generation case was pretty slow.
find_vma() is almost guaranteed to hit the vmacache, and we already hold
mmap_sem, so the cost is pretty tiny.

I'm happy to change it if you're really concerned, but I didn't think it
would be worth the trouble of plumbing it down.

>> --- a/include/uapi/asm-generic/siginfo.h~pkeys-09-siginfo	2015-09-16 10:48:15.584161859 -0700
>> +++ b/include/uapi/asm-generic/siginfo.h	2015-09-16 10:48:15.592162222 -0700
>> @@ -95,6 +95,13 @@ typedef struct siginfo {
>>  				void __user *_lower;
>>  				void __user *_upper;
>>  			} _addr_bnd;
>> +			int _pkey; /* FIXME: protection key value??
>> +				    * Do we really need this in here?
>> +				    * userspace can get the PKRU value in
>> +				    * the signal handler, but they do not
>> +				    * easily have access to the PKEY value
>> +				    * from the PTE.
>> +				    */
>>  		} _sigfault;
> 
> A couple of comments:
> 
> 1)
> 
> Please use our ABI types - this one should be 'u32' I think.
> 
> We could use 'u8' as well here, and mark another 3 bytes next to it as reserved 
> for future flags. Right now protection keys use 4 bits, but do you really think 
> they'll ever grow beyond 8 bits? PTE bits are a scarce resource in general.

I don't expect them to get bigger, at least with anything resembling the
current architecture.  Agreed about the scarcity of PTE bits.

siginfo.h is shared everywhere, so I'd ideally like to put a type in
there that all the other architectures can use.

> 3)
> 
> Please add suitable self-tests to tools/tests/selftests/x86/ that both documents 
> the preferred usage of pkeys, demonstrates all implemented aspects the new ABI and 
> provokes a fault and prints the resulting siginfo, etc.
> 
>> @@ -206,7 +214,8 @@ typedef struct siginfo {
>>  #define SEGV_MAPERR	(__SI_FAULT|1)	/* address not mapped to object */
>>  #define SEGV_ACCERR	(__SI_FAULT|2)	/* invalid permissions for mapped object */
>>  #define SEGV_BNDERR	(__SI_FAULT|3)  /* failed address bound checks */
>> -#define NSIGSEGV	3
>> +#define SEGV_PKUERR	(__SI_FAULT|4)  /* failed address bound checks */
>> +#define NSIGSEGV	4
> 
> You copy & pasted the MPX comment here, it should read something like:
> 
>    #define SEGV_PKUERR	(__SI_FAULT|4)  /* failed protection keys checks */

Whoops.  Will fix.
--
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]

#1234364

Am 24.09.2015 um 19:15 schrieb Dave Hansen:
> Christian, can you tell us how big s390's storage protection keys are?
> See the discussion below about siginfo...

Dave, sorry for the late answer.
s390 storage keys are 4bit for the protection key (and 1 bit for fetch protection, 
change and reference bit) per physical page, so 1 byte is enough for us.

We do not have the storage keys per page table, but for the page frame instead 
(shared among all mappers) so I am not sure if the whole thing will fit for s390.
Having a signal for page protection errors might be useful for us - not sure yet.

Christian

PS: In the past we worked hard to get rid of storage key usage in Linux and are now using
software reference and change tracking to be closer what others do, so its a bit odd to
see other coming with the same idea ;-)

> 
> On 09/24/2015 02:23 AM, Ingo Molnar wrote:
>>> +static u16 fetch_pkey(unsigned long address, struct task_struct *tsk)
>>> +{
> ...
>>> +		struct vm_area_struct *vma = find_vma(tsk->mm, address);
>>> +		if (vma) {
>>> +			ret = vma_pkey(vma);
>>> +		} else {
>>> +			WARN_ONCE(1, "no PTE or VMA @ %lx\n", address);
>>> +			ret = 0;
>>> +		}
>>> +	}
>>> +	return ret;
>>
>> Yeah, so I have three observations:
>>
>> 1)
>>
>> I don't think this warning is entirely right, because this is a fundamentally racy 
>> op.
>>
>> fetch_pkey(), called by force_sign_info_fault(), can be called while not holding 
>> the vma - and if we race with any other thread of the mm, the vma might be gone 
>> already.
>>
>> So any threaded app using pkeys and vmas in parallel could trigger that WARN_ON().
> 
> Agreed.  I'll remove the warning.
> 
>> 2)
>>
>> And note that this is a somewhat new scenario: in regular page faults, 
>> 'error_code' always carries a then-valid cause of the page fault with itself. So 
>> we can put that into the siginfo and can be sure that it's the reason for the 
>> fault.
>>
>> With the above pkey code, we fetch the pte separately from the fault, and without 
>> synchronizing with the fault - and we cannot do that, nor do we want to.
>>
>> So I think this code should just accept the fact that races may happen. Perhaps 
>> warn if we get here with only a single mm user. (but even that would be a bit racy 
>> as we don't serialize against exit())
> 
> Good point.
> 
>> 3)
>>
>> For user-space that somehow wants to handle pkeys dynamically and drive them via 
>> faults, this seems somewhat inefficient: we already do a find_vma() in the primary 
>> fault lookup - and with the typical pkey usecase it will find a vma, just with the 
>> wrong access permissions. But when we generate the siginfo here, why do we do a 
>> find_vma() again? Why not pass the vma to the siginfo generating function?
> 
> My assumption was that the signal generation case was pretty slow.
> find_vma() is almost guaranteed to hit the vmacache, and we already hold
> mmap_sem, so the cost is pretty tiny.
> 
> I'm happy to change it if you're really concerned, but I didn't think it
> would be worth the trouble of plumbing it down.
> 
>>> --- a/include/uapi/asm-generic/siginfo.h~pkeys-09-siginfo	2015-09-16 10:48:15.584161859 -0700
>>> +++ b/include/uapi/asm-generic/siginfo.h	2015-09-16 10:48:15.592162222 -0700
>>> @@ -95,6 +95,13 @@ typedef struct siginfo {
>>>  				void __user *_lower;
>>>  				void __user *_upper;
>>>  			} _addr_bnd;
>>> +			int _pkey; /* FIXME: protection key value??
>>> +				    * Do we really need this in here?
>>> +				    * userspace can get the PKRU value in
>>> +				    * the signal handler, but they do not
>>> +				    * easily have access to the PKEY value
>>> +				    * from the PTE.
>>> +				    */
>>>  		} _sigfault;
>>
>> A couple of comments:
>>
>> 1)
>>
>> Please use our ABI types - this one should be 'u32' I think.
>>
>> We could use 'u8' as well here, and mark another 3 bytes next to it as reserved 
>> for future flags. Right now protection keys use 4 bits, but do you really think 
>> they'll ever grow beyond 8 bits? PTE bits are a scarce resource in general.
> 
> I don't expect them to get bigger, at least with anything resembling the
> current architecture.  Agreed about the scarcity of PTE bits.
> 
> siginfo.h is shared everywhere, so I'd ideally like to put a type in
> there that all the other architectures can use.
> 
>> 3)
>>
>> Please add suitable self-tests to tools/tests/selftests/x86/ that both documents 
>> the preferred usage of pkeys, demonstrates all implemented aspects the new ABI and 
>> provokes a fault and prints the resulting siginfo, etc.
>>
>>> @@ -206,7 +214,8 @@ typedef struct siginfo {
>>>  #define SEGV_MAPERR	(__SI_FAULT|1)	/* address not mapped to object */
>>>  #define SEGV_ACCERR	(__SI_FAULT|2)	/* invalid permissions for mapped object */
>>>  #define SEGV_BNDERR	(__SI_FAULT|3)  /* failed address bound checks */
>>> -#define NSIGSEGV	3
>>> +#define SEGV_PKUERR	(__SI_FAULT|4)  /* failed address bound checks */
>>> +#define NSIGSEGV	4
>>
>> You copy & pasted the MPX comment here, it should read something like:
>>
>>    #define SEGV_PKUERR	(__SI_FAULT|4)  /* failed protection keys checks */
> 
> Whoops.  Will fix.
> 

--
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]

#1234384

On 09/28/2015 12:25 PM, Christian Borntraeger wrote:
> We do not have the storage keys per page table, but for the page frame instead 
> (shared among all mappers) so I am not sure if the whole thing will fit for s390.
> Having a signal for page protection errors might be useful for us - not sure yet.

Ugh, yeah, that's a pretty different architecture.  The stuff we have
here (syscall, VMA flags, etc...) is probably useful to you only for
controlling access to non-shared 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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web