[v1,06/19] perf/x86/intel/ds: Check insn_get_length() retval
diff mbox series

Message ID 20201223174233.28638-7-bp@alien8.de
State New, archived
Headers show
Series
  • x86/insn: Add an insn_decode() API
Related show

Commit Message

Borislav Petkov Dec. 23, 2020, 5:42 p.m. UTC
From: Borislav Petkov <bp@suse.de>

intel_pmu_pebs_fixup_ip() needs only the insn length so use the
appropriate helper instead of a full decode. A full decode differs only
in running insn_complete() on the decoded insn but that is not needed
here.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/events/intel/ds.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Peter Zijlstra Jan. 4, 2021, 1:19 p.m. UTC | #1
On Wed, Dec 23, 2020 at 06:42:20PM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> intel_pmu_pebs_fixup_ip() needs only the insn length so use the
> appropriate helper instead of a full decode. A full decode differs only
> in running insn_complete() on the decoded insn but that is not needed
> here.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/events/intel/ds.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index 67dbc91bccfe..3786b4e07078 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -1265,14 +1265,14 @@ static int intel_pmu_pebs_fixup_ip(struct pt_regs *regs)
>  		is_64bit = kernel_ip(to) || any_64bit_mode(regs);
>  #endif
>  		insn_init(&insn, kaddr, size, is_64bit);
> -		insn_get_length(&insn);
> +
>  		/*
>  		 * Make sure there was not a problem decoding the
>  		 * instruction and getting the length.  This is
>  		 * doubly important because we have an infinite
>  		 * loop if insn.length=0.
>  		 */
> -		if (!insn.length)
> +		if (insn_get_length(&insn) || !insn.length)

Do we really still need the !insn.length? That is, it *should* be
impossible to not fail insn_get_length() and still have a 0 length,
seeing how x86 doesn't have 0 length instructions.
Borislav Petkov Jan. 19, 2021, 10:40 a.m. UTC | #2
On Mon, Jan 04, 2021 at 02:19:19PM +0100, Peter Zijlstra wrote:
> Do we really still need the !insn.length? That is, it *should* be
> impossible to not fail insn_get_length() and still have a 0 length,
> seeing how x86 doesn't have 0 length instructions.

I was responding to the "doubly important" thing in the comment scarying
me about an infinite loop and thus left the length check in, in case
the insn decoder would have a bug and return success but still have
insn.length 0.

With the length check the endless loop won't happen but let's be
brave here ... :-)

So removed.

Patch
diff mbox series

diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index 67dbc91bccfe..3786b4e07078 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -1265,14 +1265,14 @@  static int intel_pmu_pebs_fixup_ip(struct pt_regs *regs)
 		is_64bit = kernel_ip(to) || any_64bit_mode(regs);
 #endif
 		insn_init(&insn, kaddr, size, is_64bit);
-		insn_get_length(&insn);
+
 		/*
 		 * Make sure there was not a problem decoding the
 		 * instruction and getting the length.  This is
 		 * doubly important because we have an infinite
 		 * loop if insn.length=0.
 		 */
-		if (!insn.length)
+		if (insn_get_length(&insn) || !insn.length)
 			break;
 
 		to += insn.length;