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

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

[Mathieu Desnoyers]

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/20220618182515.95831-1-minhquangbui99@gmail.com/ [1]
Link: https://lore.kernel.org/lkml/258546133.12151.1655739550814.JavaMail.zimbra@efficios.com/ [2]
Link: https://lore.kernel.org/lkml/87pmj1enjh.fsf@email.froward.int.ebiederm.org/ [3]
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 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

[PATCH 2/2] rseq: Kill process when unknown flags are encountered in ABI structures

[Mathieu Desnoyers]

rseq_abi()->flags and rseq_abi()->rseq_cs->flags 29 upper bits are
currently unused.

The current behavior when those bits are set is to ignore them. This is
not an ideal behavior, because when future features will start using
those flags, if user-space fails to correctly validate that the kernel
indeed supports those flags (e.g. with a new sys_rseq flags bit) before
using them, it may incorrectly assume that the kernel will handle those
flags way when in fact those will be silently ignored on older kernels.

Validating that unused flags bits are cleared will allow a smoother
transition when those flags will start to be used by allowing
applications to fail early, and obviously, when they attempt to use the
new flags on an older kernel that does not support them.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 kernel/rseq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/rseq.c b/kernel/rseq.c
index 81d7dc80787b..bda8175f8f99 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -176,7 +176,7 @@ 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))
+	if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS) || cs_flags)
 		return -EINVAL;
 
 	/* Get thread flags. */
@@ -184,7 +184,7 @@ static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
 	if (ret)
 		return ret;
 
-	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
+	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS) || flags)
 		return -EINVAL;
 
 	/*
-- 
2.30.2

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

[Mathieu Desnoyers]

----- On Jun 22, 2022, at 3:46 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com 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/20220618182515.95831-1-minhquangbui99@gmail.com/
> [1]
> Link:
> https://lore.kernel.org/lkml/258546133.12151.1655739550814.JavaMail.zimbra@efficios.com/
> [2]
> Link:
> https://lore.kernel.org/lkml/87pmj1enjh.fsf@email.froward.int.ebiederm.org/ [3]
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
> 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

Re: [PATCH 2/2] rseq: Kill process when unknown flags are encountered in ABI structures

[Mathieu Desnoyers]

----- On Jun 22, 2022, at 3:46 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> rseq_abi()->flags and rseq_abi()->rseq_cs->flags 29 upper bits are
> currently unused.
> 
> The current behavior when those bits are set is to ignore them. This is
> not an ideal behavior, because when future features will start using
> those flags, if user-space fails to correctly validate that the kernel
> indeed supports those flags (e.g. with a new sys_rseq flags bit) before
> using them, it may incorrectly assume that the kernel will handle those
> flags way when in fact those will be silently ignored on older kernels.
> 
> Validating that unused flags bits are cleared will allow a smoother
> transition when those flags will start to be used by allowing
> applications to fail early, and obviously, when they attempt to use the
> new flags on an older kernel that does not support them.

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

Thanks,

Mathieu

> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
> kernel/rseq.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index 81d7dc80787b..bda8175f8f99 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -176,7 +176,7 @@ 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))
> +	if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS) || cs_flags)
> 		return -EINVAL;
> 
> 	/* Get thread flags. */
> @@ -184,7 +184,7 @@ static int rseq_need_restart(struct task_struct *t, u32
> cs_flags)
> 	if (ret)
> 		return ret;
> 
> -	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
> +	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS) || flags)
> 		return -EINVAL;
> 
> 	/*
> --
> 2.30.2

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

[tip: sched/core] rseq: Kill process when unknown flags are encountered in ABI structures

[tip-bot2 for Mathieu Desnoyers]

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

Commit-ID:     8da3d9b8590bc178752d4b72938745e9a6c4c416
Gitweb:        https://git.kernel.org/tip/8da3d9b8590bc178752d4b72938745e9a6c4c416
Author:        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
AuthorDate:    Wed, 22 Jun 2022 15:46:17 -04:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Sat, 30 Jul 2022 10:14:18 +02:00

rseq: Kill process when unknown flags are encountered in ABI structures

rseq_abi()->flags and rseq_abi()->rseq_cs->flags 29 upper bits are
currently unused.

The current behavior when those bits are set is to ignore them. This is
not an ideal behavior, because when future features will start using
those flags, if user-space fails to correctly validate that the kernel
indeed supports those flags (e.g. with a new sys_rseq flags bit) before
using them, it may incorrectly assume that the kernel will handle those
flags way when in fact those will be silently ignored on older kernels.

Validating that unused flags bits are cleared will allow a smoother
transition when those flags will start to be used by allowing
applications to fail early, and obviously, when they attempt to use the
new flags on an older kernel that does not support them.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20220622194617.1155957-2-mathieu.desnoyers@efficios.com
---
 kernel/rseq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/rseq.c b/kernel/rseq.c
index 81d7dc8..bda8175 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -176,7 +176,7 @@ 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))
+	if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS) || cs_flags)
 		return -EINVAL;
 
 	/* Get thread flags. */
@@ -184,7 +184,7 @@ static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
 	if (ret)
 		return ret;
 
-	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
+	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS) || flags)
 		return -EINVAL;
 
 	/*

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

[tip-bot2 for Mathieu Desnoyers]

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 <mathieu.desnoyers@efficios.com>
AuthorDate:    Wed, 22 Jun 2022 15:46:16 -04:00
Committer:     Peter Zijlstra <peterz@infradead.org>
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/20220618182515.95831-1-minhquangbui99@gmail.com/ [1]
Link: https://lore.kernel.org/lkml/258546133.12151.1655739550814.JavaMail.zimbra@efficios.com/ [2]
Link: https://lore.kernel.org/lkml/87pmj1enjh.fsf@email.froward.int.ebiederm.org/ [3]
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/lkml/20220622194617.1155957-1-mathieu.desnoyers@efficios.com
---
 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)

[tip: sched/core] rseq: Kill process when unknown flags are encountered in ABI structures

[tip-bot2 for Mathieu Desnoyers]

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

Commit-ID:     c17a6ff9321355487d7d5ccaa7d406a0ea06b6c4
Gitweb:        https://git.kernel.org/tip/c17a6ff9321355487d7d5ccaa7d406a0ea06b6c4
Author:        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
AuthorDate:    Wed, 22 Jun 2022 15:46:17 -04:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 01 Aug 2022 15:21:42 +02:00

rseq: Kill process when unknown flags are encountered in ABI structures

rseq_abi()->flags and rseq_abi()->rseq_cs->flags 29 upper bits are
currently unused.

The current behavior when those bits are set is to ignore them. This is
not an ideal behavior, because when future features will start using
those flags, if user-space fails to correctly validate that the kernel
indeed supports those flags (e.g. with a new sys_rseq flags bit) before
using them, it may incorrectly assume that the kernel will handle those
flags way when in fact those will be silently ignored on older kernels.

Validating that unused flags bits are cleared will allow a smoother
transition when those flags will start to be used by allowing
applications to fail early, and obviously, when they attempt to use the
new flags on an older kernel that does not support them.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lkml.kernel.org/r/20220622194617.1155957-2-mathieu.desnoyers@efficios.com
---
 kernel/rseq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/rseq.c b/kernel/rseq.c
index 81d7dc8..bda8175 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -176,7 +176,7 @@ 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))
+	if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS) || cs_flags)
 		return -EINVAL;
 
 	/* Get thread flags. */
@@ -184,7 +184,7 @@ static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
 	if (ret)
 		return ret;
 
-	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
+	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS) || flags)
 		return -EINVAL;
 
 	/*

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

[tip-bot2 for Mathieu Desnoyers]

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 <mathieu.desnoyers@efficios.com>
AuthorDate:    Wed, 22 Jun 2022 15:46:16 -04:00
Committer:     Ingo Molnar <mingo@kernel.org>
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/20220618182515.95831-1-minhquangbui99@gmail.com/ [1]
Link: https://lore.kernel.org/lkml/258546133.12151.1655739550814.JavaMail.zimbra@efficios.com/ [2]
Link: https://lore.kernel.org/lkml/87pmj1enjh.fsf@email.froward.int.ebiederm.org/ [3]
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/lkml/20220622194617.1155957-1-mathieu.desnoyers@efficios.com
---
 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)

Re: [PATCH 2/2] rseq: Kill process when unknown flags are encountered in ABI structures

[Ingo Molnar]

* Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> rseq_abi()->flags and rseq_abi()->rseq_cs->flags 29 upper bits are
> currently unused.
> 
> The current behavior when those bits are set is to ignore them. This is
> not an ideal behavior, because when future features will start using
> those flags, if user-space fails to correctly validate that the kernel
> indeed supports those flags (e.g. with a new sys_rseq flags bit) before
> using them, it may incorrectly assume that the kernel will handle those
> flags way when in fact those will be silently ignored on older kernels.
> 
> Validating that unused flags bits are cleared will allow a smoother
> transition when those flags will start to be used by allowing
> applications to fail early, and obviously, when they attempt to use the
> new flags on an older kernel that does not support them.
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
>  kernel/rseq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index 81d7dc80787b..bda8175f8f99 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -176,7 +176,7 @@ 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))
> +	if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS) || cs_flags)
>  		return -EINVAL;
>  
>  	/* Get thread flags. */
> @@ -184,7 +184,7 @@ static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
>  	if (ret)
>  		return ret;
>  
> -	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
> +	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS) || flags)
>  		return -EINVAL;

Just to make it clear: no existing libraries/tooling out there have learned 
to rely on the old ABI that ignored unset flags, right? Only then is this 
patch ABI-safe.

Thanks,

	Ingo

Re: [PATCH 2/2] rseq: Kill process when unknown flags are encountered in ABI structures

[Florian Weimer]

* Ingo Molnar:

> * Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
>> rseq_abi()->flags and rseq_abi()->rseq_cs->flags 29 upper bits are
>> currently unused.
>> 
>> The current behavior when those bits are set is to ignore them. This is
>> not an ideal behavior, because when future features will start using
>> those flags, if user-space fails to correctly validate that the kernel
>> indeed supports those flags (e.g. with a new sys_rseq flags bit) before
>> using them, it may incorrectly assume that the kernel will handle those
>> flags way when in fact those will be silently ignored on older kernels.
>> 
>> Validating that unused flags bits are cleared will allow a smoother
>> transition when those flags will start to be used by allowing
>> applications to fail early, and obviously, when they attempt to use the
>> new flags on an older kernel that does not support them.
>> 
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> ---
>>  kernel/rseq.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/kernel/rseq.c b/kernel/rseq.c
>> index 81d7dc80787b..bda8175f8f99 100644
>> --- a/kernel/rseq.c
>> +++ b/kernel/rseq.c
>> @@ -176,7 +176,7 @@ 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))
>> +	if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS) || cs_flags)
>>  		return -EINVAL;
>>  
>>  	/* Get thread flags. */
>> @@ -184,7 +184,7 @@ static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
>>  	if (ret)
>>  		return ret;
>>  
>> -	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
>> +	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS) || flags)
>>  		return -EINVAL;
>
> Just to make it clear: no existing libraries/tooling out there have learned 
> to rely on the old ABI that ignored unset flags, right? Only then is this 
> patch ABI-safe.

I believe glibc initializes the flag fields to zero before calling the
rseq system call.  (I don't know if the rseq system call does its own
initialization; maybe it should if it doesn't do so already.)

Thanks,
Florian

Re: [PATCH 2/2] rseq: Kill process when unknown flags are encountered in ABI structures

[Mathieu Desnoyers]

----- On Aug 1, 2022, at 9:32 AM, Ingo Molnar mingo@kernel.org wrote:

> * Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
>> rseq_abi()->flags and rseq_abi()->rseq_cs->flags 29 upper bits are
>> currently unused.
>> 
>> The current behavior when those bits are set is to ignore them. This is
>> not an ideal behavior, because when future features will start using
>> those flags, if user-space fails to correctly validate that the kernel
>> indeed supports those flags (e.g. with a new sys_rseq flags bit) before
>> using them, it may incorrectly assume that the kernel will handle those
>> flags way when in fact those will be silently ignored on older kernels.
>> 
>> Validating that unused flags bits are cleared will allow a smoother
>> transition when those flags will start to be used by allowing
>> applications to fail early, and obviously, when they attempt to use the
>> new flags on an older kernel that does not support them.
>> 
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> ---
>>  kernel/rseq.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/kernel/rseq.c b/kernel/rseq.c
>> index 81d7dc80787b..bda8175f8f99 100644
>> --- a/kernel/rseq.c
>> +++ b/kernel/rseq.c
>> @@ -176,7 +176,7 @@ 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))
>> +	if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS) || cs_flags)
>>  		return -EINVAL;
>>  
>>  	/* Get thread flags. */
>> @@ -184,7 +184,7 @@ static int rseq_need_restart(struct task_struct *t, u32
>> cs_flags)
>>  	if (ret)
>>  		return ret;
>>  
>> -	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
>> +	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS) || flags)
>>  		return -EINVAL;
> 
> Just to make it clear: no existing libraries/tooling out there have learned
> to rely on the old ABI that ignored unset flags, right? Only then is this
> patch ABI-safe.

The projects I know about that use rseq at the moment don't rely on the old ABI
ignoring unset flags:

- glibc initialize the rseq_abi()->flags to 0 and do not use rseq_abi()->rseq_cs->flags yet.
- tcmalloc initialize rseq_abi()->flags and rseq_abi()->rseq_cs->flags to 0.
- librseq (still only a master branch, no officially released public API yet) initialize
  rseq_abi()->flags and rseq_abi()->rseq_cs->cs_flags to 0.
- the Linux kernel selftests initialize rseq_abi()->flags and rseq_abi()->rseq_cs->cs_flags
  to 0.
- AFAIK DynamoRIO does not rely on the kernel ignoring unset flags bits.
- AFAIK CRIU does not rely on the kernel ignoring unset flags bits.

If anyone else rely on rseq ignoring those unset flags, please yell now.

Thanks,

Mathieu

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

Re: [PATCH 2/2] rseq: Kill process when unknown flags are encountered in ABI structures

[Mathieu Desnoyers]

----- On Aug 1, 2022, at 10:25 AM, Florian Weimer fweimer@redhat.com wrote:

> * Ingo Molnar:
> 
>> * Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>>
>>> rseq_abi()->flags and rseq_abi()->rseq_cs->flags 29 upper bits are
>>> currently unused.
>>> 
>>> The current behavior when those bits are set is to ignore them. This is
>>> not an ideal behavior, because when future features will start using
>>> those flags, if user-space fails to correctly validate that the kernel
>>> indeed supports those flags (e.g. with a new sys_rseq flags bit) before
>>> using them, it may incorrectly assume that the kernel will handle those
>>> flags way when in fact those will be silently ignored on older kernels.
>>> 
>>> Validating that unused flags bits are cleared will allow a smoother
>>> transition when those flags will start to be used by allowing
>>> applications to fail early, and obviously, when they attempt to use the
>>> new flags on an older kernel that does not support them.
>>> 
>>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>>> ---
>>>  kernel/rseq.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/kernel/rseq.c b/kernel/rseq.c
>>> index 81d7dc80787b..bda8175f8f99 100644
>>> --- a/kernel/rseq.c
>>> +++ b/kernel/rseq.c
>>> @@ -176,7 +176,7 @@ 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))
>>> +	if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS) || cs_flags)
>>>  		return -EINVAL;
>>>  
>>>  	/* Get thread flags. */
>>> @@ -184,7 +184,7 @@ static int rseq_need_restart(struct task_struct *t, u32
>>> cs_flags)
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> -	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
>>> +	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS) || flags)
>>>  		return -EINVAL;
>>
>> Just to make it clear: no existing libraries/tooling out there have learned
>> to rely on the old ABI that ignored unset flags, right? Only then is this
>> patch ABI-safe.
> 
> I believe glibc initializes the flag fields to zero before calling the
> rseq system call.  (I don't know if the rseq system call does its own
> initialization; maybe it should if it doesn't do so already.)

Initialization and following updates of rseq_abi()->flags and
rseq_abi()->rseq_cs->flags is done by user-space, so the rseq
system call does not initialize any of those fields.

Indeed glibc initialize the rseq_abi()->flags to 0, and does not
use rseq_abi()->rseq_cs->flags as of now.

Thanks,

Mathieu

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

Re: [PATCH 2/2] rseq: Kill process when unknown flags are encountered in ABI structures

[Ingo Molnar]

* Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> The projects I know about that use rseq at the moment don't rely on the 
> old ABI ignoring unset flags:
> 
> - glibc initialize the rseq_abi()->flags to 0 and do not use rseq_abi()->rseq_cs->flags yet.
> - tcmalloc initialize rseq_abi()->flags and rseq_abi()->rseq_cs->flags to 0.
> - librseq (still only a master branch, no officially released public API yet) initialize
>   rseq_abi()->flags and rseq_abi()->rseq_cs->cs_flags to 0.
> - the Linux kernel selftests initialize rseq_abi()->flags and rseq_abi()->rseq_cs->cs_flags
>   to 0.
> - AFAIK DynamoRIO does not rely on the kernel ignoring unset flags bits.
> - AFAIK CRIU does not rely on the kernel ignoring unset flags bits.

Thanks - that's exhaustive enough.

> If anyone else rely on rseq ignoring those unset flags, please yell now.

Well, people are unlikely to see random lkml mails - but if gets reported 
as a regression then we need to revert. But I don't expect it to happen.

Thanks,

	Ingo