Re: [PATCH] drm/i915: Fix refcount leak and possible NULL pointer dereference.

[Tetsuo Handa]

Hello.

Chris Wilson wrote:
> > Since get_pid_task() grabs a reference on the task_struct, we have to drop the
> > refcount after reading that task\'s comm name. Also, directly reading like
> > get_pid_task()->comm can trigger an oops when get_pid_task() returned NULL.
> 
> The second issue is moot as file itself cannot exist if the task_struct
> is NULL, and the task_struct cannot be destroyed until we finish the
> function. The simpler fix would appear to be s/get_pid_task/pid_task/

I didn\'t catch. Please see below sample module.

pid_task() will return NULL if the target task has already died.
The target task could be killed (e.g. by the OOM killer) at any time, and
whether the target task\'s task_struct is not yet destroyed is irrelevant
for pid_task().

get_pid_task(file->pid, PIDTYPE_PID)->comm by chance didn\'t trigger
oops because offsetof(struct task_struct, comm) < PAGE_SIZE is true.

---------- sample start ----------
#include <linux/module.h>
#include <linux/delay.h>
#include <linux/kthread.h>

static int test_thread(void *unused)
{
        return 0;
}

static int __init test_init(void)
{
        struct task_struct *task = kthread_create(test_thread, NULL, \"test\");
        if (!IS_ERR(task)) {
                struct pid *pid;
                struct task_struct *t;
                get_task_struct(task);
                pid = get_pid(task->pids[PIDTYPE_PID].pid);
                rcu_read_lock();
                t = pid_task(pid, PIDTYPE_PID);
                rcu_read_unlock();
                printk(\"pid_task(%p)=%p\\n\", pid, t);
                wake_up_process(task);
                ssleep(1);
                rcu_read_lock();
                t = pid_task(pid, PIDTYPE_PID);
                rcu_read_unlock();
                printk(\"pid_task(%p)=%p\\n\", pid, t);
                put_task_struct(task);
                put_pid(pid);
        }
        return -EINVAL;
}

module_init(test_init);
MODULE_LICENSE(\"GPL\");
---------- sample end ----------

---------- output start ----------
pid_task(ffff8800474bff80)=ffff8800435b0e10
pid_task(ffff8800474bff80)=          (null)
---------- output end ----------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/