lkml.org 
[lkml]   [2014]   [Apr]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 6/9] uprobes/x86: Introduce uprobe_xol_ops and arch_uprobe->ops
(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@hitachi.com>

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@hitachi.com




\
 
 \ /
  Last update: 2014-04-09 15:21    [W:0.120 / U:3.312 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site