lkml.org 
[lkml]   [2017]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] uprobes/x86: emulate push insns for uprobe on x86
Yonghong,

The patch looks good to me, but I'll try to read it carefully later.
Just a couple of cosmetic nits for now.

On 11/09, Yonghong Song wrote:
>
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -53,6 +53,10 @@ struct arch_uprobe {
> u8 fixups;
> u8 ilen;
> } defparam;
> + struct {
> + u8 src_offset; /* to the start of pt_regs */
> + u8 ilen;
> + } push;
> };
> };

I know it is very easy to blame the naming ;) but src_offset doesn't
look nice. How about reg_offset ?

> +static bool push_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> + void *src_ptr = (void *)regs + auprobe->push.src_offset;
> +
> + if (emulate_push_stack(regs, *(unsigned long *)src_ptr))
> + return false;

You can declare src_ptr as

unsigned long *src_ptr = ...;

and avoid the casting below.

> +static int uprobe_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
> {
> u8 opc1 = OPCODE1(insn);
> int i;
>
> switch (opc1) {
> + case 0x50 ... 0x57:
> + return uprobe_setup_push_ops(auprobe, insn, opc1);
> +
> case 0xeb: /* jmp 8 */
> case 0xe9: /* jmp 32 */
> case 0x90: /* prefix* + nop; same as jmp with .offs = 0 */
> @@ -767,7 +863,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> if (ret)
> return ret;
>
> - ret = branch_setup_xol_ops(auprobe, &insn);
> + ret = uprobe_setup_xol_ops(auprobe, &insn);
> if (ret != -ENOSYS)
> return ret;

Well... again, this is cosmetic and I won't insist, but to me it would be
more clean to not change/rename branch_setup_xol_ops(). Instead, you can
just add

ret = uprobe_setup_push_ops(...);
if (ret != -ENOSYS)
return ret;

at the start of arch_uprobe_analyze_insn(), right after the similar
branch_setup_xol_ops() call.

Btw... please add v2 into the subject when you send the new version.

Oleg.

\
 
 \ /
  Last update: 2017-11-10 14:06    [W:0.032 / U:0.820 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site