2022-06-22 21:34:09

by Mathieu Desnoyers

Subject: [PATCH 1/2] rseq: Deprecate RSEQ_CS_FLAG_NO_RESTART_ON_* flags

The pretty much unused RSEQ_CS_FLAG_NO_RESTART_ON_* flags introduce
complexity in rseq, and are subtly buggy [1]. Solving those issues
requires introducing additional complexity in the rseq implementation
for each supported architecture.

Considering that it complexifies the rseq ABI, I am proposing that we
deprecate those flags. [2]

So far there appears to be consensus from maintainers of user-space
projects impacted by this feature that its removal would be a welcome
simplification. [3]

The deprecation approach proposed here is to issue WARN_ON_ONCE() when
encountering those flags and kill the offending process with sigsegv.
This should allow us to quickly identify whether anyone yells at us for
removing this.

Link: https://lore.kernel.org/lkml/[email protected]/ [1]
Link: https://lore.kernel.org/lkml/[email protected]/ [2]
Link: https://lore.kernel.org/lkml/[email protected]/ [3]
Signed-off-by: Mathieu Desnoyers <[email protected]>
---
kernel/rseq.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/kernel/rseq.c b/kernel/rseq.c
index 97ac20b4f738..81d7dc80787b 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -18,8 +18,9 @@
#define CREATE_TRACE_POINTS
#include <trace/events/rseq.h>

-#define RSEQ_CS_PREEMPT_MIGRATE_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE | \
- RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT)
+#define RSEQ_CS_NO_RESTART_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT | \
+ RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL | \
+ RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE)

/*
*
@@ -175,23 +176,15 @@ static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
u32 flags, event_mask;
int ret;

+ if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS))
+ return -EINVAL;
+
/* Get thread flags. */
ret = get_user(flags, &t->rseq->flags);
if (ret)
return ret;

- /* Take critical section flags into account. */
- flags |= cs_flags;
-
- /*
- * Restart on signal can only be inhibited when restart on
- * preempt and restart on migrate are inhibited too. Otherwise,
- * a preempted signal handler could fail to restart the prior
- * execution context on sigreturn.
- */
- if (unlikely((flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL) &&
- (flags & RSEQ_CS_PREEMPT_MIGRATE_FLAGS) !=
- RSEQ_CS_PREEMPT_MIGRATE_FLAGS))
+ if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
return -EINVAL;

/*
@@ -203,7 +196,7 @@ static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
t->rseq_event_mask = 0;
preempt_enable();

- return !!(event_mask & ~flags);
+ return !!event_mask;
}

static int clear_rseq_cs(struct task_struct *t)
--
2.30.2

2022-07-26 19:20:36

by Mathieu Desnoyers

Subject: Re: [PATCH 1/2] rseq: Deprecate RSEQ_CS_FLAG_NO_RESTART_ON_* flags

----- On Jun 22, 2022, at 3:46 PM, Mathieu Desnoyers [email protected] wrote:

> The pretty much unused RSEQ_CS_FLAG_NO_RESTART_ON_* flags introduce
> complexity in rseq, and are subtly buggy [1]. Solving those issues
> requires introducing additional complexity in the rseq implementation
> for each supported architecture.
>
> Considering that it complexifies the rseq ABI, I am proposing that we
> deprecate those flags. [2]
>
> So far there appears to be consensus from maintainers of user-space
> projects impacted by this feature that its removal would be a welcome
> simplification. [3]
>
> The deprecation approach proposed here is to issue WARN_ON_ONCE() when
> encountering those flags and kill the offending process with sigsegv.
> This should allow us to quickly identify whether anyone yells at us for
> removing this.

Hi Peter, would you consider pulling this patch through the tip tree ?

Thanks,

Mathieu

>
> Link:
> https://lore.kernel.org/lkml/[email protected]/
> [1]
> Link:
> https://lore.kernel.org/lkml/[email protected]/
> [2]
> Link:
> https://lore.kernel.org/lkml/[email protected]/ [3]
> Signed-off-by: Mathieu Desnoyers <[email protected]>
> ---
> kernel/rseq.c | 23 ++++++++---------------
> 1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index 97ac20b4f738..81d7dc80787b 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -18,8 +18,9 @@
> #define CREATE_TRACE_POINTS
> #include <trace/events/rseq.h>
>
> -#define RSEQ_CS_PREEMPT_MIGRATE_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE | \
> - RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT)
> +#define RSEQ_CS_NO_RESTART_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT | \
> + RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL | \
> + RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE)
>
> /*
> *
> @@ -175,23 +176,15 @@ static int rseq_need_restart(struct task_struct *t, u32
> cs_flags)
> u32 flags, event_mask;
> int ret;
>
> + if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS))
> + return -EINVAL;
> +
> /* Get thread flags. */
> ret = get_user(flags, &t->rseq->flags);
> if (ret)
> return ret;
>
> - /* Take critical section flags into account. */
> - flags |= cs_flags;
> -
> - /*
> - * Restart on signal can only be inhibited when restart on
> - * preempt and restart on migrate are inhibited too. Otherwise,
> - * a preempted signal handler could fail to restart the prior
> - * execution context on sigreturn.
> - */
> - if (unlikely((flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL) &&
> - (flags & RSEQ_CS_PREEMPT_MIGRATE_FLAGS) !=
> - RSEQ_CS_PREEMPT_MIGRATE_FLAGS))
> + if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
> return -EINVAL;
>
> /*
> @@ -203,7 +196,7 @@ static int rseq_need_restart(struct task_struct *t, u32
> cs_flags)
> t->rseq_event_mask = 0;
> preempt_enable();
>
> - return !!(event_mask & ~flags);
> + return !!event_mask;
> }
>
> static int clear_rseq_cs(struct task_struct *t)
> --
> 2.30.2

--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

2022-07-30 08:27:10

by tip-bot2 for Alexey Makhalov

Subject: [tip: sched/core] rseq: Deprecate RSEQ_CS_FLAG_NO_RESTART_ON_* flags

The following commit has been merged into the sched/core branch of tip:

Commit-ID: c040fc3cb4ce18e1be54df6294f6a4004d2664ec
Gitweb: https://git.kernel.org/tip/c040fc3cb4ce18e1be54df6294f6a4004d2664ec
Author: Mathieu Desnoyers <[email protected]>
AuthorDate: Wed, 22 Jun 2022 15:46:16 -04:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Sat, 30 Jul 2022 10:14:18 +02:00

rseq: Deprecate RSEQ_CS_FLAG_NO_RESTART_ON_* flags

The pretty much unused RSEQ_CS_FLAG_NO_RESTART_ON_* flags introduce
complexity in rseq, and are subtly buggy [1]. Solving those issues
requires introducing additional complexity in the rseq implementation
for each supported architecture.

Considering that it complexifies the rseq ABI, I am proposing that we
deprecate those flags. [2]

So far there appears to be consensus from maintainers of user-space
projects impacted by this feature that its removal would be a welcome
simplification. [3]

The deprecation approach proposed here is to issue WARN_ON_ONCE() when
encountering those flags and kill the offending process with sigsegv.
This should allow us to quickly identify whether anyone yells at us for
removing this.

Link: https://lore.kernel.org/lkml/[email protected]/ [1]
Link: https://lore.kernel.org/lkml/[email protected]/ [2]
Link: https://lore.kernel.org/lkml/[email protected]/ [3]
Signed-off-by: Mathieu Desnoyers <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]
---
kernel/rseq.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/kernel/rseq.c b/kernel/rseq.c
index 97ac20b..81d7dc8 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -18,8 +18,9 @@
#define CREATE_TRACE_POINTS
#include <trace/events/rseq.h>

-#define RSEQ_CS_PREEMPT_MIGRATE_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE | \
- RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT)
+#define RSEQ_CS_NO_RESTART_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT | \
+ RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL | \
+ RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE)

/*
*
@@ -175,23 +176,15 @@ static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
u32 flags, event_mask;
int ret;

+ if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS))
+ return -EINVAL;
+
/* Get thread flags. */
ret = get_user(flags, &t->rseq->flags);
if (ret)
return ret;

- /* Take critical section flags into account. */
- flags |= cs_flags;
-
- /*
- * Restart on signal can only be inhibited when restart on
- * preempt and restart on migrate are inhibited too. Otherwise,
- * a preempted signal handler could fail to restart the prior
- * execution context on sigreturn.
- */
- if (unlikely((flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL) &&
- (flags & RSEQ_CS_PREEMPT_MIGRATE_FLAGS) !=
- RSEQ_CS_PREEMPT_MIGRATE_FLAGS))
+ if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
return -EINVAL;

/*
@@ -203,7 +196,7 @@ static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
t->rseq_event_mask = 0;
preempt_enable();

- return !!(event_mask & ~flags);
+ return !!event_mask;
}

static int clear_rseq_cs(struct task_struct *t)

2022-08-01 14:22:33

by tip-bot2 for Alexey Makhalov

Subject: [tip: sched/core] rseq: Deprecate RSEQ_CS_FLAG_NO_RESTART_ON_* flags

The following commit has been merged into the sched/core branch of tip:

Commit-ID: 0190e4198e47fe99d002d72588f34fd62c9ab570
Gitweb: https://git.kernel.org/tip/0190e4198e47fe99d002d72588f34fd62c9ab570
Author: Mathieu Desnoyers <[email protected]>
AuthorDate: Wed, 22 Jun 2022 15:46:16 -04:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Mon, 01 Aug 2022 15:21:29 +02:00

rseq: Deprecate RSEQ_CS_FLAG_NO_RESTART_ON_* flags

The pretty much unused RSEQ_CS_FLAG_NO_RESTART_ON_* flags introduce
complexity in rseq, and are subtly buggy [1]. Solving those issues
requires introducing additional complexity in the rseq implementation
for each supported architecture.

Considering that it complexifies the rseq ABI, I am proposing that we
deprecate those flags. [2]

So far there appears to be consensus from maintainers of user-space
projects impacted by this feature that its removal would be a welcome
simplification. [3]

The deprecation approach proposed here is to issue WARN_ON_ONCE() when
encountering those flags and kill the offending process with sigsegv.
This should allow us to quickly identify whether anyone yells at us for
removing this.

Link: https://lore.kernel.org/lkml/[email protected]/ [1]
Link: https://lore.kernel.org/lkml/[email protected]/ [2]
Link: https://lore.kernel.org/lkml/[email protected]/ [3]
Signed-off-by: Mathieu Desnoyers <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]
---
kernel/rseq.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/kernel/rseq.c b/kernel/rseq.c
index 97ac20b..81d7dc8 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -18,8 +18,9 @@
#define CREATE_TRACE_POINTS
#include <trace/events/rseq.h>

-#define RSEQ_CS_PREEMPT_MIGRATE_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE | \
- RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT)
+#define RSEQ_CS_NO_RESTART_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT | \
+ RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL | \
+ RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE)

/*
*
@@ -175,23 +176,15 @@ static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
u32 flags, event_mask;
int ret;

+ if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS))
+ return -EINVAL;
+
/* Get thread flags. */
ret = get_user(flags, &t->rseq->flags);
if (ret)
return ret;

- /* Take critical section flags into account. */
- flags |= cs_flags;
-
- /*
- * Restart on signal can only be inhibited when restart on
- * preempt and restart on migrate are inhibited too. Otherwise,
- * a preempted signal handler could fail to restart the prior
- * execution context on sigreturn.
- */
- if (unlikely((flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL) &&
- (flags & RSEQ_CS_PREEMPT_MIGRATE_FLAGS) !=
- RSEQ_CS_PREEMPT_MIGRATE_FLAGS))
+ if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
return -EINVAL;

/*
@@ -203,7 +196,7 @@ static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
t->rseq_event_mask = 0;
preempt_enable();

- return !!(event_mask & ~flags);
+ return !!event_mask;
}

static int clear_rseq_cs(struct task_struct *t)