[lkml]   [2020]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [RFC] Have insn decoder functions return success/failure
On Wed, Oct 21, 2020 at 11:26:13PM +0900, Masami Hiramatsu wrote:
> Hmm, I meant someone might think it can be used for filtering the
> instruction something like,
> insn_init(insn, buf, buflen, 1);
> ret = insn_get_length(insn);
> if (!ret) {
> /* OK, this is safe */
> patch_text(buf, trampoline);
> }
> No, we need another validator for such usage.

Well, I think calling insn_get_length() should give you only the
*length* of the insn and nothing else - I mean that is what the function
is called. And I believe current use is wrong.



* Another debugging subsystem might insert this breakpoint.
* In that case, we can't recover it.
if (insn.opcode.bytes[0] == INT3_INSN_OPCODE)

So this has called get_length but it is far from clear that after that
call, the opcode bytes in insn.opcode.bytes are there.

What that should do instead IMO is this:


and *then* the return value can tell you whether the opcode bytes were
parsed properly or not. See what I mean?

That's even documented that way:

* insn_get_opcode - collect opcode(s)
* @insn: &struct insn containing instruction
* Populates @insn->opcode, updates @insn->next_byte to point past the
* opcode byte(s), and set @insn->attr (except for groups).

Similarly here:

static enum es_result vc_decode_insn(struct es_em_ctxt *ctxt)



ret = ctxt-> ? ES_OK : ES_DECODE_FAILED;

that thing wants to decode the insn but it is looking whether it parsed
an *immediate*?!

I'm not saying this is necessarily wrong - just the naming nomenclature
and the API should be properly defined when you call a function of the
insn decoder, what you are guaranteed to get and what a caller can
assume after that. And then the proper functions be called.

In the kprobes/core.c example above, it does a little further:

ddr += insn.length;

which, IMO, it should be either preceeded by a call to insn_get_length()
- yes, this time we want the insn length or, the code should call a
decoding function which gives you *both* length* and opcode bytes.
insn_decode_insn() or whatever. And *that* should be documented in that
function's kernel-doc section. And so on...

Does that make more sense?



 \ /
  Last update: 2020-10-21 18:47    [W:0.113 / U:2.352 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site