Re: [PATCH] kernel/sys.c: use RCU when accessing task_struct->real_parent

[Oleg Nesterov <oleg@redhat.com>]

Just in case, I am travelling without my work laptop until Aug 19,
can't read the code and I rarely read emails...

On 08/11, Alex Elder wrote:
>
> In the setpgid() syscall definition, a check is made to determine
> whether the target process is in the same thread group as the
> current process.  The check accesses the target process's real_parent
> pointer directly, however that field is supposed to be accessed via
> RCU.  Use rcu_dereference() to avoid this C=1 build warning:
>
>   kernel/sys.c:1144:32: warning: incorrect type in argument 1 (different address spaces)

This is the sparse warning... Do we really care? We have a lot
more users of ->real_parent without rcu_dereference().

Note also that the "rcu" annotation of ->real_parent is misleading.
If the task exits, task->real_parent points to nowhere. Same for
->group_leader.

This reminds me... months ago I was going to introduce the helper
to access ->group_leader and move it to signal_struct. Then I was
going to do the same with ->real_parent. I sent some preparations,
but then I was distracted. I'll try to return to this after PTO.

>  SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)
>  {
>  	struct task_struct *p;
> +	struct task_struct *real_parent;
>  	struct task_struct *group_leader = current->group_leader;
>  	struct pid *pids[PIDTYPE_MAX] = { 0 };
>  	struct pid *pgrp;
> @@ -1141,7 +1142,8 @@ SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)
>  	if (!thread_group_leader(p))
>  		goto out;
>
> -	if (same_thread_group(p->real_parent, group_leader)) {
> +	real_parent = rcu_dereference(p->real_parent);
> +	if (same_thread_group(real_parent, group_leader)) {
>  		err = -EPERM;
>  		if (task_session(p) != task_session(group_leader))
>  			goto out;

IIRC, this code runs under tasklist_lock (at least it should ;)
->real_parent is stable.

I'd prefer to leave this code as is (see above), but even if we
really want to shut up sparse we don't need rcu_dereference()
anyway, we are not going to dereference this pointer.

We have other helpers to read the "rcu" pointers, but I can't
recall the names...

Oleg.