From: "Kirill A. Shutemov" <[email protected]>
Add three new arch_prctl() handles:
- ARCH_CET_ENABLE/DISABLE enables or disables the specified
feature. Returns 0 on success or an error.
- ARCH_CET_LOCK prevents future disabling or enabling of the
specified feature. Returns 0 on success or an error
The features are handled per-thread and inherited over fork(2)/clone(2),
but reset on exec().
This is preparation patch. It does not impelement any features.
Signed-off-by: Kirill A. Shutemov <[email protected]>
[tweaked with feedback from tglx]
Co-developed-by: Rick Edgecombe <[email protected]>
Signed-off-by: Rick Edgecombe <[email protected]>
---
v2:
- Only allow one enable/disable per call (tglx)
- Return error code like a normal arch_prctl() (Alexander Potapenko)
- Make CET only (tglx)
arch/x86/include/asm/cet.h | 20 ++++++++++++++++
arch/x86/include/asm/processor.h | 3 +++
arch/x86/include/uapi/asm/prctl.h | 6 +++++
arch/x86/kernel/process.c | 4 ++++
arch/x86/kernel/process_64.c | 5 +++-
arch/x86/kernel/shstk.c | 38 +++++++++++++++++++++++++++++++
6 files changed, 75 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/include/asm/cet.h
create mode 100644 arch/x86/kernel/shstk.c
diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h
new file mode 100644
index 000000000000..0fa4dbc98c49
--- /dev/null
+++ b/arch/x86/include/asm/cet.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_CET_H
+#define _ASM_X86_CET_H
+
+#ifndef __ASSEMBLY__
+#include <linux/types.h>
+
+struct task_struct;
+
+#ifdef CONFIG_X86_SHADOW_STACK
+long cet_prctl(struct task_struct *task, int option,
+ unsigned long features);
+#else
+static inline long cet_prctl(struct task_struct *task, int option,
+ unsigned long features) { return -EINVAL; }
+#endif /* CONFIG_X86_SHADOW_STACK */
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ASM_X86_CET_H */
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 356308c73951..a92bf76edafe 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -530,6 +530,9 @@ struct thread_struct {
*/
u32 pkru;
+ unsigned long features;
+ unsigned long features_locked;
+
/* Floating point and extended processor state */
struct fpu fpu;
/*
diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h
index 500b96e71f18..028158e35269 100644
--- a/arch/x86/include/uapi/asm/prctl.h
+++ b/arch/x86/include/uapi/asm/prctl.h
@@ -20,4 +20,10 @@
#define ARCH_MAP_VDSO_32 0x2002
#define ARCH_MAP_VDSO_64 0x2003
+/* Don't use 0x3001-0x3004 because of old glibcs */
+
+#define ARCH_CET_ENABLE 0x4001
+#define ARCH_CET_DISABLE 0x4002
+#define ARCH_CET_LOCK 0x4003
+
#endif /* _ASM_X86_PRCTL_H */
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 58a6ea472db9..034880311e6b 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -367,6 +367,10 @@ void arch_setup_new_exec(void)
task_clear_spec_ssb_noexec(current);
speculation_ctrl_update(read_thread_flags());
}
+
+ /* Reset thread features on exec */
+ current->thread.features = 0;
+ current->thread.features_locked = 0;
}
#ifdef CONFIG_X86_IOPL_IOPERM
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 1962008fe743..8fa2c2b7de65 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -829,7 +829,10 @@ long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2)
case ARCH_MAP_VDSO_64:
return prctl_map_vdso(&vdso_image_64, arg2);
#endif
-
+ case ARCH_CET_ENABLE:
+ case ARCH_CET_DISABLE:
+ case ARCH_CET_LOCK:
+ return cet_prctl(task, option, arg2);
default:
ret = -EINVAL;
break;
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
new file mode 100644
index 000000000000..e3276ac9e9b9
--- /dev/null
+++ b/arch/x86/kernel/shstk.c
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * shstk.c - Intel shadow stack support
+ *
+ * Copyright (c) 2021, Intel Corporation.
+ * Yu-cheng Yu <[email protected]>
+ */
+
+#include <linux/sched.h>
+#include <linux/bitops.h>
+#include <asm/prctl.h>
+
+long cet_prctl(struct task_struct *task, int option, unsigned long features)
+{
+ if (option == ARCH_CET_LOCK) {
+ task->thread.features_locked |= features;
+ return 0;
+ }
+
+ /* Don't allow via ptrace */
+ if (task != current)
+ return -EINVAL;
+
+ /* Do not allow to change locked features */
+ if (features & task->thread.features_locked)
+ return -EPERM;
+
+ /* Only support enabling/disabling one feature at a time. */
+ if (hweight_long(features) > 1)
+ return -EINVAL;
+
+ if (option == ARCH_CET_DISABLE) {
+ return -EINVAL;
+ }
+
+ /* Handle ARCH_CET_ENABLE */
+ return -EINVAL;
+}
--
2.17.1
On Thu, Sep 29, 2022 at 03:29:20PM -0700, Rick Edgecombe wrote:
> From: "Kirill A. Shutemov" <[email protected]>
>
> Add three new arch_prctl() handles:
>
> - ARCH_CET_ENABLE/DISABLE enables or disables the specified
> feature. Returns 0 on success or an error.
>
> - ARCH_CET_LOCK prevents future disabling or enabling of the
> specified feature. Returns 0 on success or an error
>
> The features are handled per-thread and inherited over fork(2)/clone(2),
> but reset on exec().
>
> This is preparation patch. It does not impelement any features.
typo: "implement"
>
> Signed-off-by: Kirill A. Shutemov <[email protected]>
> [tweaked with feedback from tglx]
> Co-developed-by: Rick Edgecombe <[email protected]>
> Signed-off-by: Rick Edgecombe <[email protected]>
>
> ---
>
> v2:
> - Only allow one enable/disable per call (tglx)
> - Return error code like a normal arch_prctl() (Alexander Potapenko)
> - Make CET only (tglx)
>
> arch/x86/include/asm/cet.h | 20 ++++++++++++++++
> arch/x86/include/asm/processor.h | 3 +++
> arch/x86/include/uapi/asm/prctl.h | 6 +++++
> arch/x86/kernel/process.c | 4 ++++
> arch/x86/kernel/process_64.c | 5 +++-
> arch/x86/kernel/shstk.c | 38 +++++++++++++++++++++++++++++++
> 6 files changed, 75 insertions(+), 1 deletion(-)
> create mode 100644 arch/x86/include/asm/cet.h
> create mode 100644 arch/x86/kernel/shstk.c
>
> diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h
> new file mode 100644
> index 000000000000..0fa4dbc98c49
> --- /dev/null
> +++ b/arch/x86/include/asm/cet.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_CET_H
> +#define _ASM_X86_CET_H
> +
> +#ifndef __ASSEMBLY__
> +#include <linux/types.h>
> +
> +struct task_struct;
> +
> +#ifdef CONFIG_X86_SHADOW_STACK
> +long cet_prctl(struct task_struct *task, int option,
> + unsigned long features);
> +#else
> +static inline long cet_prctl(struct task_struct *task, int option,
> + unsigned long features) { return -EINVAL; }
> +#endif /* CONFIG_X86_SHADOW_STACK */
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#endif /* _ASM_X86_CET_H */
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 356308c73951..a92bf76edafe 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -530,6 +530,9 @@ struct thread_struct {
> */
> u32 pkru;
>
> + unsigned long features;
> + unsigned long features_locked;
Should these be wrapped in #ifdef CONFIG_X86_SHADOW_STACK (or
CONFIG_X86_CET) ?
Also, just named "features"? Is this expected to be more than CET?
> +
> /* Floating point and extended processor state */
> struct fpu fpu;
> /*
> diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h
> index 500b96e71f18..028158e35269 100644
> --- a/arch/x86/include/uapi/asm/prctl.h
> +++ b/arch/x86/include/uapi/asm/prctl.h
> @@ -20,4 +20,10 @@
> #define ARCH_MAP_VDSO_32 0x2002
> #define ARCH_MAP_VDSO_64 0x2003
>
> +/* Don't use 0x3001-0x3004 because of old glibcs */
> +
> +#define ARCH_CET_ENABLE 0x4001
> +#define ARCH_CET_DISABLE 0x4002
> +#define ARCH_CET_LOCK 0x4003
> +
> #endif /* _ASM_X86_PRCTL_H */
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 58a6ea472db9..034880311e6b 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -367,6 +367,10 @@ void arch_setup_new_exec(void)
> task_clear_spec_ssb_noexec(current);
> speculation_ctrl_update(read_thread_flags());
> }
> +
> + /* Reset thread features on exec */
> + current->thread.features = 0;
> + current->thread.features_locked = 0;
Same ifdef question here.
> }
>
> #ifdef CONFIG_X86_IOPL_IOPERM
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 1962008fe743..8fa2c2b7de65 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -829,7 +829,10 @@ long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2)
> case ARCH_MAP_VDSO_64:
> return prctl_map_vdso(&vdso_image_64, arg2);
> #endif
> -
> + case ARCH_CET_ENABLE:
> + case ARCH_CET_DISABLE:
> + case ARCH_CET_LOCK:
> + return cet_prctl(task, option, arg2);
> default:
> ret = -EINVAL;
> break;
I remain annoyed that prctl interfaces didn't use -ENOTSUP for "unknown
option". :P
> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> new file mode 100644
> index 000000000000..e3276ac9e9b9
> --- /dev/null
> +++ b/arch/x86/kernel/shstk.c
I think the Makefile addition should be moved from "x86/cet/shstk:
Add user-mode shadow stack support" to here, yes? Otherwise, there is a
bisectability randconfig-with-CONFIG_X86_SHADOW_STACK risk here (nothing
will implement "cet_prctl").
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * shstk.c - Intel shadow stack support
> + *
> + * Copyright (c) 2021, Intel Corporation.
> + * Yu-cheng Yu <[email protected]>
> + */
> +
> +#include <linux/sched.h>
> +#include <linux/bitops.h>
> +#include <asm/prctl.h>
> +
> +long cet_prctl(struct task_struct *task, int option, unsigned long features)
> +{
> + if (option == ARCH_CET_LOCK) {
> + task->thread.features_locked |= features;
> + return 0;
> + }
> +
> + /* Don't allow via ptrace */
> + if (task != current)
> + return -EINVAL;
... but locking _is_ allowed via ptrace? If that intended, it should be
explicitly mentioned in the commit log and in a comment here.
Also, perhaps -ESRCH ?
> +
> + /* Do not allow to change locked features */
> + if (features & task->thread.features_locked)
> + return -EPERM;
> +
> + /* Only support enabling/disabling one feature at a time. */
> + if (hweight_long(features) > 1)
> + return -EINVAL;
Perhaps -E2BIG ?
> + if (option == ARCH_CET_DISABLE) {
> + return -EINVAL;
> + }
> +
> + /* Handle ARCH_CET_ENABLE */
> + return -EINVAL;
> +}
> --
> 2.17.1
>
--
Kees Cook
CC Mike about ptrace/CRIU question.
On Mon, 2022-10-03 at 12:01 -0700, Kees Cook wrote:
> On Thu, Sep 29, 2022 at 03:29:20PM -0700, Rick Edgecombe wrote:
> > From: "Kirill A. Shutemov" <[email protected]>
> >
> > Add three new arch_prctl() handles:
> >
> > - ARCH_CET_ENABLE/DISABLE enables or disables the specified
> > feature. Returns 0 on success or an error.
> >
> > - ARCH_CET_LOCK prevents future disabling or enabling of the
> > specified feature. Returns 0 on success or an error
> >
> > The features are handled per-thread and inherited over
> > fork(2)/clone(2),
> > but reset on exec().
> >
> > This is preparation patch. It does not impelement any features.
>
> typo: "implement"
Oops, thanks.
>
> >
> > Signed-off-by: Kirill A. Shutemov <[email protected]>
> > [tweaked with feedback from tglx]
> > Co-developed-by: Rick Edgecombe <[email protected]>
> > Signed-off-by: Rick Edgecombe <[email protected]>
> >
> > ---
> >
> > v2:
> > - Only allow one enable/disable per call (tglx)
> > - Return error code like a normal arch_prctl() (Alexander
> > Potapenko)
> > - Make CET only (tglx)
> >
> > arch/x86/include/asm/cet.h | 20 ++++++++++++++++
> > arch/x86/include/asm/processor.h | 3 +++
> > arch/x86/include/uapi/asm/prctl.h | 6 +++++
> > arch/x86/kernel/process.c | 4 ++++
> > arch/x86/kernel/process_64.c | 5 +++-
> > arch/x86/kernel/shstk.c | 38
> > +++++++++++++++++++++++++++++++
> > 6 files changed, 75 insertions(+), 1 deletion(-)
> > create mode 100644 arch/x86/include/asm/cet.h
> > create mode 100644 arch/x86/kernel/shstk.c
> >
> > diff --git a/arch/x86/include/asm/cet.h
> > b/arch/x86/include/asm/cet.h
> > new file mode 100644
> > index 000000000000..0fa4dbc98c49
> > --- /dev/null
> > +++ b/arch/x86/include/asm/cet.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_X86_CET_H
> > +#define _ASM_X86_CET_H
> > +
> > +#ifndef __ASSEMBLY__
> > +#include <linux/types.h>
> > +
> > +struct task_struct;
> > +
> > +#ifdef CONFIG_X86_SHADOW_STACK
> > +long cet_prctl(struct task_struct *task, int option,
> > + unsigned long features);
> > +#else
> > +static inline long cet_prctl(struct task_struct *task, int option,
> > + unsigned long features) { return -EINVAL; }
> > +#endif /* CONFIG_X86_SHADOW_STACK */
> > +
> > +#endif /* __ASSEMBLY__ */
> > +
> > +#endif /* _ASM_X86_CET_H */
> > diff --git a/arch/x86/include/asm/processor.h
> > b/arch/x86/include/asm/processor.h
> > index 356308c73951..a92bf76edafe 100644
> > --- a/arch/x86/include/asm/processor.h
> > +++ b/arch/x86/include/asm/processor.h
> > @@ -530,6 +530,9 @@ struct thread_struct {
> > */
> > u32 pkru;
> >
> > + unsigned long features;
> > + unsigned long features_locked;
>
> Should these be wrapped in #ifdef CONFIG_X86_SHADOW_STACK (or
> CONFIG_X86_CET) ?
>
> Also, just named "features"? Is this expected to be more than CET?
Sigh, there have been many ideas about how this API and features
tracking could be shared with LAM. At some point there was some
discussion about LAM using the `features` as well, even if it had a
separate arch_prctl() interface. Just checking the last LAM posting,
I'm not sure it needs it. So yes, this could go back to being CET only
for the time being.
>
> > +
> > /* Floating point and extended processor state */
> > struct fpu fpu;
> > /*
> > diff --git a/arch/x86/include/uapi/asm/prctl.h
> > b/arch/x86/include/uapi/asm/prctl.h
> > index 500b96e71f18..028158e35269 100644
> > --- a/arch/x86/include/uapi/asm/prctl.h
> > +++ b/arch/x86/include/uapi/asm/prctl.h
> > @@ -20,4 +20,10 @@
> > #define ARCH_MAP_VDSO_32 0x2002
> > #define ARCH_MAP_VDSO_64 0x2003
> >
> > +/* Don't use 0x3001-0x3004 because of old glibcs */
> > +
> > +#define ARCH_CET_ENABLE 0x4001
> > +#define ARCH_CET_DISABLE 0x4002
> > +#define ARCH_CET_LOCK 0x4003
> > +
> > #endif /* _ASM_X86_PRCTL_H */
> > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > index 58a6ea472db9..034880311e6b 100644
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -367,6 +367,10 @@ void arch_setup_new_exec(void)
> > task_clear_spec_ssb_noexec(current);
> > speculation_ctrl_update(read_thread_flags());
> > }
> > +
> > + /* Reset thread features on exec */
> > + current->thread.features = 0;
> > + current->thread.features_locked = 0;
>
> Same ifdef question here.
>
> > }
> >
> > #ifdef CONFIG_X86_IOPL_IOPERM
> > diff --git a/arch/x86/kernel/process_64.c
> > b/arch/x86/kernel/process_64.c
> > index 1962008fe743..8fa2c2b7de65 100644
> > --- a/arch/x86/kernel/process_64.c
> > +++ b/arch/x86/kernel/process_64.c
> > @@ -829,7 +829,10 @@ long do_arch_prctl_64(struct task_struct
> > *task, int option, unsigned long arg2)
> > case ARCH_MAP_VDSO_64:
> > return prctl_map_vdso(&vdso_image_64, arg2);
> > #endif
> > -
> > + case ARCH_CET_ENABLE:
> > + case ARCH_CET_DISABLE:
> > + case ARCH_CET_LOCK:
> > + return cet_prctl(task, option, arg2);
> > default:
> > ret = -EINVAL;
> > break;
>
> I remain annoyed that prctl interfaces didn't use -ENOTSUP for
> "unknown
> option". :P
>
> > diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> > new file mode 100644
> > index 000000000000..e3276ac9e9b9
> > --- /dev/null
> > +++ b/arch/x86/kernel/shstk.c
>
> I think the Makefile addition should be moved from "x86/cet/shstk:
> Add user-mode shadow stack support" to here, yes? Otherwise, there is
> a
> bisectability randconfig-with-CONFIG_X86_SHADOW_STACK risk here
> (nothing
> will implement "cet_prctl").
Oh, yep, good point.
>
> > @@ -0,0 +1,38 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * shstk.c - Intel shadow stack support
> > + *
> > + * Copyright (c) 2021, Intel Corporation.
> > + * Yu-cheng Yu <[email protected]>
> > + */
> > +
> > +#include <linux/sched.h>
> > +#include <linux/bitops.h>
> > +#include <asm/prctl.h>
> > +
> > +long cet_prctl(struct task_struct *task, int option, unsigned long
> > features)
> > +{
> > + if (option == ARCH_CET_LOCK) {
> > + task->thread.features_locked |= features;
> > + return 0;
> > + }
> > +
> > + /* Don't allow via ptrace */
> > + if (task != current)
> > + return -EINVAL;
>
> ... but locking _is_ allowed via ptrace? If that intended, it should
> be
> explicitly mentioned in the commit log and in a comment here.
I believe CRIU needs to lock via ptrace as well. Maybe Mike can
confirm.
I can mention it.
>
> Also, perhaps -ESRCH ?
>
> > +
> > + /* Do not allow to change locked features */
> > + if (features & task->thread.features_locked)
> > + return -EPERM;
> > +
> > + /* Only support enabling/disabling one feature at a time. */
> > + if (hweight_long(features) > 1)
> > + return -EINVAL;
>
> Perhaps -E2BIG ?
Ehh, I don't know. E2MUCH maybe. It's not necessarily too big. Like if
a third bit was added for IBT some day, you could set SHSTK and WRSS,
it would be invalid, but still be "less" than the valid passing of just
the (hypothetical IBT bit).
>
> > + if (option == ARCH_CET_DISABLE) {
> > + return -EINVAL;
> > + }
> > +
> > + /* Handle ARCH_CET_ENABLE */
> > + return -EINVAL;
> > +}
> > --
> > 2.17.1
> >
>
>
On Mon, Oct 03, 2022 at 10:51:02PM +0000, Edgecombe, Rick P wrote:
> CC Mike about ptrace/CRIU question.
>
> On Mon, 2022-10-03 at 12:01 -0700, Kees Cook wrote:
> > On Thu, Sep 29, 2022 at 03:29:20PM -0700, Rick Edgecombe wrote:
> > > From: "Kirill A. Shutemov" <[email protected]>
> > >
> > > Add three new arch_prctl() handles:
> > >
> > > - ARCH_CET_ENABLE/DISABLE enables or disables the specified
> > > feature. Returns 0 on success or an error.
> > >
> > > - ARCH_CET_LOCK prevents future disabling or enabling of the
> > > specified feature. Returns 0 on success or an error
> > >
> > > The features are handled per-thread and inherited over
> > > fork(2)/clone(2),
> > > but reset on exec().
...
> > > +#include <linux/sched.h>
> > > +#include <linux/bitops.h>
> > > +#include <asm/prctl.h>
> > > +
> > > +long cet_prctl(struct task_struct *task, int option, unsigned long
> > > features)
> > > +{
> > > + if (option == ARCH_CET_LOCK) {
> > > + task->thread.features_locked |= features;
> > > + return 0;
> > > + }
> > > +
> > > + /* Don't allow via ptrace */
> > > + if (task != current)
> > > + return -EINVAL;
> >
> > ... but locking _is_ allowed via ptrace? If that intended, it should
> > be
> > explicitly mentioned in the commit log and in a comment here.
>
> I believe CRIU needs to lock via ptrace as well. Maybe Mike can
> confirm.
Actually, I didn't use ptrace for locking, I did it with "plain"
arch_prctl().
I still can't say for sure CRIU won't need this, I didn't have time yet to
have a closer look at this set.
--
Sincerely yours,
Mike.
* Rick Edgecombe:
> + /* Only support enabling/disabling one feature at a time. */
> + if (hweight_long(features) > 1)
> + return -EINVAL;
This means we'll soon need three extra system calls for x86-64 process
start: SHSTK, IBT, and switching off vsyscall emulation. (The latter
does not need any special CPU support.)
Maybe we can do something else instead to make the strace output a
little bit cleaner?
Thanks,
Florian
On Mon, 2022-10-10 at 12:56 +0200, Florian Weimer wrote:
> > + /* Only support enabling/disabling one feature at a time. */
> > + if (hweight_long(features) > 1)
> > + return -EINVAL;
>
> This means we'll soon need three extra system calls for x86-64
> process
> start: SHSTK, IBT, and switching off vsyscall emulation. (The latter
> does not need any special CPU support.)
>
> Maybe we can do something else instead to make the strace output a
> little bit cleaner?
In previous versions it supported enabling multiple features in a
single syscall. Thomas Gleixner pointed out that (this was on the LAM
patchset that shared the interface at the time) it makes the behavior
of what to do when one feature fails to enable complicated:
https://lore.kernel.org/lkml/87zgjjqico.ffs@tglx/
* Rick P. Edgecombe:
> On Mon, 2022-10-10 at 12:56 +0200, Florian Weimer wrote:
>> > + /* Only support enabling/disabling one feature at a time. */
>> > + if (hweight_long(features) > 1)
>> > + return -EINVAL;
>>
>> This means we'll soon need three extra system calls for x86-64
>> process
>> start: SHSTK, IBT, and switching off vsyscall emulation. (The latter
>> does not need any special CPU support.)
>>
>> Maybe we can do something else instead to make the strace output a
>> little bit cleaner?
>
> In previous versions it supported enabling multiple features in a
> single syscall. Thomas Gleixner pointed out that (this was on the LAM
> patchset that shared the interface at the time) it makes the behavior
> of what to do when one feature fails to enable complicated:
>
> https://lore.kernel.org/lkml/87zgjjqico.ffs@tglx/
Can we return the bits for the features that were actually enabled?
Those three don't have cross-dependencies in the sense that you would
only use X & Y together, but not X or Y alone.
Thanks,
Florian
On Wed, 2022-10-12 at 14:18 +0200, Florian Weimer wrote:
> * Rick P. Edgecombe:
>
> > On Mon, 2022-10-10 at 12:56 +0200, Florian Weimer wrote:
> > > > + /* Only support enabling/disabling one feature at a time.
> > > > */
> > > > + if (hweight_long(features) > 1)
> > > > + return -EINVAL;
> > >
> > > This means we'll soon need three extra system calls for x86-64
> > > process
> > > start: SHSTK, IBT, and switching off vsyscall emulation. (The
> > > latter
> > > does not need any special CPU support.)
> > >
> > > Maybe we can do something else instead to make the strace output
> > > a
> > > little bit cleaner?
> >
> > In previous versions it supported enabling multiple features in a
> > single syscall. Thomas Gleixner pointed out that (this was on the
> > LAM
> > patchset that shared the interface at the time) it makes the
> > behavior
> > of what to do when one feature fails to enable complicated:
> >
> > https://lore.kernel.org/lkml/87zgjjqico.ffs@tglx/
>
> Can we return the bits for the features that were actually enabled?
Actually that specific option is covered in that thread as well. I was
thinking we would need to pass a struct in an out to do a batch
operation. Thomas suggested it could be added later and to start with a
simpler option. Is an extra syscall or two at startup really a big
problem?
> Those three don't have cross-dependencies in the sense that you would
> only use X & Y together, but not X or Y alone.
I don't fully follow this, but WRSS does actually depend on SHSTK.