Re: [PATCH next] sched,freezer: prevent tasks from escaping being frozen

[Chen Ridong]

On 2025/7/4 15:57, Peter Zijlstra wrote:
> On Fri, Jul 04, 2025 at 11:11:52AM +0800, Chen Ridong wrote:
> 
> Your patches are mangled; please educate your MUA.
> 
Hi Peter,

Thank you for your review and feedback
Apologies for the formatting issues in the patch.

>> --- a/kernel/freezer.c
>> +++ b/kernel/freezer.c
>> @@ -71,19 +71,20 @@ bool __refrigerator(bool check_kthr_stop)
>>         for (;;) {
>>                 bool freeze;
>>
>> -               raw_spin_lock_irq(&current->pi_lock);
>> -               WRITE_ONCE(current->__state, TASK_FROZEN);
>> -               /* unstale saved_state so that __thaw_task() will wake
>> us up */
>> -               current->saved_state = TASK_RUNNING;
>> -               raw_spin_unlock_irq(&current->pi_lock);
>> -
>>                 spin_lock_irq(&freezer_lock);
>> -               freeze = freezing(current) && !(check_kthr_stop &&
>> kthread_should_stop());
>> +               freeze = (freezing(current) || !cgroup_thawed(current))
>> +                        && !(check_kthr_stop && kthread_should_stop());
> 
> This makes no sense to me; why can't this stay in cgroup_freezing()?
> 
> Also, can someone please fix that broken comment style there.
> 
The change relates to commit cff5f49d433f ("cgroup_freezer: cgroup_freezing: Check if not frozen"),
which modified cgroup_freezing() to verify the FROZEN flag isn't set. The freezing(p) will return
false if the cgroup is frozen.

>>                 spin_unlock_irq(&freezer_lock);
>>
>>                 if (!freeze)
>>                         break;
>>
>> +               raw_spin_lock_irq(&current->pi_lock);
>> +               WRITE_ONCE(current->__state, TASK_FROZEN);
>> +               /* unstale saved_state so that __thaw_task() will wake
>> us up */
>> +               current->saved_state = TASK_RUNNING;
>> +               raw_spin_unlock_irq(&current->pi_lock);
>> +
> 
> And I'm not quite sure I understand this hunk either. If we bail out,
> current->__state is reset to TASK_RUNNING, so what's the problem?

The issue occurs in this race scenario:

echo FROZEN > freezer.state
  freeze_cgroup()
    freeze_task()
      fake_signal_wake_up() // wakes task to freeze it

In task context:
get_signal
  try_to_freeze
    __refrigerator
      WRITE_ONCE(current->__state, TASK_FROZEN); // set TASK_FROZEN
      // race: cgroup state updates to frozen
      freezing(current) now return false
      // We bail out, the task is not frozen but it should be frozen.

I hope this explanation clarifies the issue I encountered.

Here's the corrected format patch:

diff --git a/kernel/freezer.c b/kernel/freezer.c
index 8d530d0949ff..16e98a6b497a 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -71,19 +71,20 @@ bool __refrigerator(bool check_kthr_stop)
        for (;;) {
                bool freeze;

-               raw_spin_lock_irq(&current->pi_lock);
-               WRITE_ONCE(current->__state, TASK_FROZEN);
-               /* unstale saved_state so that __thaw_task() will wake us up */
-               current->saved_state = TASK_RUNNING;
-               raw_spin_unlock_irq(&current->pi_lock);
-
                spin_lock_irq(&freezer_lock);
-               freeze = freezing(current) && !(check_kthr_stop && kthread_should_stop());
+               freeze = (freezing(current) || !cgroup_thawed(current))
+                        && !(check_kthr_stop && kthread_should_stop());
                spin_unlock_irq(&freezer_lock);

                if (!freeze)
                        break;

+               raw_spin_lock_irq(&current->pi_lock);
+               WRITE_ONCE(current->__state, TASK_FROZEN);
+               /* unstale saved_state so that __thaw_task() will wake us up */
+               current->saved_state = TASK_RUNNING;
+               raw_spin_unlock_irq(&current->pi_lock);
+
                was_frozen = true;
                schedule();
        }

Best regards,
Ridong