Re: [PATCH v4 3/6] arm64: Kprobes with single stepping support

From: David Long
Date: Wed Jan 21 2015 - 13:03:19 EST


On 01/19/15 04:03, Pratyush Anand wrote:


On Saturday 17 January 2015 12:58 AM, David Long wrote:
+static bool aarch64_insn_is_steppable(u32 insn)
+{
+ if (aarch64_get_insn_class(insn) == AARCH64_INSN_CLS_BR_SYS) {
+ if (aarch64_insn_is_branch(insn))
+ return false;
+
+ /* modification of daif creates issues */
+ if (aarch64_insn_is_msr_daif(insn))
+ return false;
+
+ if (aarch64_insn_is_hint(insn))
+ return aarch64_insn_is_nop(insn);
+
+ return true;
+ }
+
+ if (aarch64_insn_uses_literal(insn))
+ return false;
+
+ if (aarch64_insn_is_exclusive(insn))
+ return false;
+
+ return true;

Default true return may not be a good idea until we are sure that we
are returning false for all possible
simulation and rejection cases. In my opinion, its better to return
true only for steppable and false for
all remaining.


I struggled a little with this when I did it but I decided if the
question was: "should we have to recognize every instruction before
deciding it was single-steppable or should we only recognize
instructions that are *not* single-steppable", maybe it was OK to do the
latter while recognizing extensions to the instruction set *could* end
up (temporarly) allowing us to try and fail (badly) at single-stepping
any problematic new instructions. Certainly opinions could differ. If

Lets see what others say, but I see that this approach will result in
undesired behavior. For example: a probe has been tried to insert to svc
instruction. SVC or any other exception generation instruction is
expected to be rejected. But, current aarch64_insn_is_steppable will
return true for it and then kprobe/uprobe code will allow to insert
probe at that instruction, which will be wrong, no? I mean, I do not see
a way to get into last else (INSN_REJECTED) of arm_kprobe_decode_insn.

So, if we go with this approach we need to insure that we cover all
simulation-able and reject-able cases in aarch64_insn_is_steppable.


yes, of course. Any case that's missing in the current code needs to be fixed. If the result starts to look less practical than the table-driven code then the new approach needs to be discarded.

~Pratyush



the consensus is that we can't allow this to ever happen (because old
kprobe code is running on new hardware) then I think the only choice is
to return to parsing binary tables. Hopefully I could still find a way
to leverage insn.c in that case.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/