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

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

On 25/01/16 17:55, Oliver O'Halloran wrote:
> I think this bug can only be triggered if the instruction to
> simulate is malformed. The switch in the else case only handles
> the zero and one case, but it extracts bits 4:1 from the
> instruction word so it may be other values. It's pretty minor, but
> a bug is a bug.
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

The patch summary should probably be something along the lines of
"powerpc/sstep: fix switch fall-through when analysing malformed rld* 
instructions" or similar. The rest of the message should have the more 
specific details of the bug you're fixing.

In general, we always mention the affected subsystems in the patch 
summary line and write both the summary line and the message so that 
other developers can get a quick understanding of what the patch does 
without actually needing to read the code. Keep in mind that commit 
messages will show up in the git logs of every kernel developer, not 
just powerpc people.

> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -925,6 +925,7 @@ int __kprobes analyse_instr(struct instruction_op *op, struct pt_regs *regs,
>   			}
>   		}
>   #endif
> +	break; /* illegal instruction */

I had a cursory glance at the code and it's not obvious to me that this 
is the correct way to deal with an invalid instruction. What happens 
when you break out of the switch? It looks like it just ends up 
returning 0, the same as any other instruction that isn't executed 
directly in the analyse_instr() stage.

Is there anywhere else in the sstep code that deals well with malformed 
instructions?

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