Messages in this thread | | | Date | Wed, 11 Feb 2009 23:08:21 +0100 | From | Oleg Nesterov <> | Subject | Re: [patch] x86, ptrace: fix double-free on race |
| |
On 02/11, Markus Metzger wrote: > > Ptrace_detach() races with __ptrace_unlink() if the traced task is > reaped while detaching. This might cause a double-free of the BTS > buffer. > > Change the ptrace_detach() path to only do the memory accounting in > ptrace_bts_detach() and leave the buffer free to ptrace_bts_untrace() > which will be called from __ptrace_unlink().
Thanks Markus, I think this should close the "really bad" problems for 2.6.29.
Off-topic. This is subjective, so please feel to ignore, but personally I dislike the usage of ptrace_fork() in copy_process(). And ptrace_fork() itself.
To me, this has nothing to do with ptrace at all. copy_process() or dup_task_struct() just must clear/tweak some fields after memcpy().
Perhaps it is better to kill all these ptrace_fork/arch_ptrace_fork/ x86_ptrace_fork/ptrace_bts_fork helpers, and make a single function
static inline void arch_bts_init(struct task_struct *p) { #ifdef CONFIG_X86_PTRACE_BTS if (unlikely(p->bts)) { p->bts = NULL; p->bts_buffer = NULL; p->bts_size = 0; p->thread.bts_ovfl_signal = 0; } #endif /* CONFIG_X86_PTRACE_BTS */ }
which is called by arch_dup_task_struct(). The "if (ptrace)" check in copy_process() is just wrong imho, even if it is currently correct. Let's suppose we add, say, bts_spinlock to the fields above. In that case we must initialize it even if the forking task is not ptraced.
Of course, in any case this is not 2.6.29 material.
Btw, just curious, bts_ovfl_signal is not used, except the user can set/get it via PTRACE_BTS_CONFIG/PTRACE_BTS_STATUS ?
Oleg.
| |