Re: [PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

(2013/12/13 4:46), Oleg Nesterov wrote:
> On 12/12, Masami Hiramatsu wrote:
>>
>> (2013/12/12 3:11), Oleg Nesterov wrote:
>>> On 12/11, Masami Hiramatsu wrote:
>>>>
>>>> But it could skip the handler_chain silently. It could confuse users
>>>> why their probe doesn't hit as expected.
>>>
>>> No, we will restart the same (probed) instruction, handle_swbp()
>>> will be called again, get_utask() will be called again.
>>
>> Hmm, in that case, how would you avoid infinite recursive loop??
> 
> Masami, I do not understand your concerns ;) see below.
> 
>> Would you repeat it until get_utask() != NULL?
> 
> Yes, the task will loop until kmalloc(GFP_KERNEL) succeeds, and I see
> nothing wrong here.
> 
> Just in case, let me remind that it won't loop in kernel mode, it can
> take a signal, it can be killed. And it is not recursive, this is
> like restart after page fault (which btw can fault again if the page
> was unmapped again, and "in theory" this loop can be infinite too).

Ah! I got it :)

> And why this is bad? Once again, this is GFP_KERNEL allocation, if it
> loops "indefinitely" there is something wrong. Even a single GFP_KERNEL
> failure likely means the task is already killed by oom, so it will
> simply exit when it returns to user-mode.

Indeed. It should be killed.

> And how this differs from, say, the "endless" should_alloc_retry() loop
> in __alloc_pages_slowpath() ? And note that in this case we loop in
> kernel mode. Of course this is not possible "in practice", but the same
> is true for the "endless" loop you are worried about.

Agreed, at least that is not uprobe's business :)

> 
>>>> Hmm, in that case, should uprobes handlers never be called on ppc with
>>>> this change?
>>>
>>> Why? With this change ppc will have ->utask != NULL even if it doesn't
>>> need it at all.
>>
>> Ah, I see. This changes that.
> 
> Yes, this is why the changelog says "a bit unfortunate", we allocate the
> memory even there is no trace_uprobe consumer. So it would be nice to
> cleanup this later somehow, but imho this is a low priority problem and
> perhaps we will simply postulate that uprobe_consumer->handler() can
> rely on task->utask != NULL and remove get_utask() from pre_ssout().
> The only necessary cleanup (in my opinion) is that we should add another
> member into the union in uprobe_task for trace_uprobe.c, but again I
> think we should do this later to avoid the (potentially conflicting)
> changes in this series.
> 
> Oleg.
> 

Thank you,


-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com