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

[Oleg Nesterov <oleg@redhat.com>]

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).

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.

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.

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