Re: [PATCH] signal: use task parameter instead of current in task_join_group_stop()

[Josh Law <objecting@objecting.org>]

On 15 March 2026 19:29:06 GMT, Oleg Nesterov <oleg@redhat.com> wrote:
>Hi Josh,
>
>Well, this is subjective, but I don't like your patch in any case...
>In fact I think it is wrong. See below.
>
>On 03/15, Josh Law wrote:
>>
>> task_join_group_stop() takes a task_struct parameter but reads jobctl
>> and signal from current instead.
>
>And in my opinion this is what it should actually do.
>
>Because task_join_group_stop(p) asks 'p' to join the current's group-stop
>if it is active.
>
>And to me this logic looks very clear. but may be task_join_group_stop()
>should be renamed to make it even more clear, I dunno.
>
>> This works today because the sole
>> caller (copy_process) always passes a new thread in the same thread
>> group as current, so task->signal == current->signal and
>> task->jobctl is a copy of current->jobctl from dup_task_struct().
>
>I see it differently.
>
>Your change assumes that task->jobctl was correctly copied by dup_task_struct().
>We should not rely on that.
>
>And at first glance we can't rely on that. current->jobctl can change between between
>dup_task_struct() and copy_process() -> spin_lock(&current->sighand->siglock);
>
>Oleg.
>
>> Use the task parameter directly so the function is self-consistent
>> with its API and will not silently break if a future caller passes a
>> task from a different thread group.
>> 
>> Signed-off-by: Josh Law <objecting@objecting.org>
>> ---
>>  kernel/signal.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index 116cf678c4a3..cb417e3674ed 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -386,8 +386,8 @@ static bool task_participate_group_stop(struct task_struct *task)
>>  
>>  void task_join_group_stop(struct task_struct *task)
>>  {
>> -	unsigned long mask = current->jobctl & JOBCTL_STOP_SIGMASK;
>> -	struct signal_struct *sig = current->signal;
>> +	unsigned long mask = task->jobctl & JOBCTL_STOP_SIGMASK;
>> +	struct signal_struct *sig = task->signal;
>>  
>>  	if (sig->group_stop_count) {
>>  		sig->group_stop_count++;
>> -- 
>> 2.34.1
>> 
>


Thanks for the review Oleg, and you are right. Sorry for the inconvenience 


V/R


Josh Law