Re: [RFC] Have insn decoder functions return success/failure

From: Masami Hiramatsu <mhiramat@kernel.org>
To: Borislav Petkov <bp@alien8.de>
Cc: Andy Lutomirski <luto@kernel.org>, x86-ml <x86@kernel.org>,
	Joerg Roedel <jroedel@suse.de>,
	lkml <linux-kernel@vger.kernel.org>
Subject: Re: [RFC] Have insn decoder functions return success/failure
Date: Sat, 24 Oct 2020 16:13:15 +0900	[thread overview]
Message-ID: <20201024161315.267499b854b1d08213f7c445@kernel.org> (raw)
In-Reply-To: <20201023232741.GF23324@zn.tnic>

On Sat, 24 Oct 2020 01:27:41 +0200
Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Oct 23, 2020 at 07:47:04PM +0900, Masami Hiramatsu wrote:
> > Thanks! I look forward to it.
> 
> Ok, here's a first stab, it is a single big diff and totally untested
> but it should show what I mean. I've made some notes while converting,
> as I went along.

Thanks, so will you split this into several patches, since I saw some
cleanups in this patch?

> 
> Have a look at insn_decode() and its call sites: they are almost trivial
> now because caller needs simply to do:
> 
> 	if (insn_decode(insn, buffer, ...))
> 
> and not care about any helper functions.

Yeah, that's good to me because in the most cases, user needs prefix,
length or total decoded info.

BTW, it seems you returns 1 for errors, I rather like -EINVAL or -EILSEQ
for errors so that user can also write

 if (insn_decode() < 0)
   ...

I think "positive" and "zero" pair can easily mislead user to "true" and
"false" trap.

> For some of the call sites it still makes sense to do a piecemeal insn
> decoding and I've left them this way but they can be converted too, if
> one wants.

Yeah, for the kprobes, if you see the insn_init() and insn_get_length()
those can be replaced with one insn_decode(). 

> In any case, just have a look please and lemme know if that looks OKish.
> I'll do the actual splitting and testing afterwards.

Except for the return value, it looks good to me.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>