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 <[email protected]>
---
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
----- On Jun 22, 2022, at 3:46 PM, Mathieu Desnoyers [email protected] 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 <[email protected]>
> ---
> 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
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 <[email protected]>
AuthorDate: Wed, 22 Jun 2022 15:46:17 -04:00
Committer: Peter Zijlstra <[email protected]>
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 <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
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;
/*
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 <[email protected]>
AuthorDate: Wed, 22 Jun 2022 15:46:17 -04:00
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
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;
/*
* Mathieu Desnoyers <[email protected]> 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 <[email protected]>
> ---
> 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
----- On Aug 1, 2022, at 9:32 AM, Ingo Molnar [email protected] wrote:
> * Mathieu Desnoyers <[email protected]> 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 <[email protected]>
>> ---
>> 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
* Ingo Molnar:
> * Mathieu Desnoyers <[email protected]> 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 <[email protected]>
>> ---
>> 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
----- On Aug 1, 2022, at 10:25 AM, Florian Weimer [email protected] wrote:
> * Ingo Molnar:
>
>> * Mathieu Desnoyers <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
* Mathieu Desnoyers <[email protected]> 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