Messages in this thread | | | Subject | Re: [PATCH] drm/i915: Fix refcount leak and possible NULL pointer dereference. | From | Tetsuo Handa <> | Date | Thu, 26 Dec 2013 09:45:02 +0900 |
| |
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 ----------
| |