Hi Robin,
Thank you for having a look at my patchset.
On 27.09.2018 19:01, Robin Murphy wrote:
> On 26/09/18 13:12, Maciej Slodczyk wrote:
> [...]
>> @@ -38,16 +78,44 @@ int arch_uprobe_analyze_insn(struct arch_uprobe
>> *auprobe, struct mm_struct *mm,
>> unsigned long addr)
>> {
>> probes_opcode_t insn;
>> + enum probes_insn retval;
>> + unsigned int bpinsn;
>> - /* TODO: Currently we do not support AARCH32 instruction probing */
>> - if (mm->context.flags & MMCF_AARCH32)
>> - return -ENOTSUPP;
>> - else if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE))
>> + insn = *(probes_opcode_t *)(&auprobe->insn[0]);
>> +
>> + if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE))
>> return -EINVAL;
>> - insn = *(probes_opcode_t *)(&auprobe->insn[0]);
>> + /* check if AARCH32 */
>> + if (is_compat_task()) {
>> +
>> + /* Thumb is not supported yet */
>> + if (addr & 0x3)
>
> I'm only skimming, so forgive me if I'm missing something which should
> be obvious, but this has a big red flag all over it. If "addr" is the
> actual instruction address (or even a branch target, for a
> non-interworking branch), plenty of Thumb instructions will just happen
> to lie at 4-byte-aligned addresses anyway.
>
That's the same way Thumb instructions are filtered out in arch/arm
uprobes and kprobes code. I believe that at this point all Thumb
instruction have bit 0 set. Please correct me if I'm wrong.
> Furthermore, how would this check ever catch anything anyway given
> !IS_ALIGNED(addr, AARCH64_INSN_SIZE) above?
You're right, there's no point in checking it here. I'll fix it in v3.
Thank you,
Maciej
