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

[Oleg Nesterov <oleg@redhat.com>]

On 02/10, Markus Metzger wrote:
>
> On Tue, 2009-02-10 at 21:21 +0100, Markus Metzger wrote:
> > On Tue, 2009-02-10 at 19:40 +0100, Oleg Nesterov wrote:
>
> > > Perhaps, for 2.6.29, we can do something like the "patch" below?
> > > 
> > > --- a/arch/x86/kernel/ptrace.c
> > > +++ b/arch/x86/kernel/ptrace.c
> > > @@ -810,11 +810,15 @@ static void ptrace_bts_untrace(struct ta
> > >  
> > >  static void ptrace_bts_detach(struct task_struct *child)
> > >  {
> > > +	// We can race with de_thread/do_wait which
> > > +	// can do ptrace_bts_untrace() before us
> > >  	if (unlikely(child->bts)) {
> > > -		ds_release_bts(child->bts);
> > > -		child->bts = NULL;
> > > -
> > > -		ptrace_bts_free_buffer(child);
> > > +		// This all will be freed by ptrace_bts_untrace()
> > > +		// later, but we should update ->mm
> > > +		down_write(->mmap_sem);
> > > +		mm->total_vm  -= bts_size;
> > > +		mm->locked_vm -= bts_size);
> > > +		up_write(->mmap_sem);
> > >  	}
> > >  }
> > >  #else
> > > 
> > 
>
> There's still a race.
> The kfree() is safe, now, but ptrace_bts_untrace() might have cleared
> child->bts_size before we can refund the memory.

Yes sure, please note the "We can race..." comment at the top
of ptrace_bts_detach().

The goal of this patch is to avoid the crash. The memory accounting
in ->mm is still not right. But at least, the tracer can not "steal"
the memory above the limits. And the "good" tracer should not exit
without detach, and it shouldn't release the tracee from sub-thread
if this can race with detach.

So, afaics, the worst thing which can happen is: the "bad" tracer
is punished by the "unfair" mm->xxx_vm numbers.

Except exec() can release the main thread whatever the tracer does...

> We need to make ptrace_bts_untrace() ignore child->bts_size and clear
> it in ptrace_bts_detach().

This is worse, now we can leak the memory if the tracer doesn't
do ptrace_detach().

Oleg.