remove the no longer needed pid_alive() check in __task_pid_nr_ns()

Submitted by Oleg Nesterov on April 21, 2020, 10:19 a.m.

Details

Message ID 20200421101904.GA9358@redhat.com
State New
Series "signal: Avoid corrupting si_pid and si_uid in do_notify_parent"
Headers show

Commit Message

Oleg Nesterov April 21, 2020, 10:19 a.m.
Starting from 2c4704756cab ("pids: Move the pgrp and session pid pointers
from task_struct to signal_struct") __task_pid_nr_ns() doesn't dereference
task->group_leader, we can remove the pid_alive() check.

pid_nr_ns() has to check pid != NULL anyway, pid_alive() just adds the
unnecessary confusion.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/pid.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/kernel/pid.c b/kernel/pid.c
index bc21c0fb26d8..47221db038e2 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -475,8 +475,7 @@  pid_t __task_pid_nr_ns(struct task_struct *task, enum pid_type type,
 	rcu_read_lock();
 	if (!ns)
 		ns = task_active_pid_ns(current);
-	if (likely(pid_alive(task)))
-		nr = pid_nr_ns(rcu_dereference(*task_pid_ptr(task, type)), ns);
+	nr = pid_nr_ns(rcu_dereference(*task_pid_ptr(task, type)), ns);
 	rcu_read_unlock();
 
 	return nr;

Comments

Christian Brauner April 21, 2020, 10:50 a.m.
On Tue, Apr 21, 2020 at 12:19:04PM +0200, Oleg Nesterov wrote:
> Starting from 2c4704756cab ("pids: Move the pgrp and session pid pointers
> from task_struct to signal_struct") __task_pid_nr_ns() doesn't dereference
> task->group_leader, we can remove the pid_alive() check.
> 
> pid_nr_ns() has to check pid != NULL anyway, pid_alive() just adds the
> unnecessary confusion.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
Eric W. Biederman April 21, 2020, 3:05 p.m.
Oleg Nesterov <oleg@redhat.com> writes:

> Starting from 2c4704756cab ("pids: Move the pgrp and session pid pointers
> from task_struct to signal_struct") __task_pid_nr_ns() doesn't dereference
> task->group_leader, we can remove the pid_alive() check.
>
> pid_nr_ns() has to check pid != NULL anyway, pid_alive() just adds the
> unnecessary confusion.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>

> ---

Good catch that does simplify things.

Eric

>  kernel/pid.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/pid.c b/kernel/pid.c
> index bc21c0fb26d8..47221db038e2 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -475,8 +475,7 @@ pid_t __task_pid_nr_ns(struct task_struct *task, enum pid_type type,
>  	rcu_read_lock();
>  	if (!ns)
>  		ns = task_active_pid_ns(current);
> -	if (likely(pid_alive(task)))
> -		nr = pid_nr_ns(rcu_dereference(*task_pid_ptr(task, type)), ns);
> +	nr = pid_nr_ns(rcu_dereference(*task_pid_ptr(task, type)), ns);
>  	rcu_read_unlock();
>  
>  	return nr;
Oleg Nesterov April 24, 2020, 6:05 p.m.
On 04/21, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > Starting from 2c4704756cab ("pids: Move the pgrp and session pid pointers
> > from task_struct to signal_struct") __task_pid_nr_ns() doesn't dereference
> > task->group_leader, we can remove the pid_alive() check.
> >
> > pid_nr_ns() has to check pid != NULL anyway, pid_alive() just adds the
> > unnecessary confusion.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>

Thanks, can you take this patch?

Oleg.
Eric W. Biederman April 24, 2020, 7:54 p.m.
Oleg Nesterov <oleg@redhat.com> writes:

> On 04/21, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.com> writes:
>>
>> > Starting from 2c4704756cab ("pids: Move the pgrp and session pid pointers
>> > from task_struct to signal_struct") __task_pid_nr_ns() doesn't dereference
>> > task->group_leader, we can remove the pid_alive() check.
>> >
>> > pid_nr_ns() has to check pid != NULL anyway, pid_alive() just adds the
>> > unnecessary confusion.
>> >
>> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> Thanks, can you take this patch?

I plan to.  Probably on a topic branch for signals.  I am sorting
that out now.

Eric