Re: [PATCH v3 2/6] posix-cpu-timers: Use PIDTYPE_TGID to simplify the logic in lookup_task

[Thomas Gleixner <tglx@linutronix.de>]

ebiederm@xmission.com (Eric W. Biederman) writes:
> Using pid_task(find_vpid(N), PIDTYPE_TGID) guarantees that if a task
> is found it is at that moment the thread group leader.  Which removes
> the need for the follow on test has_group_leader_pid.
>
> I have reorganized the rest of the code in lookup_task for clarity,
> and created a common return for most of the code.

Sorry, it's way harder to read than the very explicit exits which were
there before.

> The special case for clock_gettime with "pid == gettid" is not my
> favorite.  I strongly suspect it isn't used as gettid is such a pain,
> and passing 0 is much easier.  Still it is easier to keep this special
> case than to do the reasarch that will show it isn't used.

It might be not your favorite, but when I refactored the code I learned
the hard way that one of the test suites has assumptions that
clock_gettime(PROCESS) works from any task of a group and not just for
the group leader. Sure we could fix the test suite, but test code tends
to be copied ...

>  /*
>   * Functions for validating access to tasks.
>   */
> -static struct task_struct *lookup_task(const pid_t pid, bool thread,
> +static struct task_struct *lookup_task(const pid_t which_pid, bool thread,
>  				       bool gettime)
>  {
>  	struct task_struct *p;
> +	struct pid *pid;
>  
>  	/*
>  	 * If the encoded PID is 0, then the timer is targeted at current
>  	 * or the process to which current belongs.
>  	 */
> -	if (!pid)
> +	if (!which_pid)
>  		return thread ? current : current->group_leader;
>  
> -	p = find_task_by_vpid(pid);
> -	if (!p)
> -		return p;
> -
> -	if (thread)
> -		return same_thread_group(p, current) ? p : NULL;
> -
> -	if (gettime) {
> +	pid = find_vpid(which_pid);
> +	if (thread) {
> +		p = pid_task(pid, PIDTYPE_PID);
> +		if (p && !same_thread_group(p, current))
> +			p = NULL;
> +	} else {
>  		/*
>  		 * For clock_gettime(PROCESS) the task does not need to be
>  		 * the actual group leader. tsk->sighand gives
> @@ -76,13 +75,13 @@ static struct task_struct *lookup_task(const pid_t pid, bool thread,
>  		 * reference on it and store the task pointer until the
>  		 * timer is destroyed.

Btw, this comment is wrong since

     55e8c8eb2c7b ("posix-cpu-timers: Store a reference to a pid not a task")

Thanks,

        tglx