LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline
@ 2018-10-30  6:55 Zhenzhong Duan
  2018-10-30  8:36 ` Peter Zijlstra
  0 siblings, 1 reply; 8+ messages in thread
From: Zhenzhong Duan @ 2018-10-30  6:55 UTC (permalink / raw)
  To: Linux-Kernel
  Cc: mingo, konrad.wilk, dwmw, tglx, Srinivas REDDY Eeda, bp, peterz, hpa

Since CONFIG_RETPOLINE hard depends on compiler support now, so
replacing indirect-jump check with the range check is safe in that case.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: David Woodhouse <dwmw@amazon.co.uk>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/kernel/kprobes/opt.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 40b16b2..1136b29 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -203,6 +203,7 @@ static int copy_optimized_instructions(u8 *dest, u8 *src, u8 *real)
 	return len;
 }
 
+#ifndef CONFIG_RETPOLINE
 /* Check whether insn is indirect jump */
 static int __insn_is_indirect_jump(struct insn *insn)
 {
@@ -210,6 +211,7 @@ static int __insn_is_indirect_jump(struct insn *insn)
 		(X86_MODRM_REG(insn->modrm.value) & 6) == 4) || /* Jump */
 		insn->opcode.bytes[0] == 0xea);	/* Segment based jump */
 }
+#endif
 
 /* Check whether insn jumps into specified address range */
 static int insn_jump_into_range(struct insn *insn, unsigned long start, int len)
@@ -240,20 +242,16 @@ static int insn_jump_into_range(struct insn *insn, unsigned long start, int len)
 
 static int insn_is_indirect_jump(struct insn *insn)
 {
-	int ret = __insn_is_indirect_jump(insn);
+	int ret;
 
 #ifdef CONFIG_RETPOLINE
-	/*
-	 * Jump to x86_indirect_thunk_* is treated as an indirect jump.
-	 * Note that even with CONFIG_RETPOLINE=y, the kernel compiled with
-	 * older gcc may use indirect jump. So we add this check instead of
-	 * replace indirect-jump check.
-	 */
-	if (!ret)
+	/* Jump to x86_indirect_thunk_* is treated as an indirect jump. */
 		ret = insn_jump_into_range(insn,
 				(unsigned long)__indirect_thunk_start,
 				(unsigned long)__indirect_thunk_end -
 				(unsigned long)__indirect_thunk_start);
+#else
+		ret = __insn_is_indirect_jump(insn);
 #endif
 	return ret;
 }
-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline
  2018-10-30  6:55 [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline Zhenzhong Duan
@ 2018-10-30  8:36 ` Peter Zijlstra
  2018-10-31  6:01   ` Zhenzhong Duan
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2018-10-30  8:36 UTC (permalink / raw)
  To: Zhenzhong Duan
  Cc: Linux-Kernel, mingo, konrad.wilk, dwmw, tglx,
	Srinivas REDDY Eeda, bp, hpa

On Mon, Oct 29, 2018 at 11:55:06PM -0700, Zhenzhong Duan wrote:
> Since CONFIG_RETPOLINE hard depends on compiler support now, so
> replacing indirect-jump check with the range check is safe in that case.

Can we put kprobes on module init text before we run alternatives on it?

> @@ -240,20 +242,16 @@ static int insn_jump_into_range(struct insn *insn, unsigned long start, int len)
>  
>  static int insn_is_indirect_jump(struct insn *insn)
>  {
> -	int ret = __insn_is_indirect_jump(insn);
> +	int ret;
>  
>  #ifdef CONFIG_RETPOLINE
> -	/*
> -	 * Jump to x86_indirect_thunk_* is treated as an indirect jump.
> -	 * Note that even with CONFIG_RETPOLINE=y, the kernel compiled with
> -	 * older gcc may use indirect jump. So we add this check instead of
> -	 * replace indirect-jump check.
> -	 */
> -	if (!ret)
> +	/* Jump to x86_indirect_thunk_* is treated as an indirect jump. */
>  		ret = insn_jump_into_range(insn,
>  				(unsigned long)__indirect_thunk_start,
>  				(unsigned long)__indirect_thunk_end -
>  				(unsigned long)__indirect_thunk_start);
> +#else
> +		ret = __insn_is_indirect_jump(insn);
>  #endif
>  	return ret;
>  }

The resulting code is indented wrong.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline
  2018-10-30  8:36 ` Peter Zijlstra
@ 2018-10-31  6:01   ` Zhenzhong Duan
  2018-10-31 13:53     ` Peter Zijlstra
  0 siblings, 1 reply; 8+ messages in thread
From: Zhenzhong Duan @ 2018-10-31  6:01 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Linux-Kernel, mingo, konrad.wilk, dwmw, tglx,
	Srinivas REDDY Eeda, bp, hpa

On 2018/10/30 16:36, Peter Zijlstra wrote:
> On Mon, Oct 29, 2018 at 11:55:06PM -0700, Zhenzhong Duan wrote:
>> Since CONFIG_RETPOLINE hard depends on compiler support now, so
>> replacing indirect-jump check with the range check is safe in that case.
> 
> Can we put kprobes on module init text before we run alternatives on it?

Forgive me I doesn't understand your question. Do you mean this patch 
impact kprobes on module init text?

> 
>> @@ -240,20 +242,16 @@ static int insn_jump_into_range(struct insn *insn, unsigned long start, int len)
>>   
>>   static int insn_is_indirect_jump(struct insn *insn)
>>   {
>> -	int ret = __insn_is_indirect_jump(insn);
>> +	int ret;
>>   
>>   #ifdef CONFIG_RETPOLINE
>> -	/*
>> -	 * Jump to x86_indirect_thunk_* is treated as an indirect jump.
>> -	 * Note that even with CONFIG_RETPOLINE=y, the kernel compiled with
>> -	 * older gcc may use indirect jump. So we add this check instead of
>> -	 * replace indirect-jump check.
>> -	 */
>> -	if (!ret)
>> +	/* Jump to x86_indirect_thunk_* is treated as an indirect jump. */
>>   		ret = insn_jump_into_range(insn,
>>   				(unsigned long)__indirect_thunk_start,
>>   				(unsigned long)__indirect_thunk_end -
>>   				(unsigned long)__indirect_thunk_start);
>> +#else
>> +		ret = __insn_is_indirect_jump(insn);
>>   #endif
>>   	return ret;
>>   }
> 
> The resulting code is indented wrong.
> 

Oh, yes. Thanks for point out.

Zhenzhong

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline
  2018-10-31  6:01   ` Zhenzhong Duan
@ 2018-10-31 13:53     ` Peter Zijlstra
  2018-10-31 14:00       ` Peter Zijlstra
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2018-10-31 13:53 UTC (permalink / raw)
  To: Zhenzhong Duan
  Cc: Linux-Kernel, mingo, konrad.wilk, dwmw, tglx,
	Srinivas REDDY Eeda, bp, hpa

On Wed, Oct 31, 2018 at 02:01:20PM +0800, Zhenzhong Duan wrote:
> On 2018/10/30 16:36, Peter Zijlstra wrote:
> > On Mon, Oct 29, 2018 at 11:55:06PM -0700, Zhenzhong Duan wrote:
> > > Since CONFIG_RETPOLINE hard depends on compiler support now, so
> > > replacing indirect-jump check with the range check is safe in that case.
> > 
> > Can we put kprobes on module init text before we run alternatives on it?
> 
> Forgive me I doesn't understand your question. Do you mean this patch impact
> kprobes on module init text?

In that case we would still see the indirect paravirt calls for example,
and we'd still need that cascade you took out.

Now, I'm not at all sure we're able to use kprobes at those times, so it
might be a non-issue.

> > > @@ -240,20 +242,16 @@ static int insn_jump_into_range(struct insn *insn, unsigned long start, int len)
> > >   static int insn_is_indirect_jump(struct insn *insn)
> > >   {
> > > -	int ret = __insn_is_indirect_jump(insn);
> > > +	int ret;
> > >   #ifdef CONFIG_RETPOLINE
> > > -	/*
> > > -	 * Jump to x86_indirect_thunk_* is treated as an indirect jump.
> > > -	 * Note that even with CONFIG_RETPOLINE=y, the kernel compiled with
> > > -	 * older gcc may use indirect jump. So we add this check instead of
> > > -	 * replace indirect-jump check.
> > > -	 */
> > > -	if (!ret)
> > > +	/* Jump to x86_indirect_thunk_* is treated as an indirect jump. */
> > >   		ret = insn_jump_into_range(insn,
> > >   				(unsigned long)__indirect_thunk_start,
> > >   				(unsigned long)__indirect_thunk_end -
> > >   				(unsigned long)__indirect_thunk_start);
> > > +#else
> > > +		ret = __insn_is_indirect_jump(insn);
> > >   #endif
> > >   	return ret;
> > >   }
> > 
> > The resulting code is indented wrong.
> > 
> 
> Oh, yes. Thanks for point out.
> 
> Zhenzhong

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline
  2018-10-31 13:53     ` Peter Zijlstra
@ 2018-10-31 14:00       ` Peter Zijlstra
  2018-11-01  2:02         ` Zhenzhong Duan
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2018-10-31 14:00 UTC (permalink / raw)
  To: Zhenzhong Duan
  Cc: Linux-Kernel, mingo, konrad.wilk, dwmw, tglx,
	Srinivas REDDY Eeda, bp, hpa

On Wed, Oct 31, 2018 at 02:53:20PM +0100, Peter Zijlstra wrote:
> On Wed, Oct 31, 2018 at 02:01:20PM +0800, Zhenzhong Duan wrote:
> > On 2018/10/30 16:36, Peter Zijlstra wrote:
> > > On Mon, Oct 29, 2018 at 11:55:06PM -0700, Zhenzhong Duan wrote:
> > > > Since CONFIG_RETPOLINE hard depends on compiler support now, so
> > > > replacing indirect-jump check with the range check is safe in that case.
> > > 
> > > Can we put kprobes on module init text before we run alternatives on it?
> > 
> > Forgive me I doesn't understand your question. Do you mean this patch impact
> > kprobes on module init text?
> 
> In that case we would still see the indirect paravirt calls for example,
> and we'd still need that cascade you took out.
> 
> Now, I'm not at all sure we're able to use kprobes at those times, so it
> might be a non-issue.

Hmm, what about the case where we have RETPOLINE runtime disabled? Then
the CALL_NOSPEC alternative patches in an indirect call again, and the
retpolines are gone.

Does that not need the __insn_is_indirect_jump() thing?

> > > > @@ -240,20 +242,16 @@ static int insn_jump_into_range(struct insn *insn, unsigned long start, int len)
> > > >   static int insn_is_indirect_jump(struct insn *insn)
> > > >   {
> > > > -	int ret = __insn_is_indirect_jump(insn);
> > > > +	int ret;
> > > >   #ifdef CONFIG_RETPOLINE
> > > > -	/*
> > > > -	 * Jump to x86_indirect_thunk_* is treated as an indirect jump.
> > > > -	 * Note that even with CONFIG_RETPOLINE=y, the kernel compiled with
> > > > -	 * older gcc may use indirect jump. So we add this check instead of
> > > > -	 * replace indirect-jump check.
> > > > -	 */
> > > > -	if (!ret)
> > > > +	/* Jump to x86_indirect_thunk_* is treated as an indirect jump. */
> > > >   		ret = insn_jump_into_range(insn,
> > > >   				(unsigned long)__indirect_thunk_start,
> > > >   				(unsigned long)__indirect_thunk_end -
> > > >   				(unsigned long)__indirect_thunk_start);
> > > > +#else
> > > > +		ret = __insn_is_indirect_jump(insn);
> > > >   #endif
> > > >   	return ret;
> > > >   }
> > > 
> > > The resulting code is indented wrong.
> > > 
> > 
> > Oh, yes. Thanks for point out.
> > 
> > Zhenzhong

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline
  2018-10-31 14:00       ` Peter Zijlstra
@ 2018-11-01  2:02         ` Zhenzhong Duan
  2018-11-01  8:56           ` Peter Zijlstra
  0 siblings, 1 reply; 8+ messages in thread
From: Zhenzhong Duan @ 2018-11-01  2:02 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Linux-Kernel, mingo, konrad.wilk, dwmw, tglx,
	Srinivas REDDY Eeda, bp, hpa

On 2018/10/31 22:00, Peter Zijlstra wrote:
> On Wed, Oct 31, 2018 at 02:53:20PM +0100, Peter Zijlstra wrote:
>> On Wed, Oct 31, 2018 at 02:01:20PM +0800, Zhenzhong Duan wrote:
>>> On 2018/10/30 16:36, Peter Zijlstra wrote:
>>>> On Mon, Oct 29, 2018 at 11:55:06PM -0700, Zhenzhong Duan wrote:
>>>>> Since CONFIG_RETPOLINE hard depends on compiler support now, so
>>>>> replacing indirect-jump check with the range check is safe in that case.
>>>>
>>>> Can we put kprobes on module init text before we run alternatives on it?
>>>
>>> Forgive me I doesn't understand your question. Do you mean this patch impact
>>> kprobes on module init text?
>>
>> In that case we would still see the indirect paravirt calls for example,
>> and we'd still need that cascade you took out.

Understood.
In another case when loading a non-retpoline module, we suffer the same.
>>
>> Now, I'm not at all sure we're able to use kprobes at those times, so it
>> might be a non-issue.

Not sure, but if it's possible then alternative patching may cover the 
kprobes, it looks like a bug.
> 
> Hmm, what about the case where we have RETPOLINE runtime disabled? Then
> the CALL_NOSPEC alternative patches in an indirect call again, and the
> retpolines are gone.

Is RETPOLINE runtime toggle supported in upstream? I don't see such code.
> 
> Does that not need the __insn_is_indirect_jump() thing?

Yes it's needed if RETPOLINE runtime disabled.

Based on all above reasons, I'd like to drop this patch.

Thanks
Zhenzhong

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline
  2018-11-01  2:02         ` Zhenzhong Duan
@ 2018-11-01  8:56           ` Peter Zijlstra
  2018-11-01  9:21             ` Zhenzhong Duan
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2018-11-01  8:56 UTC (permalink / raw)
  To: Zhenzhong Duan
  Cc: Linux-Kernel, mingo, konrad.wilk, dwmw, tglx,
	Srinivas REDDY Eeda, bp, hpa

On Thu, Nov 01, 2018 at 10:02:14AM +0800, Zhenzhong Duan wrote:
> > Hmm, what about the case where we have RETPOLINE runtime disabled? Then
> > the CALL_NOSPEC alternative patches in an indirect call again, and the
> > retpolines are gone.
> 
> Is RETPOLINE runtime toggle supported in upstream? I don't see such code.

arch/x86/kernel/cpu/bugs.c look for the "nospectre_v2" and related
options. That will avoid X86_FEATURE_RETPOLINE from being set, and thus
the JMP_NOSPEC and CALL_NOSPEC alternatives will not patch out the
indirect jump / call.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline
  2018-11-01  8:56           ` Peter Zijlstra
@ 2018-11-01  9:21             ` Zhenzhong Duan
  0 siblings, 0 replies; 8+ messages in thread
From: Zhenzhong Duan @ 2018-11-01  9:21 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Linux-Kernel, mingo, konrad.wilk, dwmw, tglx,
	Srinivas REDDY Eeda, bp, hpa

On 2018/11/1 16:56, Peter Zijlstra wrote:
> On Thu, Nov 01, 2018 at 10:02:14AM +0800, Zhenzhong Duan wrote:
>>> Hmm, what about the case where we have RETPOLINE runtime disabled? Then
>>> the CALL_NOSPEC alternative patches in an indirect call again, and the
>>> retpolines are gone.
>>
>> Is RETPOLINE runtime toggle supported in upstream? I don't see such code.
> 
> arch/x86/kernel/cpu/bugs.c look for the "nospectre_v2" and related
> options. That will avoid X86_FEATURE_RETPOLINE from being set, and thus
> the JMP_NOSPEC and CALL_NOSPEC alternatives will not patch out the
> indirect jump / call.
> 
Yes, in this case there are also indirect branches. I'll drop 3rd patch 
in v2.

Thanks
Zhenzhong

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-11-01  9:21 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-30  6:55 [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline Zhenzhong Duan
2018-10-30  8:36 ` Peter Zijlstra
2018-10-31  6:01   ` Zhenzhong Duan
2018-10-31 13:53     ` Peter Zijlstra
2018-10-31 14:00       ` Peter Zijlstra
2018-11-01  2:02         ` Zhenzhong Duan
2018-11-01  8:56           ` Peter Zijlstra
2018-11-01  9:21             ` Zhenzhong Duan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).