![]() | ||||||||||
Messages in this thread |
Linus Torvalds wrote: > > On Fri, 30 Sep 2005, Oleg Nesterov wrote: > > > > Roland, could you please explain this code in wait_task_stopped() > > > > if (!exit_code || p->state > TASK_STOPPED) > > goto bail_ref; > > Regardless of any other explanations, it turns out that "p->state" can be > something like "TASK_RUNNING | TASK_NONINTERACTIVE", and then this would > trigger totally incorrectly. > > > It looks like "WSTOPPED | WNOWAIT is illegal for TASK_TRACED child" > > to me. Is this correct? I think no. > > No, I think it's correct. If you have a traced child, you can't just wait > for it. You need to use ptrace to release it first. But it is ok to do do_wait(WSTOPPED /* without WNOWAIT */) for TASK_TRACED child. wait_task_stopped() does not actually wait, it just eats ->exit_code. > > Actually, I don't understand why we are checking p->state at all, we > > already dropped tasklist_lock, the state can change at any monent. > > If it's TASK_TRACED, and it's our child, then it shouldn't be changing. It can be child of other thread in our thread group (do_wait() iterates over all threads ->children lists) and that thread can do PTRACE_DETACH. But this does not matter. > Besides, even if it does, we had a perfectly fine race, and we'll have > been woken up again and we'll just go through the do_wait() loop once > more. Yes, > So I think the code is mostly correct. But that ">" is definitely > incorrect. > > Maybe it should just be > > if (!exit_code || (p->state & TASK_TRACED)) > > instead? I still think that checking p->state here just pointless and confusing. The task was TASK_STOPPED or TASK_TRACED on entering, we have WNOWAIT flag, we should just call wait_noreap_copyout(). And 'p' can change it's ->state just after the check. And I think that wait_task_stopped() should get ->exit_code and ->si_code atomically under tasklist_lock, p->ptrace can be changed after we dropped that lock. Btw, do_wait(): case TASK_STOPPED: if (!(options & WUNTRACED) && !my_ptrace_child(p)) continue; Looks like it should be '||' here? Oleg. - 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/ | |||||||||
| Last update: 2005-10-01 09:22 [W:0.324 / U:0.280 seconds] ©2003-2008 Jasper Spaans | ||||||||||