<div dir="ltr"><span style="font-size:12.8px">> The patch summary should probably be something along the lines of</span><br style="font-size:12.8px"><span style="font-size:12.8px">"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.</span><br style="font-size:12.8px">><br style="font-size:12.8px"><span style="font-size:12.8px">> 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.</span><br><div><br></div><div>That's fair.</div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">> 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.</span><br style="font-size:12.8px">><br style="font-size:12.8px"><span style="font-size:12.8px">>Is there anywhere else in the sstep code that deals well with malformed instructions?</span><span style="font-size:12.8px"><br></span></div><div><br></div><div>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.</div><div><br></div><div>Oliver</div><div><span style="font-size:12.8px"><br></span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 27, 2016 at 11:52 AM, Andrew Donnellan <span dir="ltr"><<a href="mailto:andrew.donnellan@au1.ibm.com" target="_blank">andrew.donnellan@au1.ibm.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 25/01/16 17:55, Oliver O'Halloran wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I think this bug can only be triggered if the instruction to<br>
simulate is malformed. The switch in the else case only handles<br>
the zero and one case, but it extracts bits 4:1 from the<br>
instruction word so it may be other values. It's pretty minor, but<br>
a bug is a bug.<br>
<br>
Signed-off-by: Oliver O'Halloran <<a href="mailto:oohall@gmail.com" target="_blank">oohall@gmail.com</a>><br>
</blockquote>
<br></span>
The patch summary should probably be something along the lines of<br>
"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.<br>
<br>
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.<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
--- a/arch/powerpc/lib/sstep.c<br>
+++ b/arch/powerpc/lib/sstep.c<br>
@@ -925,6 +925,7 @@ int __kprobes analyse_instr(struct instruction_op *op, struct pt_regs *regs,<br>
}<br>
}<br>
#endif<br>
+ break; /* illegal instruction */<br>
</blockquote>
<br></span>
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.<br>
<br>
Is there anywhere else in the sstep code that deals well with malformed instructions?<span class="HOEnZb"><font color="#888888"><br>
<br>
-- <br>
Andrew Donnellan Software Engineer, OzLabs<br>
<a href="mailto:andrew.donnellan@au1.ibm.com" target="_blank">andrew.donnellan@au1.ibm.com</a> Australia Development Lab, Canberra<br>
<a href="tel:%2B61%202%206201%208874" value="+61262018874" target="_blank">+61 2 6201 8874</a> (work) IBM Australia Limited<br>
<br>
</font></span></blockquote></div><br></div>