From: Peter Zijlstra <[email protected]>
Use arch_atomic_*() and READ_ONCE_NOCHECK() to ensure nothing untoward
creeps in and ruins things.
That is; this is the INT3 text poke handler, strictly limit the code
that runs in it, lest it inadvertenly hits yet another INT3.
Reported-by: Thomas Gleixner <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/kernel/alternative.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -960,9 +960,9 @@ static struct bp_patching_desc *bp_desc;
static __always_inline
struct bp_patching_desc *try_get_desc(struct bp_patching_desc **descp)
{
- struct bp_patching_desc *desc = READ_ONCE(*descp); /* rcu_dereference */
+ struct bp_patching_desc *desc = READ_ONCE_NOCHECK(*descp); /* rcu_dereference */
- if (!desc || !atomic_inc_not_zero(&desc->refs))
+ if (!desc || !arch_atomic_inc_not_zero(&desc->refs))
return NULL;
return desc;
@@ -971,7 +971,7 @@ struct bp_patching_desc *try_get_desc(st
static __always_inline void put_desc(struct bp_patching_desc *desc)
{
smp_mb__before_atomic();
- atomic_dec(&desc->refs);
+ arch_atomic_dec(&desc->refs);
}
static __always_inline void *text_poke_addr(struct text_poke_loc *tp)
On Tue, 05 May 2020 15:49:28 +0200
Thomas Gleixner <[email protected]> wrote:
> From: Peter Zijlstra <[email protected]>
>
> Use arch_atomic_*() and READ_ONCE_NOCHECK() to ensure nothing untoward
> creeps in and ruins things.
>
> That is; this is the INT3 text poke handler, strictly limit the code
> that runs in it, lest it inadvertenly hits yet another INT3.
>
> Reported-by: Thomas Gleixner <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
Looks good to me.
Reviewed-by: Masami Hiramatsu <[email protected]>
> ---
> arch/x86/kernel/alternative.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -960,9 +960,9 @@ static struct bp_patching_desc *bp_desc;
> static __always_inline
> struct bp_patching_desc *try_get_desc(struct bp_patching_desc **descp)
> {
> - struct bp_patching_desc *desc = READ_ONCE(*descp); /* rcu_dereference */
> + struct bp_patching_desc *desc = READ_ONCE_NOCHECK(*descp); /* rcu_dereference */
>
> - if (!desc || !atomic_inc_not_zero(&desc->refs))
> + if (!desc || !arch_atomic_inc_not_zero(&desc->refs))
> return NULL;
>
> return desc;
> @@ -971,7 +971,7 @@ struct bp_patching_desc *try_get_desc(st
> static __always_inline void put_desc(struct bp_patching_desc *desc)
> {
> smp_mb__before_atomic();
> - atomic_dec(&desc->refs);
> + arch_atomic_dec(&desc->refs);
> }
>
> static __always_inline void *text_poke_addr(struct text_poke_loc *tp)
>
--
Masami Hiramatsu <[email protected]>
On Tue, May 5, 2020 at 7:15 AM Thomas Gleixner <[email protected]> wrote:
>
> From: Peter Zijlstra <[email protected]>
>
> Use arch_atomic_*() and READ_ONCE_NOCHECK() to ensure nothing untoward
> creeps in and ruins things.
>
> That is; this is the INT3 text poke handler, strictly limit the code
> that runs in it, lest it inadvertenly hits yet another INT3.
Acked-by: Andy Lutomirski <[email protected]>
Does objtool catch this error?
On Wed, May 13, 2020 at 09:57:52PM -0700, Andy Lutomirski wrote:
> On Tue, May 5, 2020 at 7:15 AM Thomas Gleixner <[email protected]> wrote:
> >
> > From: Peter Zijlstra <[email protected]>
> >
> > Use arch_atomic_*() and READ_ONCE_NOCHECK() to ensure nothing untoward
> > creeps in and ruins things.
> >
> > That is; this is the INT3 text poke handler, strictly limit the code
> > that runs in it, lest it inadvertenly hits yet another INT3.
>
>
> Acked-by: Andy Lutomirski <[email protected]>
>
> Does objtool catch this error?
It does not. I'll put it on the (endless) todo list..
Peter Zijlstra <[email protected]> writes:
> On Wed, May 13, 2020 at 09:57:52PM -0700, Andy Lutomirski wrote:
>> On Tue, May 5, 2020 at 7:15 AM Thomas Gleixner <[email protected]> wrote:
>> >
>> > From: Peter Zijlstra <[email protected]>
>> >
>> > Use arch_atomic_*() and READ_ONCE_NOCHECK() to ensure nothing untoward
>> > creeps in and ruins things.
>> >
>> > That is; this is the INT3 text poke handler, strictly limit the code
>> > that runs in it, lest it inadvertenly hits yet another INT3.
>>
>>
>> Acked-by: Andy Lutomirski <[email protected]>
>>
>> Does objtool catch this error?
>
> It does not. I'll put it on the (endless) todo list..
Well, at least it detects when that code calls out into something which
is not in the non-instrumentable section.
As long as instrumentation respects the rules that this section is taboo,
this should not happen. Emphasis on *should*.
On Thu, May 14, 2020 at 02:51:32PM +0200, Thomas Gleixner wrote:
> Peter Zijlstra <[email protected]> writes:
> > On Wed, May 13, 2020 at 09:57:52PM -0700, Andy Lutomirski wrote:
> >> On Tue, May 5, 2020 at 7:15 AM Thomas Gleixner <[email protected]> wrote:
> >> >
> >> > From: Peter Zijlstra <[email protected]>
> >> >
> >> > Use arch_atomic_*() and READ_ONCE_NOCHECK() to ensure nothing untoward
> >> > creeps in and ruins things.
> >> >
> >> > That is; this is the INT3 text poke handler, strictly limit the code
> >> > that runs in it, lest it inadvertenly hits yet another INT3.
> >>
> >>
> >> Acked-by: Andy Lutomirski <[email protected]>
> >>
> >> Does objtool catch this error?
> >
> > It does not. I'll put it on the (endless) todo list..
>
> Well, at least it detects when that code calls out into something which
> is not in the non-instrumentable section.
True, but the more specific problem is that noinstr code can use
jump_label/static_call just fine.
So a more specific test is validating none of that happens in the INT3
handler before poke_int3_handler(). Which is what I think Andy was
after.
> On May 14, 2020, at 6:15 AM, Peter Zijlstra <[email protected]> wrote:
>
> On Thu, May 14, 2020 at 02:51:32PM +0200, Thomas Gleixner wrote:
>> Peter Zijlstra <[email protected]> writes:
>>> On Wed, May 13, 2020 at 09:57:52PM -0700, Andy Lutomirski wrote:
>>>>> On Tue, May 5, 2020 at 7:15 AM Thomas Gleixner <[email protected]> wrote:
>>>>>>
>>>>>> From: Peter Zijlstra <[email protected]>
>>>>>>
>>>>>> Use arch_atomic_*() and READ_ONCE_NOCHECK() to ensure nothing untoward
>>>>>> creeps in and ruins things.
>>>>>>
>>>>>> That is; this is the INT3 text poke handler, strictly limit the code
>>>>>> that runs in it, lest it inadvertenly hits yet another INT3.
>>>>>
>>>>>
>>>>> Acked-by: Andy Lutomirski <[email protected]>
>>>>>
>>>>> Does objtool catch this error?
>>>
>>> It does not. I'll put it on the (endless) todo list..
>>
>> Well, at least it detects when that code calls out into something which
>> is not in the non-instrumentable section.
>
> True, but the more specific problem is that noinstr code can use
> jump_label/static_call just fine.
>
> So a more specific test is validating none of that happens in the INT3
> handler before poke_int3_handler(). Which is what I think Andy was
> after.
Exactly. I admit that sleep-deprived Andy was actually thinking “tglx and/or PeterZ found this by inspection, and somewhere it escaped objtool’s notice,” which is sort of the same thing :)
Peter Zijlstra <[email protected]> writes:
> On Thu, May 14, 2020 at 02:51:32PM +0200, Thomas Gleixner wrote:
>> Peter Zijlstra <[email protected]> writes:
>> > On Wed, May 13, 2020 at 09:57:52PM -0700, Andy Lutomirski wrote:
>> >> On Tue, May 5, 2020 at 7:15 AM Thomas Gleixner <[email protected]> wrote:
>> >> >
>> >> > From: Peter Zijlstra <[email protected]>
>> >> >
>> >> > Use arch_atomic_*() and READ_ONCE_NOCHECK() to ensure nothing untoward
>> >> > creeps in and ruins things.
>> >> >
>> >> > That is; this is the INT3 text poke handler, strictly limit the code
>> >> > that runs in it, lest it inadvertenly hits yet another INT3.
>> >>
>> >>
>> >> Acked-by: Andy Lutomirski <[email protected]>
>> >>
>> >> Does objtool catch this error?
>> >
>> > It does not. I'll put it on the (endless) todo list..
>>
>> Well, at least it detects when that code calls out into something which
>> is not in the non-instrumentable section.
>
> True, but the more specific problem is that noinstr code can use
> jump_label/static_call just fine.
>
> So a more specific test is validating none of that happens in the INT3
> handler before poke_int3_handler(). Which is what I think Andy was
> after.
Indeed. Forgot about that one.
Hmm, alternatives and jumplabel patch locations in entry.text and
noinstr.text can be valid at least during early boot where we know that
we don't run those code pathes...
Thanks,
tglx
> On May 14, 2020, at 8:06 AM, Thomas Gleixner <[email protected]> wrote:
>
> Peter Zijlstra <[email protected]> writes:
>>> On Thu, May 14, 2020 at 02:51:32PM +0200, Thomas Gleixner wrote:
>>> Peter Zijlstra <[email protected]> writes:
>>>> On Wed, May 13, 2020 at 09:57:52PM -0700, Andy Lutomirski wrote:
>>>>> On Tue, May 5, 2020 at 7:15 AM Thomas Gleixner <[email protected]> wrote:
>>>>>>
>>>>>> From: Peter Zijlstra <[email protected]>
>>>>>>
>>>>>> Use arch_atomic_*() and READ_ONCE_NOCHECK() to ensure nothing untoward
>>>>>> creeps in and ruins things.
>>>>>>
>>>>>> That is; this is the INT3 text poke handler, strictly limit the code
>>>>>> that runs in it, lest it inadvertenly hits yet another INT3.
>>>>>
>>>>>
>>>>> Acked-by: Andy Lutomirski <[email protected]>
>>>>>
>>>>> Does objtool catch this error?
>>>>
>>>> It does not. I'll put it on the (endless) todo list..
>>>
>>> Well, at least it detects when that code calls out into something which
>>> is not in the non-instrumentable section.
>>
>> True, but the more specific problem is that noinstr code can use
>> jump_label/static_call just fine.
>>
>> So a more specific test is validating none of that happens in the INT3
>> handler before poke_int3_handler(). Which is what I think Andy was
>> after.
>
> Indeed. Forgot about that one.
>
> Hmm, alternatives and jumplabel patch locations in entry.text and
> noinstr.text can be valid at least during early boot where we know that
> we don't run those code pathes...
Alternatives should be valid regardless. Isn’t the world essentially stopped while we apply them?
>
> Thanks,
>
> tglx
On Thu, May 14, 2020 at 08:08:46AM -0700, Andy Lutomirski wrote:
> Alternatives should be valid regardless. Isn’t the world essentially stopped while we apply them?
Yes, we do that before we go SMP.
The following commit has been merged into the x86/entry branch of tip:
Commit-ID: a53a1d0435cdc2b66f41f75fa1cee31e8fe6d99e
Gitweb: https://git.kernel.org/tip/a53a1d0435cdc2b66f41f75fa1cee31e8fe6d99e
Author: Peter Zijlstra <[email protected]>
AuthorDate: Fri, 24 Jan 2020 22:08:45 +01:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Tue, 19 May 2020 16:04:05 +02:00
x86/int3: Avoid atomic instrumentation
Use arch_atomic_*() and __READ_ONCE() to ensure nothing untoward
creeps in and ruins things.
That is; this is the INT3 text poke handler, strictly limit the code
that runs in it, lest it inadvertenly hits yet another INT3.
Reported-by: Thomas Gleixner <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Masami Hiramatsu <[email protected]>
Reviewed-by: Alexandre Chartre <[email protected]>
Acked-by: Andy Lutomirski <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/kernel/alternative.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 1f4cb2c..686c8ac 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -960,9 +960,9 @@ static struct bp_patching_desc *bp_desc;
static __always_inline
struct bp_patching_desc *try_get_desc(struct bp_patching_desc **descp)
{
- struct bp_patching_desc *desc = READ_ONCE(*descp); /* rcu_dereference */
+ struct bp_patching_desc *desc = __READ_ONCE(*descp); /* rcu_dereference */
- if (!desc || !atomic_inc_not_zero(&desc->refs))
+ if (!desc || !arch_atomic_inc_not_zero(&desc->refs))
return NULL;
return desc;
@@ -971,7 +971,7 @@ struct bp_patching_desc *try_get_desc(struct bp_patching_desc **descp)
static __always_inline void put_desc(struct bp_patching_desc *desc)
{
smp_mb__before_atomic();
- atomic_dec(&desc->refs);
+ arch_atomic_dec(&desc->refs);
}
static __always_inline void *text_poke_addr(struct text_poke_loc *tp)