From 8feebc6713cd78cbba8c4e2a6d92299669052026 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Mon, 27 Apr 2020 14:32:54 -0500 Subject: posix-cpu-timer: Tidy up group_leader logic in lookup_task Replace has_group_leader_pid with thread_group_leader. Years ago Oleg suggested changing thread_group_leader to has_group_leader_pid to handle races. Looking at the code then and now I don't see how it ever helped. Especially as then the code really did need to be the thread_group_leader. Today it doesn't make a difference if thread_group_leader races with de_thread as the task returned from lookup_task in the non-thread case is just used to find values in task->signal. Since the races with de_thread have never been handled revert has_group_header_pid to thread_group_leader for clarity. Update the comment in lookup_task to remove implementation details that are no longer true and to mention task->signal instead of task->sighand, as the relevant cpu timer details are all in task->signal. Ref: 55e8c8eb2c7b ("posix-cpu-timers: Store a reference to a pid not a task") Ref: c0deae8c9587 ("posix-cpu-timers: Rcu_read_lock/unlock protect find_task_by_vpid call") Signed-off-by: "Eric W. Biederman" --- kernel/time/posix-cpu-timers.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) (limited to 'kernel/time') diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c index 2fd3b3fa68bf..e4051e417bcb 100644 --- a/kernel/time/posix-cpu-timers.c +++ b/kernel/time/posix-cpu-timers.c @@ -69,12 +69,8 @@ static struct task_struct *lookup_task(const pid_t pid, bool thread, if (gettime) { /* * For clock_gettime(PROCESS) the task does not need to be - * the actual group leader. tsk->sighand gives + * the actual group leader. task->signal gives * access to the group's clock. - * - * Timers need the group leader because they take a - * reference on it and store the task pointer until the - * timer is destroyed. */ return (p == current || thread_group_leader(p)) ? p : NULL; } @@ -82,7 +78,7 @@ static struct task_struct *lookup_task(const pid_t pid, bool thread, /* * For processes require that p is group leader. */ - return has_group_leader_pid(p) ? p : NULL; + return thread_group_leader(p) ? p : NULL; } static struct task_struct *__get_task_for_clock(const clockid_t clock, -- cgit v1.2.3 From c7f5194054e103d18d267385b866b5b90511a425 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Tue, 28 Apr 2020 13:00:39 -0500 Subject: posix-cpu-timer: Unify the now redundant code in lookup_task Now that both !thread paths through lookup_task call thread_group_leader, unify them into the single test at the end of lookup_task. This unification just makes it clear what is happening in the gettime special case of lookup_task. Signed-off-by: "Eric W. Biederman" --- kernel/time/posix-cpu-timers.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) (limited to 'kernel/time') diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c index e4051e417bcb..b7f972fb115e 100644 --- a/kernel/time/posix-cpu-timers.c +++ b/kernel/time/posix-cpu-timers.c @@ -66,14 +66,13 @@ static struct task_struct *lookup_task(const pid_t pid, bool thread, if (thread) return same_thread_group(p, current) ? p : NULL; - if (gettime) { - /* - * For clock_gettime(PROCESS) the task does not need to be - * the actual group leader. task->signal gives - * access to the group's clock. - */ - return (p == current || thread_group_leader(p)) ? p : NULL; - } + /* + * For clock_gettime(PROCESS) the task does not need to be + * the actual group leader. task->signal gives + * access to the group's clock. + */ + if (gettime && (p == current)) + return p; /* * For processes require that p is group leader. -- cgit v1.2.3 From 9bf7c32409354b4a2fa3207d369a22c8233f718c Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Sat, 25 Apr 2020 18:38:54 -0500 Subject: posix-cpu-timers: Extend rcu_read_lock removing task_struct references Now that the code stores of pid references it is no longer necessary or desirable to take a reference on task_struct in __get_task_for_clock. Instead extend the scope of rcu_read_lock and remove the reference counting on struct task_struct entirely. Signed-off-by: "Eric W. Biederman" --- kernel/time/posix-cpu-timers.c | 43 ++++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 20 deletions(-) (limited to 'kernel/time') diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c index b7f972fb115e..91996dd089a4 100644 --- a/kernel/time/posix-cpu-timers.c +++ b/kernel/time/posix-cpu-timers.c @@ -81,36 +81,36 @@ static struct task_struct *lookup_task(const pid_t pid, bool thread, } static struct task_struct *__get_task_for_clock(const clockid_t clock, - bool getref, bool gettime) + bool gettime) { const bool thread = !!CPUCLOCK_PERTHREAD(clock); const pid_t pid = CPUCLOCK_PID(clock); - struct task_struct *p; if (CPUCLOCK_WHICH(clock) >= CPUCLOCK_MAX) return NULL; - rcu_read_lock(); - p = lookup_task(pid, thread, gettime); - if (p && getref) - get_task_struct(p); - rcu_read_unlock(); - return p; + return lookup_task(pid, thread, gettime); } static inline struct task_struct *get_task_for_clock(const clockid_t clock) { - return __get_task_for_clock(clock, true, false); + return __get_task_for_clock(clock, false); } static inline struct task_struct *get_task_for_clock_get(const clockid_t clock) { - return __get_task_for_clock(clock, true, true); + return __get_task_for_clock(clock, true); } static inline int validate_clock_permissions(const clockid_t clock) { - return __get_task_for_clock(clock, false, false) ? 0 : -EINVAL; + int ret; + + rcu_read_lock(); + ret = __get_task_for_clock(clock, false) ? 0 : -EINVAL; + rcu_read_unlock(); + + return ret; } static inline enum pid_type cpu_timer_pid_type(struct k_itimer *timer) @@ -368,15 +368,18 @@ static int posix_cpu_clock_get(const clockid_t clock, struct timespec64 *tp) struct task_struct *tsk; u64 t; + rcu_read_lock(); tsk = get_task_for_clock_get(clock); - if (!tsk) + if (!tsk) { + rcu_read_unlock(); return -EINVAL; + } if (CPUCLOCK_PERTHREAD(clock)) t = cpu_clock_sample(clkid, tsk); else t = cpu_clock_sample_group(clkid, tsk, false); - put_task_struct(tsk); + rcu_read_unlock(); *tp = ns_to_timespec64(t); return 0; @@ -389,19 +392,19 @@ static int posix_cpu_clock_get(const clockid_t clock, struct timespec64 *tp) */ static int posix_cpu_timer_create(struct k_itimer *new_timer) { - struct task_struct *p = get_task_for_clock(new_timer->it_clock); + struct task_struct *p; - if (!p) + rcu_read_lock(); + p = get_task_for_clock(new_timer->it_clock); + if (!p) { + rcu_read_unlock(); return -EINVAL; + } new_timer->kclock = &clock_posix_cpu; timerqueue_init(&new_timer->it.cpu.node); new_timer->it.cpu.pid = get_task_pid(p, cpu_timer_pid_type(new_timer)); - /* - * get_task_for_clock() took a reference on @p. Drop it as the timer - * holds a reference on the pid of @p. - */ - put_task_struct(p); + rcu_read_unlock(); return 0; } -- cgit v1.2.3 From fece98260f31292dd468925c7582065bf6193212 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Mon, 27 Apr 2020 09:38:29 -0500 Subject: posix-cpu-timers: Replace cpu_timer_pid_type with clock_pid_type Taking a clock and returning a pid_type is a more general and a superset of taking a timer and returning a pid_type. Perform this generalization so that future changes may use this code on clocks as well as timers. Signed-off-by: "Eric W. Biederman" --- kernel/time/posix-cpu-timers.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'kernel/time') diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c index 91996dd089a4..42f673974d71 100644 --- a/kernel/time/posix-cpu-timers.c +++ b/kernel/time/posix-cpu-timers.c @@ -113,14 +113,14 @@ static inline int validate_clock_permissions(const clockid_t clock) return ret; } -static inline enum pid_type cpu_timer_pid_type(struct k_itimer *timer) +static inline enum pid_type clock_pid_type(const clockid_t clock) { - return CPUCLOCK_PERTHREAD(timer->it_clock) ? PIDTYPE_PID : PIDTYPE_TGID; + return CPUCLOCK_PERTHREAD(clock) ? PIDTYPE_PID : PIDTYPE_TGID; } static inline struct task_struct *cpu_timer_task_rcu(struct k_itimer *timer) { - return pid_task(timer->it.cpu.pid, cpu_timer_pid_type(timer)); + return pid_task(timer->it.cpu.pid, clock_pid_type(timer->it_clock)); } /* @@ -403,7 +403,7 @@ static int posix_cpu_timer_create(struct k_itimer *new_timer) new_timer->kclock = &clock_posix_cpu; timerqueue_init(&new_timer->it.cpu.node); - new_timer->it.cpu.pid = get_task_pid(p, cpu_timer_pid_type(new_timer)); + new_timer->it.cpu.pid = get_task_pid(p, clock_pid_type(new_timer->it_clock)); rcu_read_unlock(); return 0; } -- cgit v1.2.3 From 964987738b3fe557cb1ee37acb4e7f85e29b7cea Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Mon, 27 Apr 2020 07:55:07 -0500 Subject: posix-cpu-timers: Replace __get_task_for_clock with pid_for_clock Now that the codes store references to pids instead of referendes to tasks. Looking up a task for a clock instead of looking up a struct pid makes the code more difficult to verify it is correct than necessary. In posix_cpu_timers_create get_task_pid can race with release_task for threads and return a NULL pid. As put_pid and cpu_timer_task_rcu handle NULL pids just fine the code works without problems but it is an extra case to consider and keep in mind while verifying and modifying the code. There are races with de_thread to consider that only don't apply because thread clocks are only allowed for threads in the same thread_group. So instead of leaving a burden for people making modification to the code in the future return a rcu protected struct pid for the clock instead. The logic for __get_task_for_pid and lookup_task has been folded into the new function pid_for_clock with the only change being the logic has been modified from working on a task to working on a pid that will be returned. In posix_cpu_clock_get instead of calling pid_for_clock checking the result and then calling pid_task to get the task. The result of pid_for_clock is fed directly into pid_task. This is safe because pid_task handles NULL pids. As such an extra error check was unnecessary. Instead of hiding the flag that enables the special clock_gettime handling, I have made the 3 callers just pass the flag in themselves. That is less code and seems just as simple to work with as the wrapper functions. Historically the clock_gettime special case of allowing a process clock to be found by the thread id did not even exist [33ab0fec3352] but Thomas Gleixner reports that he has found code that uses that functionality [55e8c8eb2c7b]. Link: https://lkml.kernel.org/r/87zhaxqkwa.fsf@nanos.tec.linutronix.de/ Ref: 33ab0fec3352 ("posix-timers: Consolidate posix_cpu_clock_get()") Ref: 55e8c8eb2c7b ("posix-cpu-timers: Store a reference to a pid not a task") Signed-off-by: "Eric W. Biederman" --- kernel/time/posix-cpu-timers.c | 75 +++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 45 deletions(-) (limited to 'kernel/time') diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c index 42f673974d71..165117996ea0 100644 --- a/kernel/time/posix-cpu-timers.c +++ b/kernel/time/posix-cpu-timers.c @@ -47,59 +47,44 @@ void update_rlimit_cpu(struct task_struct *task, unsigned long rlim_new) /* * Functions for validating access to tasks. */ -static struct task_struct *lookup_task(const pid_t pid, bool thread, - bool gettime) +static struct pid *pid_for_clock(const clockid_t clock, bool gettime) { - struct task_struct *p; + const bool thread = !!CPUCLOCK_PERTHREAD(clock); + const pid_t upid = CPUCLOCK_PID(clock); + struct pid *pid; + + if (CPUCLOCK_WHICH(clock) >= CPUCLOCK_MAX) + return NULL; /* * If the encoded PID is 0, then the timer is targeted at current * or the process to which current belongs. */ - if (!pid) - return thread ? current : current->group_leader; + if (upid == 0) + return thread ? task_pid(current) : task_tgid(current); - p = find_task_by_vpid(pid); - if (!p) - return p; + pid = find_vpid(upid); + if (!pid) + return NULL; - if (thread) - return same_thread_group(p, current) ? p : NULL; + if (thread) { + struct task_struct *tsk = pid_task(pid, PIDTYPE_PID); + return (tsk && same_thread_group(tsk, current)) ? pid : NULL; + } /* - * For clock_gettime(PROCESS) the task does not need to be - * the actual group leader. task->signal gives - * access to the group's clock. + * For clock_gettime(PROCESS) allow finding the process by + * with the pid of the current task. The code needs the tgid + * of the process so that pid_task(pid, PIDTYPE_TGID) can be + * used to find the process. */ - if (gettime && (p == current)) - return p; + if (gettime && (pid == task_pid(current))) + return task_tgid(current); /* - * For processes require that p is group leader. + * For processes require that pid identifies a process. */ - return thread_group_leader(p) ? p : NULL; -} - -static struct task_struct *__get_task_for_clock(const clockid_t clock, - bool gettime) -{ - const bool thread = !!CPUCLOCK_PERTHREAD(clock); - const pid_t pid = CPUCLOCK_PID(clock); - - if (CPUCLOCK_WHICH(clock) >= CPUCLOCK_MAX) - return NULL; - - return lookup_task(pid, thread, gettime); -} - -static inline struct task_struct *get_task_for_clock(const clockid_t clock) -{ - return __get_task_for_clock(clock, false); -} - -static inline struct task_struct *get_task_for_clock_get(const clockid_t clock) -{ - return __get_task_for_clock(clock, true); + return pid_has_task(pid, PIDTYPE_TGID) ? pid : NULL; } static inline int validate_clock_permissions(const clockid_t clock) @@ -107,7 +92,7 @@ static inline int validate_clock_permissions(const clockid_t clock) int ret; rcu_read_lock(); - ret = __get_task_for_clock(clock, false) ? 0 : -EINVAL; + ret = pid_for_clock(clock, false) ? 0 : -EINVAL; rcu_read_unlock(); return ret; @@ -369,7 +354,7 @@ static int posix_cpu_clock_get(const clockid_t clock, struct timespec64 *tp) u64 t; rcu_read_lock(); - tsk = get_task_for_clock_get(clock); + tsk = pid_task(pid_for_clock(clock, true), clock_pid_type(clock)); if (!tsk) { rcu_read_unlock(); return -EINVAL; @@ -392,18 +377,18 @@ static int posix_cpu_clock_get(const clockid_t clock, struct timespec64 *tp) */ static int posix_cpu_timer_create(struct k_itimer *new_timer) { - struct task_struct *p; + struct pid *pid; rcu_read_lock(); - p = get_task_for_clock(new_timer->it_clock); - if (!p) { + pid = pid_for_clock(new_timer->it_clock, false); + if (!pid) { rcu_read_unlock(); return -EINVAL; } new_timer->kclock = &clock_posix_cpu; timerqueue_init(&new_timer->it.cpu.node); - new_timer->it.cpu.pid = get_task_pid(p, clock_pid_type(new_timer->it_clock)); + new_timer->it.cpu.pid = get_pid(pid); rcu_read_unlock(); return 0; } -- cgit v1.2.3