Hi all,
Le torstaina 18. toukokuuta 2023 19.19.43 EEST, vous avez écrit :
> This patch add two riscv-specific prctls, to allow usespace control the
> use of vector unit:
>
> * PR_RISCV_V_SET_CONTROL: control the permission to use Vector at next,
> or all following execve for a thread. Turning off a thread's Vector
> live is not possible since libraries may have registered ifunc that
> may execute Vector instructions.
> * PR_RISCV_V_GET_CONTROL: get the same permission setting for the
> current thread, and the setting for following execve(s).
So far the story was that if the nth bit in the ELF HWCAP auxillary vector was
set, then the nth single lettered extension was supported. There is already
userspace code out there that expects this of the V bit. (I know I have
written such code, and I also know others did likewise.) This is how it
already works for the D and F bits.
Admittedly, upstream Linux has never ever set that bit to this day. But still,
if we end up with the bit set in a process that has had V support disabled by
the parent (or the sysctl), existing userspace will encounter SIGILL and
break.
IMO, the bit must be masked not only whence the kernel lacks V support (as
PATCH 02 does), but also if the process starts with V disabled.
There are two ways to achieve this:
1) V is never ever set, and userspace is forced to use hwprobe() instead.
2) V is set only in processes starting with V enabled (and it's their own
fault if they disabled it in future child threads).
Br,
--
レミ・デニ-クールモン
http://www.remlab.net/
On Sun, May 21, 2023 at 1:41 PM Rémi Denis-Courmont <[email protected]> wrote:
>
> Hi all,
>
> Le torstaina 18. toukokuuta 2023 19.19.43 EEST, vous avez écrit :
> > This patch add two riscv-specific prctls, to allow usespace control the
> > use of vector unit:
> >
> > * PR_RISCV_V_SET_CONTROL: control the permission to use Vector at next,
> > or all following execve for a thread. Turning off a thread's Vector
> > live is not possible since libraries may have registered ifunc that
> > may execute Vector instructions.
> > * PR_RISCV_V_GET_CONTROL: get the same permission setting for the
> > current thread, and the setting for following execve(s).
>
> So far the story was that if the nth bit in the ELF HWCAP auxillary vector was
> set, then the nth single lettered extension was supported. There is already
> userspace code out there that expects this of the V bit. (I know I have
> written such code, and I also know others did likewise.) This is how it
> already works for the D and F bits.
Yes, the V bit in ELF_HWCAP becomes vague in this series.
>
> Admittedly, upstream Linux has never ever set that bit to this day. But still,
> if we end up with the bit set in a process that has had V support disabled by
> the parent (or the sysctl), existing userspace will encounter SIGILL and
> break.
>
> IMO, the bit must be masked not only whence the kernel lacks V support (as
> PATCH 02 does), but also if the process starts with V disabled.
This is going to change ELF_HWCAP from a macro to a function. The
function will turn on COMPAT_HWCAP_ISA_V iff V is supported and
allowed. I am going to do this in v21 If this looks sane. i.e.
Currently I don't see other architectures which give different
ELF_HWCAP values on each execve. If ELF_HWCAP is not a right place to
encode the information then userspace has to make the prctl() call to
be certain on whether V is usable.
>
> There are two ways to achieve this:
> 1) V is never ever set, and userspace is forced to use hwprobe() instead.
> 2) V is set only in processes starting with V enabled (and it's their own
> fault if they disabled it in future child threads).
The prctl() interface does not allow processes to turn off V once it
is enabled in its current (execve) context. The process can only
disable V when the next execve() happens. Then, if we implement
ELF_HWCAP as mentioned above, the kernel will reload a new HWCAP for
the process. By then, the new HWCAP will have V masked since it is not
allowed.
>
> Br,
>
> --
> レミ・デニ-クールモン
> http://www.remlab.net/
>
>
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Thanks,
Andy
Hi,
Le 22 mai 2023 11:28:28 GMT+03:00, Andy Chiu <[email protected]> a écrit :
>On Sun, May 21, 2023 at 1:41 PM Rémi Denis-Courmont <[email protected]> wrote:
>>
>> Hi all,
>>
>> Le torstaina 18. toukokuuta 2023 19.19.43 EEST, vous avez écrit :
>> > This patch add two riscv-specific prctls, to allow usespace control the
>> > use of vector unit:
>> >
>> > * PR_RISCV_V_SET_CONTROL: control the permission to use Vector at next,
>> > or all following execve for a thread. Turning off a thread's Vector
>> > live is not possible since libraries may have registered ifunc that
>> > may execute Vector instructions.
>> > * PR_RISCV_V_GET_CONTROL: get the same permission setting for the
>> > current thread, and the setting for following execve(s).
>>
>> So far the story was that if the nth bit in the ELF HWCAP auxillary vector was
>> set, then the nth single lettered extension was supported. There is already
>> userspace code out there that expects this of the V bit. (I know I have
>> written such code, and I also know others did likewise.) This is how it
>> already works for the D and F bits.
>
>Yes, the V bit in ELF_HWCAP becomes vague in this series.
>> Admittedly, upstream Linux has never ever set that bit to this day. But still,
>> if we end up with the bit set in a process that has had V support disabled by
>> the parent (or the sysctl), existing userspace will encounter SIGILL and
>> break.
>>
>> IMO, the bit must be masked not only whence the kernel lacks V support (as
>> PATCH 02 does), but also if the process starts with V disabled.
>
>This is going to change ELF_HWCAP from a macro to a function. The
>function will turn on COMPAT_HWCAP_ISA_V iff V is supported and
>allowed. I am going to do this in v21 If this looks sane. i.e.
>Currently I don't see other architectures which give different
>ELF_HWCAP values on each execve. If ELF_HWCAP is not a right place to
>encode the information then userspace has to make the prctl() call to
>be certain on whether V is usable.
I don't think the value of an auxillary vector entry can change in an existing process nor that we need that. If an application starts with V disabled, you can keep the V bit clear even if V gets enabled later on; that won't break existing userspace code, which simply won't use vectors.
What does break existing userspace is setting the V bit whilst vectors are disabled.
>
>>
>> There are two ways to achieve this:
>> 1) V is never ever set, and userspace is forced to use hwprobe() instead.
>> 2) V is set only in processes starting with V enabled (and it's their own
>> fault if they disabled it in future child threads).
>
>The prctl() interface does not allow processes to turn off V once it
>is enabled in its current (execve) context. The process can only
>disable V when the next execve() happens. Then, if we implement
>ELF_HWCAP as mentioned above, the kernel will reload a new HWCAP for
>the process. By then, the new HWCAP will have V masked since it is not
>allowed.
>
>>
>> Br,
>>
>> --
>> レミ・デニ-クールモン
>> http://www.remlab.net/
>>
>>
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>Thanks,
>Andy
>
On Mon, 22 May 2023 02:58:45 PDT (-0700), [email protected] wrote:
> Hi,
>
> Le 22 mai 2023 11:28:28 GMT+03:00, Andy Chiu <[email protected]> a écrit :
>>On Sun, May 21, 2023 at 1:41 PM Rémi Denis-Courmont <[email protected]> wrote:
>>>
>>> Hi all,
>>>
>>> Le torstaina 18. toukokuuta 2023 19.19.43 EEST, vous avez écrit :
>>> > This patch add two riscv-specific prctls, to allow usespace control the
>>> > use of vector unit:
>>> >
>>> > * PR_RISCV_V_SET_CONTROL: control the permission to use Vector at next,
>>> > or all following execve for a thread. Turning off a thread's Vector
>>> > live is not possible since libraries may have registered ifunc that
>>> > may execute Vector instructions.
>>> > * PR_RISCV_V_GET_CONTROL: get the same permission setting for the
>>> > current thread, and the setting for following execve(s).
>>>
>>> So far the story was that if the nth bit in the ELF HWCAP auxillary vector was
>>> set, then the nth single lettered extension was supported. There is already
>>> userspace code out there that expects this of the V bit. (I know I have
>>> written such code, and I also know others did likewise.) This is how it
>>> already works for the D and F bits.
>>
>>Yes, the V bit in ELF_HWCAP becomes vague in this series.
>
>
>>> Admittedly, upstream Linux has never ever set that bit to this day. But still,
>>> if we end up with the bit set in a process that has had V support disabled by
>>> the parent (or the sysctl), existing userspace will encounter SIGILL and
>>> break.
>>>
>>> IMO, the bit must be masked not only whence the kernel lacks V support (as
>>> PATCH 02 does), but also if the process starts with V disabled.
>>
>>This is going to change ELF_HWCAP from a macro to a function. The
>>function will turn on COMPAT_HWCAP_ISA_V iff V is supported and
>>allowed. I am going to do this in v21 If this looks sane. i.e.
>>Currently I don't see other architectures which give different
>>ELF_HWCAP values on each execve. If ELF_HWCAP is not a right place to
>>encode the information then userspace has to make the prctl() call to
>>be certain on whether V is usable.
>
> I don't think the value of an auxillary vector entry can change in an existing process nor that we need that. If an application starts with V disabled, you can keep the V bit clear even if V gets enabled later on; that won't break existing userspace code, which simply won't use vectors.
>
> What does break existing userspace is setting the V bit whilst vectors are disabled.
So maybe the right answer is to just not set V at all? The
single-letter extensions are sort of defunct now, there's multi-letter
sub extensions for most things, but V got ratified with those
sub-extensions so we could just call it extra-ambiguous?
>
>
>
>>
>>>
>>> There are two ways to achieve this:
>>> 1) V is never ever set, and userspace is forced to use hwprobe() instead.
>>> 2) V is set only in processes starting with V enabled (and it's their own
>>> fault if they disabled it in future child threads).
>>
>>The prctl() interface does not allow processes to turn off V once it
>>is enabled in its current (execve) context. The process can only
>>disable V when the next execve() happens. Then, if we implement
>>ELF_HWCAP as mentioned above, the kernel will reload a new HWCAP for
>>the process. By then, the new HWCAP will have V masked since it is not
>>allowed.
>>
>>>
>>> Br,
>>>
>>> --
>>> レミ・デニ-クールモン
>>> http://www.remlab.net/
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> linux-riscv mailing list
>>> [email protected]
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>
>>Thanks,
>>Andy
>>
How about adding the following 2 patches to deal with the problem.
However, I am concerned if this makes ELF_HWCAP too complicated. e.g.
Future extensions may as well want to do something here after we create
an entry here.
Thanks,
Andy
[-- Attachment #1: patch.diff --]
From: Andy Chiu <[email protected]>
Date: Wed, 24 May 2023 08:00:11 +0000
Subject: [PATCH 1/2] riscv: hwcap: change ELF_HWCAP to a function
Signed-off-by: Andy Chiu <[email protected]>
---
arch/riscv/include/asm/elf.h | 2 +-
arch/riscv/include/asm/hwcap.h | 2 ++
arch/riscv/kernel/cpufeature.c | 5 +++++
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
index ca23c4f6c440..c24280774caf 100644
--- a/arch/riscv/include/asm/elf.h
+++ b/arch/riscv/include/asm/elf.h
@@ -66,7 +66,7 @@ extern bool compat_elf_check_arch(Elf32_Ehdr *hdr);
* via a bitmap that coorespends to each single-letter ISA extension. This is
* essentially defunct, but will remain for compatibility with userspace.
*/
-#define ELF_HWCAP (elf_hwcap & ((1UL << RISCV_ISA_EXT_BASE) - 1))
+#define ELF_HWCAP riscv_get_elf_hwcap()
extern unsigned long elf_hwcap;
/*
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 574385930ba7..e6c288ac4581 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -61,6 +61,8 @@
#include <linux/jump_label.h>
+unsigned long riscv_get_elf_hwcap(void);
+
struct riscv_isa_ext_data {
/* Name of the extension displayed to userspace via /proc/cpuinfo */
char uprop[RISCV_ISA_EXT_NAME_LEN_MAX];
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 28032b083463..29c0680652a0 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -293,6 +293,11 @@ void __init riscv_fill_hwcap(void)
pr_info("riscv: ELF capabilities %s\n", print_str);
}
+unsigned long riscv_get_elf_hwcap(void)
+{
+ return (elf_hwcap & ((1UL << RISCV_ISA_EXT_BASE) - 1));
+}
+
#ifdef CONFIG_RISCV_ALTERNATIVE
/*
* Alternative patch sites consider 48 bits when determining when to patch
--
2.17.1
[-- Attachment #2: patch2.diff --]
From: Andy Chiu <[email protected]>
Date: Wed, 24 May 2023 08:00:11 +0000
Subject: [PATCH 1/2] riscv: hwcap: change ELF_HWCAP to a function
Signed-off-by: Andy Chiu <[email protected]>
---
arch/riscv/include/asm/elf.h | 2 +-
arch/riscv/include/asm/hwcap.h | 2 ++
arch/riscv/kernel/cpufeature.c | 5 +++++
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
index ca23c4f6c440..c24280774caf 100644
--- a/arch/riscv/include/asm/elf.h
+++ b/arch/riscv/include/asm/elf.h
@@ -66,7 +66,7 @@ extern bool compat_elf_check_arch(Elf32_Ehdr *hdr);
* via a bitmap that coorespends to each single-letter ISA extension. This is
* essentially defunct, but will remain for compatibility with userspace.
*/
-#define ELF_HWCAP (elf_hwcap & ((1UL << RISCV_ISA_EXT_BASE) - 1))
+#define ELF_HWCAP riscv_get_elf_hwcap()
extern unsigned long elf_hwcap;
/*
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 574385930ba7..e6c288ac4581 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -61,6 +61,8 @@
#include <linux/jump_label.h>
+unsigned long riscv_get_elf_hwcap(void);
+
struct riscv_isa_ext_data {
/* Name of the extension displayed to userspace via /proc/cpuinfo */
char uprop[RISCV_ISA_EXT_NAME_LEN_MAX];
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 28032b083463..29c0680652a0 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -293,6 +293,11 @@ void __init riscv_fill_hwcap(void)
pr_info("riscv: ELF capabilities %s\n", print_str);
}
+unsigned long riscv_get_elf_hwcap(void)
+{
+ return (elf_hwcap & ((1UL << RISCV_ISA_EXT_BASE) - 1));
+}
+
#ifdef CONFIG_RISCV_ALTERNATIVE
/*
* Alternative patch sites consider 48 bits when determining when to patch
--
2.17.1
Le keskiviikkona 24. toukokuuta 2023, 12.25.30 EEST Andy Chiu a écrit :
> How about adding the following 2 patches to deal with the problem.
> However, I am concerned if this makes ELF_HWCAP too complicated. e.g.
> Future extensions may as well want to do something here after we create
> an entry here.
Fine with me but I feel that we are missing the bigger picture here, as to
what the usage pattern/guideline for the prctl() on userspace side should be?
Maybe it was already discussed, and I just missed it, but I couldn't find it in
the documentation.
--
Rémi Denis-Courmont
http://www.remlab.net/
Le keskiviikkona 24. toukokuuta 2023, 3.18.26 EEST Palmer Dabbelt a écrit :
> > I don't think the value of an auxillary vector entry can change in an
> > existing process nor that we need that. If an application starts with V
> > disabled, you can keep the V bit clear even if V gets enabled later on;
> > that won't break existing userspace code, which simply won't use vectors.
> >
> > What does break existing userspace is setting the V bit whilst vectors are
> > disabled.
> So maybe the right answer is to just not set V at all?
That is one possibility that I can live, although it feels unnecessarily user-
hostile compared to setting it only if the process _started_ with V enabled.
> The
> single-letter extensions are sort of defunct now, there's multi-letter
> sub extensions for most things, but V got ratified with those
> sub-extensions so we could just call it extra-ambiguous?
Maybe; I must admit I have zero visibility to RVI inner workings. At least C,
D and F bits could work for JIT use cases, I suppose. E and M are totally
impractical to support. G, I, X and Z cannot are already wasted by the design,
and I guess we will now waste all 16 others.
But as for V, what is the user-space story for the prctl()? Who is intended to
enablet V mode? If there is no clear story, it is all but guaranteed that
random libraries will call it, and just _blindly_ assume that there is enough
stack space for signal handling. If so, then there is not much point having a
prctl() in the first place; might as well stick to just a kernel Kconfig with no
runtime configuration.
--
Rémi Denis-Courmont
http://www.remlab.net/
On Thu, May 25, 2023 at 12:16 AM Rémi Denis-Courmont <[email protected]> wrote:
>
> Le keskiviikkona 24. toukokuuta 2023, 12.25.30 EEST Andy Chiu a écrit :
> > How about adding the following 2 patches to deal with the problem.
> > However, I am concerned if this makes ELF_HWCAP too complicated. e.g.
> > Future extensions may as well want to do something here after we create
> > an entry here.
>
> Fine with me but I feel that we are missing the bigger picture here, as to
> what the usage pattern/guideline for the prctl() on userspace side should be?
Yes, I agree the use of this prctl is only meaningful for the init
system, or some ansenstor processes that make policy decisions. And
with ELF_HWCAP reflecting per-process's availability of V, which is
decided by either the prctl/sysctl, programs/libraries do not really
need to call this prctl unless they are doing aforementioned stuffs.
I am going to document these when rolling out the next revision.
>
> Maybe it was already discussed, and I just missed it, but I couldn't find it in
> the documentation.
We had discussed the potential theoretical abi breakage[1], and came
out with a need of a prctl/sysctl interface for distro's early
userspace programs.
>
> --
> Rémi Denis-Courmont
> http://www.remlab.net/
>
>
>
[1]:https://lore.kernel.org/all/mhng-47aa965a-2b25-4ac0-984a-c2e6f3a051ee@palmer-ri-x1c9/
Thanks,
Andy