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

[Alex Elder]

On 8/12/26 2:55 AM, Oleg Nesterov wrote:
> 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.

Right, there are some cases where there is other protection as
well (like the one you point out below).

> 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.

Yes, this is a better solution overall.  I sent this patch to
address one warning I kept seeing in my builds, but Andrew
pointed out there are lots of others that would also warn.

> >   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.

OK that's perfectly fine with me.  I retract this patch,
and will look forward to you introducing helpers to address
this in a more general way.

Thanks a lot.

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