2017-12-18 18:54:40

by Dave Hansen

[permalink] [raw]
Subject: Re: [PATCH v9 29/51] mm/mprotect, powerpc/mm/pkeys, x86/mm/pkeys: Add sysfs interface

On 11/06/2017 12:57 AM, Ram Pai wrote:
> Expose useful information for programs using memory protection keys.
> Provide implementation for powerpc and x86.
>
> On a powerpc system with pkeys support, here is what is shown:
>
> $ head /sys/kernel/mm/protection_keys/*
> ==> /sys/kernel/mm/protection_keys/disable_access_supported <==
> true

This is cute, but I don't think it should be part of the ABI. Put it in
debugfs if you want it for cute tests. The stuff that this tells you
can and should come from pkey_alloc() for the ABI.

http://man7.org/linux/man-pages/man7/pkeys.7.html

> Any application wanting to use protection keys needs to be able to
> function without them. They might be unavailable because the
> hardware that the application runs on does not support them, the
> kernel code does not contain support, the kernel support has been
> disabled, or because the keys have all been allocated, perhaps by a
> library the application is using. It is recommended that
> applications wanting to use protection keys should simply call
> pkey_alloc(2) and test whether the call succeeds, instead of
> attempting to detect support for the feature in any other way.

Do you really not have standard way on ppc to say whether hardware
features are supported by the kernel? For instance, how do you know if
a given set of registers are known to and are being context-switched by
the kernel?


2017-12-18 22:19:09

by Ram Pai

[permalink] [raw]
Subject: Re: [PATCH v9 29/51] mm/mprotect, powerpc/mm/pkeys, x86/mm/pkeys: Add sysfs interface

On Mon, Dec 18, 2017 at 10:54:26AM -0800, Dave Hansen wrote:
> On 11/06/2017 12:57 AM, Ram Pai wrote:
> > Expose useful information for programs using memory protection keys.
> > Provide implementation for powerpc and x86.
> >
> > On a powerpc system with pkeys support, here is what is shown:
> >
> > $ head /sys/kernel/mm/protection_keys/*

> > ==> /sys/kernel/mm/protection_keys/disable_access_supported <==
> > true
>
> This is cute, but I don't think it should be part of the ABI. Put it in

thanks :)

> debugfs if you want it for cute tests. The stuff that this tells you
> can and should come from pkey_alloc() for the ABI.

Applications can make system calls with different parameters and on
failure determine indirectly that such a feature may not be available in
the kernel/hardware. But from an application point of view, I think, it
is a very clumsy/difficult way to determine that.

For example, an application can keep making pkey_alloc() calls and count
till the call fails, to determine the number of keys supported by the
system. And then the application has to release those keys too. Too
much side-effect just to determine a simple thing. Do we want the
application to endure this pain?

I think we should aim to provide sufficient API/ABI for the application
to consume the feature efficiently, and not any more.

I do not claim that the ABI exposed by this patch is sufficiently
optimal. But I do believe it is tending towards it.

currently the following ABI is exposed.

a) total number of keys available in the system. This information may
not be useful and can possibly be dropped.

b) minimum number of keys available to the application.
if libraries consumes a few, they could provide a library
interface to the application informing the number available to
the application. The library interface can leverage (b) to
provide the information.

c) types of disable-rights supported by keys.
Helps the application to determine the types of disable-features
available. This is helpful, otherwise the app has to
make pkey_alloc() call with the corresponding parameter set
and see if it suceeds or fails. Painful from an application
point of view, in my opinion.

>
> http://man7.org/linux/man-pages/man7/pkeys.7.html
>
> > Any application wanting to use protection keys needs to be able to
> > function without them. They might be unavailable because the
> > hardware that the application runs on does not support them, the
> > kernel code does not contain support, the kernel support has been
> > disabled, or because the keys have all been allocated, perhaps by a
> > library the application is using. It is recommended that
> > applications wanting to use protection keys should simply call
> > pkey_alloc(2) and test whether the call succeeds, instead of
> > attempting to detect support for the feature in any other way.
>
> Do you really not have standard way on ppc to say whether hardware
> features are supported by the kernel? For instance, how do you know if
> a given set of registers are known to and are being context-switched by
> the kernel?

I think on x86 you look for some hardware registers to determine which
hardware features are enabled by the kernel.

We do not have generic support for something like that on ppc.
The kernel looks at the device tree to determine what hardware features
are available. But does not have mechanism to tell the hardware to track
which of its features are currently enabled/used by the kernel; atleast
not for the memory-key feature.

RP

2017-12-18 22:28:27

by Dave Hansen

[permalink] [raw]
Subject: Re: [PATCH v9 29/51] mm/mprotect, powerpc/mm/pkeys, x86/mm/pkeys: Add sysfs interface

On 12/18/2017 02:18 PM, Ram Pai wrote:
> b) minimum number of keys available to the application.
> if libraries consumes a few, they could provide a library
> interface to the application informing the number available to
> the application. The library interface can leverage (b) to
> provide the information.

OK, let's see a real user of this including a few libraries. Then we'll
put it in the kernel.

> c) types of disable-rights supported by keys.
> Helps the application to determine the types of disable-features
> available. This is helpful, otherwise the app has to
> make pkey_alloc() call with the corresponding parameter set
> and see if it suceeds or fails. Painful from an application
> point of view, in my opinion.

Again, let's see a real-world use of this. How does it look? How does
an app "fall back" if it can't set a restriction the way it wants to?

Are we *sure* that such an interface makes sense? For instance, will it
be possible for some keys to be execute-disable while other are only
write-disable?

> I think on x86 you look for some hardware registers to determine which
> hardware features are enabled by the kernel.

No, we use CPUID. It's a part of the ISA that's designed for
enumerating CPU and (sometimes) OS support for CPU features.

> We do not have generic support for something like that on ppc.
> The kernel looks at the device tree to determine what hardware features
> are available. But does not have mechanism to tell the hardware to track
> which of its features are currently enabled/used by the kernel; atleast
> not for the memory-key feature.

Bummer. You're missing out.

But, you could still do this with a syscall. "Hey, kernel, do you
support this feature?"

2017-12-18 23:16:08

by Ram Pai

[permalink] [raw]
Subject: Re: [PATCH v9 29/51] mm/mprotect, powerpc/mm/pkeys, x86/mm/pkeys: Add sysfs interface

On Mon, Dec 18, 2017 at 02:28:14PM -0800, Dave Hansen wrote:
> On 12/18/2017 02:18 PM, Ram Pai wrote:
> > b) minimum number of keys available to the application.
> > if libraries consumes a few, they could provide a library
> > interface to the application informing the number available to
> > the application. The library interface can leverage (b) to
> > provide the information.
>
> OK, let's see a real user of this including a few libraries. Then we'll
> put it in the kernel.
>
> > c) types of disable-rights supported by keys.
> > Helps the application to determine the types of disable-features
> > available. This is helpful, otherwise the app has to
> > make pkey_alloc() call with the corresponding parameter set
> > and see if it suceeds or fails. Painful from an application
> > point of view, in my opinion.
>
> Again, let's see a real-world use of this. How does it look? How does
> an app "fall back" if it can't set a restriction the way it wants to?
>
> Are we *sure* that such an interface makes sense? For instance, will it
> be possible for some keys to be execute-disable while other are only
> write-disable?

Can it be on x86?

its not possible on ppc. the keys can *all* be the-same-attributes-disable all the
time.

However you are right. Its conceivable that some arch could provide a
feature where it can be x-attribute-disable for key 'a' and
y-attribute-disable for key 'b'. But than its a bit of a headache
for an application to consume such a feature.

Ben: I recall you requesting this feature. Thoughts?

>
> > I think on x86 you look for some hardware registers to determine
> > which hardware features are enabled by the kernel.
>
> No, we use CPUID. It's a part of the ISA that's designed for
> enumerating CPU and (sometimes) OS support for CPU features.
>
> > We do not have generic support for something like that on ppc. The
> > kernel looks at the device tree to determine what hardware features
> > are available. But does not have mechanism to tell the hardware to
> > track which of its features are currently enabled/used by the
> > kernel; atleast not for the memory-key feature.
>
> Bummer. You're missing out.
>
> But, you could still do this with a syscall. "Hey, kernel, do you
> support this feature?"

or do powerpc specific sysfs interface.
or a debugfs interface.

RP

2017-12-19 08:31:45

by Gabriel Paubert

[permalink] [raw]
Subject: Re: [PATCH v9 29/51] mm/mprotect, powerpc/mm/pkeys, x86/mm/pkeys: Add sysfs interface

On Mon, Dec 18, 2017 at 03:15:51PM -0800, Ram Pai wrote:
> On Mon, Dec 18, 2017 at 02:28:14PM -0800, Dave Hansen wrote:
> > On 12/18/2017 02:18 PM, Ram Pai wrote:
> > > b) minimum number of keys available to the application.
> > > if libraries consumes a few, they could provide a library
> > > interface to the application informing the number available to
> > > the application. The library interface can leverage (b) to
> > > provide the information.
> >
> > OK, let's see a real user of this including a few libraries. Then we'll
> > put it in the kernel.
> >
> > > c) types of disable-rights supported by keys.
> > > Helps the application to determine the types of disable-features
> > > available. This is helpful, otherwise the app has to
> > > make pkey_alloc() call with the corresponding parameter set
> > > and see if it suceeds or fails. Painful from an application
> > > point of view, in my opinion.
> >
> > Again, let's see a real-world use of this. How does it look? How does
> > an app "fall back" if it can't set a restriction the way it wants to?
> >
> > Are we *sure* that such an interface makes sense? For instance, will it
> > be possible for some keys to be execute-disable while other are only
> > write-disable?
>
> Can it be on x86?
>
> its not possible on ppc. the keys can *all* be the-same-attributes-disable all the
> time.
>
> However you are right. Its conceivable that some arch could provide a
> feature where it can be x-attribute-disable for key 'a' and
> y-attribute-disable for key 'b'. But than its a bit of a headache
> for an application to consume such a feature.
>
> Ben: I recall you requesting this feature. Thoughts?
>
> >
> > > I think on x86 you look for some hardware registers to determine
> > > which hardware features are enabled by the kernel.
> >
> > No, we use CPUID. It's a part of the ISA that's designed for
> > enumerating CPU and (sometimes) OS support for CPU features.
> >
> > > We do not have generic support for something like that on ppc. The
> > > kernel looks at the device tree to determine what hardware features
> > > are available. But does not have mechanism to tell the hardware to
> > > track which of its features are currently enabled/used by the
> > > kernel; atleast not for the memory-key feature.
> >
> > Bummer. You're missing out.
> >
> > But, you could still do this with a syscall. "Hey, kernel, do you
> > support this feature?"
>
> or do powerpc specific sysfs interface.
> or a debugfs interface.

getauxval(3) ?

With AT_HWCAP or HWCAP2 as parameter already gives information about
features supported by the hardware and the kernel.

Taking one bit to expose the availability of protection keys to
applications does not look impossible.

Do I miss something obvious?

Gabriel

2017-12-19 10:50:37

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH v9 29/51] mm/mprotect, powerpc/mm/pkeys, x86/mm/pkeys: Add sysfs interface

Dave Hansen <[email protected]> writes:

> On 11/06/2017 12:57 AM, Ram Pai wrote:
>> Expose useful information for programs using memory protection keys.
>> Provide implementation for powerpc and x86.
>>
>> On a powerpc system with pkeys support, here is what is shown:
>>
>> $ head /sys/kernel/mm/protection_keys/*
>> ==> /sys/kernel/mm/protection_keys/disable_access_supported <==
>> true
>
> This is cute, but I don't think it should be part of the ABI. Put it in
> debugfs if you want it for cute tests. The stuff that this tells you
> can and should come from pkey_alloc() for the ABI.

Yeah I agree this is not sysfs material.

In particular the total/usable numbers are completely useless vs other
threads allocating pkeys out from under you.

> http://man7.org/linux/man-pages/man7/pkeys.7.html
>
>> Any application wanting to use protection keys needs to be able to
>> function without them. They might be unavailable because the
>> hardware that the application runs on does not support them, the
>> kernel code does not contain support, the kernel support has been
>> disabled, or because the keys have all been allocated, perhaps by a
>> library the application is using. It is recommended that
>> applications wanting to use protection keys should simply call
>> pkey_alloc(2) and test whether the call succeeds, instead of
>> attempting to detect support for the feature in any other way.
>
> Do you really not have standard way on ppc to say whether hardware
> features are supported by the kernel? For instance, how do you know if
> a given set of registers are known to and are being context-switched by
> the kernel?

Yes we do, we emit feature bits in the AT_HWCAP entry of the aux vector,
same as some other architectures.

But I don't see the need to use a feature bit for pkeys. If they're not
supported then pkey_alloc() will just always fail. Apps have to handle
that anyway because keys are a finite resource.

cheers

2017-12-19 16:22:40

by Ram Pai

[permalink] [raw]
Subject: Re: [PATCH v9 29/51] mm/mprotect, powerpc/mm/pkeys, x86/mm/pkeys: Add sysfs interface

On Tue, Dec 19, 2017 at 09:31:22AM +0100, Gabriel Paubert wrote:
> On Mon, Dec 18, 2017 at 03:15:51PM -0800, Ram Pai wrote:
> > On Mon, Dec 18, 2017 at 02:28:14PM -0800, Dave Hansen wrote:
> > > On 12/18/2017 02:18 PM, Ram Pai wrote:
> > >
....snip...
> > > > I think on x86 you look for some hardware registers to determine
> > > > which hardware features are enabled by the kernel.
> > >
> > > No, we use CPUID. It's a part of the ISA that's designed for
> > > enumerating CPU and (sometimes) OS support for CPU features.
> > >
> > > > We do not have generic support for something like that on ppc. The
> > > > kernel looks at the device tree to determine what hardware features
> > > > are available. But does not have mechanism to tell the hardware to
> > > > track which of its features are currently enabled/used by the
> > > > kernel; atleast not for the memory-key feature.
> > >
> > > Bummer. You're missing out.
> > >
> > > But, you could still do this with a syscall. "Hey, kernel, do you
> > > support this feature?"
> >
> > or do powerpc specific sysfs interface.
> > or a debugfs interface.
>
> getauxval(3) ?
>
> With AT_HWCAP or HWCAP2 as parameter already gives information about
> features supported by the hardware and the kernel.
>
> Taking one bit to expose the availability of protection keys to
> applications does not look impossible.
>
> Do I miss something obvious?

No. I am told this is possible aswell.

RP

2017-12-19 16:32:43

by Ram Pai

[permalink] [raw]
Subject: Re: [PATCH v9 29/51] mm/mprotect, powerpc/mm/pkeys, x86/mm/pkeys: Add sysfs interface

On Tue, Dec 19, 2017 at 09:50:24PM +1100, Michael Ellerman wrote:
> Dave Hansen <[email protected]> writes:
>
> > On 11/06/2017 12:57 AM, Ram Pai wrote:
> >> Expose useful information for programs using memory protection keys.
> >> Provide implementation for powerpc and x86.
> >>
> >> On a powerpc system with pkeys support, here is what is shown:
> >>
> >> $ head /sys/kernel/mm/protection_keys/*
> >> ==> /sys/kernel/mm/protection_keys/disable_access_supported <==
> >> true
> >
> > This is cute, but I don't think it should be part of the ABI. Put it in
> > debugfs if you want it for cute tests. The stuff that this tells you
> > can and should come from pkey_alloc() for the ABI.
>
> Yeah I agree this is not sysfs material.
>
> In particular the total/usable numbers are completely useless vs other
> threads allocating pkeys out from under you.

The usable number is the minimum number of keys available for use by the
application, not the number of keys **currently** available. Its a
static number.

I am dropping this patch. We can revisit this when a clear request for
such a feature emerges.

>
> >
> >> Any application wanting to use protection keys needs to be able to
> >> function without them. They might be unavailable because the
> >> hardware that the application runs on does not support them, the
> >> kernel code does not contain support, the kernel support has been
> >> disabled, or because the keys have all been allocated, perhaps by a
> >> library the application is using. It is recommended that
> >> applications wanting to use protection keys should simply call
> >> pkey_alloc(2) and test whether the call succeeds, instead of
> >> attempting to detect support for the feature in any other way.
> >
> > Do you really not have standard way on ppc to say whether hardware
> > features are supported by the kernel? For instance, how do you know if
> > a given set of registers are known to and are being context-switched by
> > the kernel?
>
> Yes we do, we emit feature bits in the AT_HWCAP entry of the aux vector,
> same as some other architectures.

Ah. I was not aware of this.
Thanks,
RP

2017-12-19 21:35:54

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH v9 29/51] mm/mprotect, powerpc/mm/pkeys, x86/mm/pkeys: Add sysfs interface

On Mon, 2017-12-18 at 14:28 -0800, Dave Hansen wrote:
> > We do not have generic support for something like that on ppc.
> > The kernel looks at the device tree to determine what hardware features
> > are available. But does not have mechanism to tell the hardware to track
> > which of its features are currently enabled/used by the kernel; atleast
> > not for the memory-key feature.
>
> Bummer. You're missing out.
>
> But, you could still do this with a syscall. "Hey, kernel, do you
> support this feature?"

I'm not sure I understand Ram's original (quoted) point, but informing
userspace of CPU features is what AT_HWCAP's are about.

Ben.

2017-12-20 17:50:37

by Ram Pai

[permalink] [raw]
Subject: Re: [PATCH v9 29/51] mm/mprotect, powerpc/mm/pkeys, x86/mm/pkeys: Add sysfs interface

On Wed, Dec 20, 2017 at 08:34:56AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2017-12-18 at 14:28 -0800, Dave Hansen wrote:
> > > We do not have generic support for something like that on ppc.
> > > The kernel looks at the device tree to determine what hardware features
> > > are available. But does not have mechanism to tell the hardware to track
> > > which of its features are currently enabled/used by the kernel; atleast
> > > not for the memory-key feature.
> >
> > Bummer. You're missing out.
> >
> > But, you could still do this with a syscall. "Hey, kernel, do you
> > support this feature?"
>
> I'm not sure I understand Ram's original (quoted) point, but informing
> userspace of CPU features is what AT_HWCAP's are about.

Ben, my original point was -- we developed this patch to satisfy a concern
you raised back on July 11th; cut-n-pasted below.

-------------------------------------------------------------------
That leads to the question... How do you tell userspace.

(apologies if I missed that in an existing patch in the series)

How do we inform userspace of the key capabilities ? There are
at least two things userspace may want to know already:

- What protection bits are supported for a key

- How many keys exist

- Which keys are available for use by userspace. On PowerPC,
the kernel can reserve some keys for itself, so can the
hypervisor. In fact, they do.
--------------------------------------------------------------------


The argument against this patch is -- it should not be baked into
the ABI as yet, since we do not have clarity on what applications need.

As it stands today the only way to figure out the information from
userspace is by probing the kernel through calls to sys_pkey_alloc().

AT_HWCAP can be used, but that will certainly not be capable of
providing all the information that userspace might expect.

Your thoughts?
RP

2017-12-21 00:30:31

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH v9 29/51] mm/mprotect, powerpc/mm/pkeys, x86/mm/pkeys: Add sysfs interface

On Wed, 2017-12-20 at 09:50 -0800, Ram Pai wrote:
> The argument against this patch is -- it should not be baked into
> the ABI as yet, since we do not have clarity on what applications need.
>
> As it stands today the only way to figure out the information from
> userspace is by probing the kernel through calls to sys_pkey_alloc().
>
> AT_HWCAP can be used, but that will certainly not be capable of
> providing all the information that userspace might expect.
>
> Your thoughts?

Well, there's one well known application wanting that whole keys
business, so why not ask them what works for them ?

In the meantime, that shouldn't block the rest of the patches.

Cheers,
Ben.