Re: [PATCH v2 6/9] uprobes/x86: Introduce uprobe_xol_ops and arch_uprobe->ops

[Masami Hiramatsu]

(2014/04/09 1:10), Oleg Nesterov wrote:
> On 04/08, Masami Hiramatsu wrote:
>>
>> (2014/04/05 3:51), Oleg Nesterov wrote:
>>>
>>> TODO: An error from adjust_ret_addr() shouldn't be silently ignored,
>>> we should teach arch_uprobe_post_xol() or handle_singlestep() paths
>>> to restart the probed insn in this case. And probably "adjust" can
>>> be simplified and turned into set_ret_addr(). It seems that we do
>>> not really need copy_from_user(), we can always calculate the value
>>> we need to write into *regs->sp.
>>
>> It seems that you fixed this in 8/9, we don't need the TODO list in
>> the description.
> 
> Well, OK, I'll update the changelog and remove the "error ... ignored"
> part. Although to be honest, I do not understand why do you think it
> is bad to document the other problems you found while you were writing
> the patch.

Because you know how to fix that and you just can do that in following
patches :). In that case, you don't need to state it here.


>>> +	if (auprobe->ops->emulate)
>>> +		return auprobe->ops->emulate(auprobe, regs);
>>> +
>>> +	/* TODO: move this code into ->emulate() hook */
>>
>> If you think this can move into the emulate(),
> 
> Yes sure,
> 
>> you should do in this
>> patch.
> 
> No, sorry, I strongly disagree, this should come as a separate change,
> and only after "Emulate jmp's".

Ah, I see, with your RFC series. :)

>> Since the following code runs by default, there should be
>> no problem to do that.
> 
> Hmm. Not sure I understand "by default".

I meant that the auprobe->ops->emulate() is always skipped and the
old code is always run, since the default_emulate_op() is NULL at
this point.

> If you meant that this should
> go into default_emulate_op() (which we do not have), then I strongly
> disagree again.
> 
> It should not, if nothing else we need to record insn->length somewhere,
> this should go into ttt_emulate_op() we add later. And it simply looks
> much more natural to handle jmp's and nop's together.

I see, OK with ttt_emulate_op() series.

Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>

Thank you :)

> 
> Masami, this time I simply can't understand your objections, please
> clarify.
> 
> Thanks,
> 
> Oleg.
> 
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@xxxxxxxxxxx


--
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/