[lkml]   [2004]   [Sep]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH]: Re: kernel 2.6.9-rc1-mm4 oops
    Ingo Molnar wrote:
    > * Kirill Korotaev <> wrote:

    >>This patch removes sighand checks from the next_thread(), since they
    >>are incorrect and has nothing to do with the next_thread() function.
    >>So they could trigger BUG() when there were no actually bug at all.
    > the problem is, generally it is not valid to have a thread on the thread
    > list that has no ->sighand structure. This is what happens when we exit
    > a task:
    > write_lock_irq(&tasklist_lock);
    > ...
    > __exit_sighand(p);
    > __unhash_process(p);

    > the BUG() is useful for all the code that uses next_thread() - you can
    > only do a safe next_thread() iteration if you've locked ->sighand.

    1. I don't see spin_lock() on p->sighand->siglock in do_task_stat()
    before calling next_thread(). And the check inside next_thread() permits
    only one of the locks to be taken:

    if (!spin_is_locked(&p->sighand->siglock) &&

    which is probably wrong, since tasklist_lock is always required!

    2. I think the idea of checking sighand is quite obscure.
    Probably it would be better to call pid_alive() for check at such places
    in proc, isn't it?

    3. And yes, now I agree that this check and BUG() prevented
    next_thread() from walking through the deleted list_head in tsk->pid_list.

    But I would propose to reorganize these checks in next_thread() to
    something like this:

    if (!rwlock_is_locked(&tasklist_lock) || p->pids[PIDTYPE_TGID].nr == 0)

    the last check ensures that we are still hashed and this check is more
    straithforward for understanding, agree?

    4. If we do checks this way, then we can found strange proc numeric
    results. Suppose, you have read the do_task_stat() and it iterated
    through the threads and summed the times in this loop with
    next_thread(). Next, the thread dies and you can read the results w/o
    this loop and threads times, only this thread stats. Looks a bit
    invalid. Don't you think so?
    Maybe we should return an error?

    > so i believe your fix papers over the real bug which is the use of an
    > almost-dead task for thread iterations. Since we've already done
    > __unhash_process() not doing the BUG introduces a more subtle bug: the
    > use of the stale PID pointers! So i believe the right fix is the one
    > below, which (under the safety of read_lock(tasklock)) checks for the
    > availability of task->sighand - and skips the thread iterations if so.
    You patch is correct. Though I would like to hear on my previous notes
    about pid_alive().


    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2005-03-22 14:06    [W:0.043 / U:0.140 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site