Re: [PATCH 2/2] kernel: rerun task_work while freezing in get_signal()

[Pavel Begunkov <asml.silence@gmail.com>]

On 7/8/24 11:42, Oleg Nesterov wrote:
> On 07/07, Pavel Begunkov wrote:
>>
>> io_uring can asynchronously add a task_work while the task is getting
>> freezed. TIF_NOTIFY_SIGNAL will prevent the task from sleeping in
>> do_freezer_trap(), and since the get_signal()'s relock loop doesn't
>> retry task_work, the task will spin there not being able to sleep
>> until the freezing is cancelled / the task is killed / etc.
>>
>> Cc: stable@vger.kernel.org
>> Link: https://github.com/systemd/systemd/issues/33626
>> Fixes: 3146cba99aa28 ("io-wq: make worker creation resilient against signals")
> 
> I don't think we should blame io_uring even if so far it is the only user
> of TWA_SIGNAL.

And it's not entirely correct even for backporting purposes,
I'll pin it to when freezing was introduced then.

> Perhaps we should change do_freezer_trap() somehow, not sure... It assumes
> that TIF_SIGPENDING is the only reason to not sleep in TASK_INTERRUPTIBLE,
> today this is not true.

Let's CC Peter Zijlstra and Tejun in case they might have
some input on that.

Link to this patch for convenience:
https://lore.kernel.org/all/1d935e9d87fd8672ef3e8a9a0db340d355ea08b4.1720368770.git.asml.silence@gmail.com/

>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -2694,6 +2694,10 @@ bool get_signal(struct ksignal *ksig)
>>   	try_to_freeze();
>>   
>>   relock:
>> +	clear_notify_signal();
>> +	if (unlikely(task_work_pending(current)))
>> +		task_work_run();
>> +
>>   	spin_lock_irq(&sighand->siglock);
> 
> Well, but can't we kill the same code at the start of get_signal() then?
> Of course, in this case get_signal() should check signal_pending(), not
> task_sigpending().

Should be fine, but I didn't want to change the
try_to_freeze() -> __refrigerator() path, which also reschedules.

> Or perhaps something like the patch below makes more sense? I dunno...

It needs a far backporting, I'd really prefer to keep it
lean and without more side effects if possible, unless
there is a strong opinion on that.

> Oleg.
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 1f9dd41c04be..e2ae85293fbb 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2676,6 +2676,7 @@ bool get_signal(struct ksignal *ksig)
>   	struct signal_struct *signal = current->signal;
>   	int signr;
>   
> +start:
>   	clear_notify_signal();
>   	if (unlikely(task_work_pending(current)))
>   		task_work_run();
> @@ -2760,10 +2761,11 @@ bool get_signal(struct ksignal *ksig)
>   			if (current->jobctl & JOBCTL_TRAP_MASK) {
>   				do_jobctl_trap();
>   				spin_unlock_irq(&sighand->siglock);
> +				goto relock;
>   			} else if (current->jobctl & JOBCTL_TRAP_FREEZE)
>   				do_freezer_trap();
> -
> -			goto relock;
> +				goto start;
> +			}
>   		}
>   
>   		/*
> 

-- 
Pavel Begunkov