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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 27 Apr 2020 06:53:42 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linux FS Devel <linux-fsdevel@...r.kernel.org>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Alexey Gladkov <legion@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Alexey Gladkov <gladkov.alexey@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>,
        "Paul E. McKenney" <paulmck@...nel.org>
Subject: Re: [PATCH v3 1/6] posix-cpu-timers: Always call __get_task_for_clock holding rcu_read_lock

Thomas Gleixner <tglx@...utronix.de> writes:

> ebiederm@...ssion.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.

Well excess flags bad.  But in this case I can do even better.

The code no longer takes a reference on task_struct so this patch
removes unnecessary code.

I will see if I can say that better.



>> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.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