Re: [PATCH] Fix fall-through from case 30 (rld*) to case 31

[Andrew Donnellan <andrew.donnellan@au1.ibm.com>]

On 27/01/16 16:29, oliver wrote:
>>Is there anywhere else in the sstep code that deals well with malformed instructions?
>
> When you break out of the switch the opcode type is marked as unknown
> and when further attempts to parse the instruction fail it returns zero
> to indicate failure. Also, many of the instructions handled by the
> function are only valid in 64bit mode. For 32bit processors these
> instructions would be illegal and the code that handles them is
> #ifdef`ed out when compiling for 32 bit platforms so simply breaking out
> of the switch and letting it propagate should be the right move here.

analyse_instr() returns 0 whenever it analyses but does not execute an 
instruction - it's not a failure as such.

In emulate_step(), if analyse_instr() returns 0 it will test for a bunch 
of instruction classes which require memory operations, and classes 
which can't be single-stepped. UNKNOWN isn't handled specifically, so 
it'll skip all that and return 0 at the end (meaning the step was not 
successfully emulated - as opposed to -1, which is used for instructions 
that are not allowed to be stepped).

This in turn is handled differently depending on whether emulate_step() 
is invoked in the kprobes, uprobes or hw_breakpoint code.

Rather than breaking out and relying on behaviour later in the code, I'd 
suggest either:

  - creating a goto label for bad instructions that clearly sets the 
type to UNKNOWN and returns 0 (and maybe adding some handling for that 
in emulate_step(), raise some kind of nice big warning at the very least)

  - make analyse_instr() return -1 on invalid instructions, which 
emulate_step() will immediately propagate, then make sure that whoever 
calls emulate_step() handles that appropriately


Andrew

-- 
Andrew Donnellan              Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com  Australia Development Lab, Canberra
+61 2 6201 8874 (work)        IBM Australia Limited