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

[Alex Elder <elder@riscstar.com>]

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