[lkml]   [2012]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH 2/2] mm/linux-next: Fix rcu locking in vm_is_stack
    On 03/07, Siddhesh Poyarekar wrote:
    > Take rcu read lock before we do anything at all with the threadgroup
    > list. Also use list_first_entry_rcu to safely get the reference to the
    > first task in the list.

    Sorry, but both patches are absolutely wrong, afaics. This change fixes
    nothing, just obfuscates the code.

    Probably my explanation was not clear, let me try again.

    rcu_read_lock() can not help without the additional checks. By the
    time you take it, task->thread_group->next can point to nowhere.


    > @@ -3932,18 +3932,20 @@ pid_t vm_is_stack(struct task_struct *task,
    > return task->pid;
    > if (in_group) {
    > - struct task_struct *t = task;
    > + struct task_struct *t;
    > rcu_read_lock();
    > - while_each_thread(task, t) {
    > + t = list_first_entry_rcu(&task->thread_group,
    > + struct task_struct, thread_group);

    It is not safe to dereference this pointer.

    Once again. You have the task_struct *task. It exits,
    but task->thread_group->next still points to another thread T. Now suppose
    that T exits too. But task->thread_group->next was not changed, it still
    points to T. RCU grace period passes, T is freed.

    After that you take rcu_read_lock(), but it is too late! >next points to
    the already freed/reused memory. How can list_first_entry_rcu() help?

    What you need is something like

    if (!pid_alive(task))
    goto done;

    t = task;
    do {
    } while_each(task, t);


    And. Imho it is not good to have the (afaics exactly?) same code in
    mm/nommu.c, even with the same names. Why it is not possible to make
    a single definition?


     \ /
      Last update: 2012-03-07 16:49    [W:0.023 / U:37.228 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site