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

[Thomas Gleixner <tglx@linutronix.de>]

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