Re: [PATCH, for 2.6.29] ptrace: fix the usage of ptrace_fork()

[Oleg Nesterov <oleg@redhat.com>]

On 02/09, Metzger, Markus T wrote:
>
> >PTRACE_BTS_CONFIG allocates ->bts_buffer via alloc_locked_buffer()
> >which updates mm->total_vm/locked_vm.
> >
> >ptrace_detach() does free_locked_buffer() which "restores" mm->xxx_vm.
>
> That's what I expect to be the normal case.
>
>
> >But if the tracer exits we are doing __ptrace_unlink()->ptrace_bts_untrace()
> >which uses a plain kfree(), in that case we don't update mm->xxx_vm ?
>
> That's correct.
> When the tracer dies, do_exit() first calls exit_mm() and then calls exit_notify(), which eventually calls __ptrace_unlink() and ptrace_bts_untrace().
>
> At this time, the tracer's mm is already gone,

It is not if we have sub-threads (which share the same ->mm),

> but the bts buffer is not.
> The code reclaims the memory; so we should not leak memory.

yes, there is no memory leak,

> The user should not see any problems with his ulimit, either, since the task that had the memory accounted against his locked and total limit is as good as dead.

again, if the process is multithreaded, it is not dead. It (other threads)
continues to run with the same ->mm. Only the tracer thread is dead.

> >Or, the tracer can reap a zombie tracee without PTRACE_DETACH, in that
> >case we don't update ->mm too.
>
> I'm not sure I understand that scenario.
> A task ptraces another task, requests branch trace, and does not detach when the tracee dies?

Yes. Again, we don't leak the memory, but the accounting in mm->total_vm/locked_vm
is not right.

> Ideally (that is, from my point of view, at least), the tracer would call
> ptrace_detach() very early in do_exit(), so ptrace would not have to distinguish
> the various ways a tracer or tracee could die.

Well, yes I agree. Please look at http://marc.info/?t=123411902800001

> >Oh, and afaics ptrace_detach()->ptrace_bts_detach() can race with the
> >tracer's sub-thread which does do_wait()->release_task() (if the tracee
> >was killed before detach takes tasklist), the kernel can crash in this
> >case.
>
> Are you saying that ptrace_detach() should call ptrace_disable() with tasklist_lock held for write?

We can't do this. And btw one of the reasons we can't is that
ptrace_bts_free_buffer() needs ->mmap_sem ;)

Oleg.