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

[Oleg Nesterov <oleg@redhat.com>]

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
>