Re: [PATCH v3 1/6] posix-cpu-timers: Always call __get_task_for_clock holding rcu_read_lock

Thomas Gleixner
2020-04-27T09:43:21Z

ebiederm@xmission.com (Eric W. Biederman) writes:

> This allows the getref flag to be removed and the callers can
> than take a task reference if needed.

That changelog lacks any form of information why this should be
changed. I can see the point vs. patch 2, but pretty please put coherent
explanations into each patch.

> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
> kernel/time/posix-cpu-timers.c | 41 +++++++++++++++++-----------------
> 1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
> index 2fd3b3fa68bf..eba41c70f0f0 100644
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -86,36 +86,34 @@ 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;

New line between declarations and code please.

> + rcu_read_lock();
> + ret = __get_task_for_clock(clock, false) ? 0 : -EINVAL;
> + rcu_read_unlock();
> + return ret;
> }

Thanks,

tglx