task_join_group_stop() takes a task_struct parameter but reads jobctl
and signal from current instead.  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().

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

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
>

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