Messages in this thread | | | Subject | Re: [PATCH rc1-mm 2/3] coredump: shutdown current process first | From | (Eric W. Biederman) | Date | Fri, 14 Apr 2006 11:02:04 -0600 |
| |
Oleg Nesterov <oleg@tv-sign.ru> writes:
> On 04/14, Eric W. Biederman wrote: >> >> Oleg Nesterov <oleg@tv-sign.ru> writes: >> >> > On 04/10, Roland McGrath wrote: >> >> >> >> I would be inclined to restructure the inner loop something like this: >> >> >> >> p = g; >> >> while (unlikely(p->mm == NULL)) { >> >> p = next_thread(p); >> >> if (p == g) >> >> break; >> >> } >> >> if (p->mm == mm) { >> >> /* >> >> * p->sighand can't disappear, but >> >> * may be changed by de_thread() >> >> */ >> >> lock_task_sighand(p, &flags); >> >> zap_process(p); >> >> unlock_task_sighand(p, &flags); >> >> } >> > >> > Yes, I agree, this is much more understandable. >> >> There is one piece of zap_threads that still makes me uncomfortable. >> >> task_lock is used to protect p->mm. >> Therefore killing a process based upon p->mm == mm is racy >> with respect to sys_unshare I believe if we don't take >> task_lock. > > Well, unshare(CLONE_VM) is not yet supported. Currently (as I see > it) mm->mmap_sem is enough to protect against changing ->mm. Yes, > exit_mm/exec_mmap take task_lock too, so it can be used as well. > Please correct my understanding.
So what has me unsettled is that task_lock is used to protect p->mm. The other place this could be a problem is exit_mm. But it does appear that deliberately takes the mm_sem to prevent this problem. So it looks like I was just missed that trick.
> I think it is better to take ->mmap_sem in sys_unshare, this path > is rare.
Agreed.
Eric - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |