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

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

[Chen Ridong]

From: Chen Ridong <chenridong@huawei.com>

The commit cff5f49d433f ("cgroup_freezer: cgroup_freezing: Check if not
frozen") modified the cgroup_freezing() logic to also verify that the
FROZEN flag is not set, which affects the return value of the freezing()
function.

In __refrigerator(), the FROZEN flag is set before checking whether the
task should be frozen. This creates a race condition where:
1. The task's FROZEN flag is set.
2. The cgroup freezer state changes to FROZEN (Can be triggered by reading
   freezer.state).
3. freezing() is called and returns false.

As a result, the task may escape being frozen when it should be.

To fix this, move the setting of the FROZEN flag to occur just before
schedule(). This ensures the flag is only set when we're certain the
task must be switched out.

Fixes: cff5f49d433f ("cgroup_freezer: cgroup_freezing: Check if not frozen")
Reported-by: Zhong Jiawei<zhongjiawei1@huawei.com>
Signed-off-by: Chen Ridong <chenridong@huawei.com>
---
 kernel/freezer.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index 8d530d0949ff..89edd7550d27 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -71,12 +71,6 @@ 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());
 		spin_unlock_irq(&freezer_lock);
@@ -84,6 +78,12 @@ bool __refrigerator(bool check_kthr_stop)
 		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();
 	}
-- 
2.34.1

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

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 741 bytes --]

Hello Ridong.

On Thu, Jul 03, 2025 at 01:34:27PM +0000, Chen Ridong <chenridong@huaweicloud.com> wrote:
> 2. The cgroup freezer state changes to FROZEN (Can be triggered by reading
>    freezer.state).
/o\

> 3. freezing() is called and returns false.

I can see how this can happen because freezer_lock != freezer_mutex.

> As a result, the task may escape being frozen when it should be.
> 
> To fix this, move the setting of the FROZEN flag to occur just before
> schedule(). This ensures the flag is only set when we're certain the
> task must be switched out.

Is it sufficient? (If the task is spuriously woken up, the next
iteration in that refrigerator loop would be subject to same race, no?)

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Chen Ridong]

On 2025/7/4 1:01, Michal Koutný wrote:
> Hello Ridong.
> 
> On Thu, Jul 03, 2025 at 01:34:27PM +0000, Chen Ridong <chenridong@huaweicloud.com> wrote:
>> 2. The cgroup freezer state changes to FROZEN (Can be triggered by reading
>>    freezer.state).
> /o\
> 
>> 3. freezing() is called and returns false.
> 
> I can see how this can happen because freezer_lock != freezer_mutex.
> 
>> As a result, the task may escape being frozen when it should be.
>>
>> To fix this, move the setting of the FROZEN flag to occur just before
>> schedule(). This ensures the flag is only set when we're certain the
>> task must be switched out.
> 
> Is it sufficient? (If the task is spuriously woken up, the next
> iteration in that refrigerator loop would be subject to same race, no?)
> 
> Thanks,
> Michal

Hi, Michal:

Regarding your question: Did you mean that the task was frozen, received
another signal to wake up, but should have remained frozen instead of
entering the running state?

For this scenario, the solution I've found is that the task can only
break out of the frozen state when its cgroup is thawed. The code
modification would look like the following, and we'll need to add the
cgroup_thawed(p) function:

--- 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);
+

I'd welcome any suggestions for better solutions to this problem.

Best regards,
Ridong

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

[Chen Ridong]

On 2025/7/4 11:02, Chen Ridong wrote:
> 
> 
> On 2025/7/4 1:01, Michal Koutný wrote:
>> Hello Ridong.
>>
>> On Thu, Jul 03, 2025 at 01:34:27PM +0000, Chen Ridong <chenridong@huaweicloud.com> wrote:
>>> 2. The cgroup freezer state changes to FROZEN (Can be triggered by reading
>>>    freezer.state).
>> /o\
>>
>>> 3. freezing() is called and returns false.
>>
>> I can see how this can happen because freezer_lock != freezer_mutex.
>>
>>> As a result, the task may escape being frozen when it should be.
>>>
>>> To fix this, move the setting of the FROZEN flag to occur just before
>>> schedule(). This ensures the flag is only set when we're certain the
>>> task must be switched out.
>>
>> Is it sufficient? (If the task is spuriously woken up, the next
>> iteration in that refrigerator loop would be subject to same race, no?)
>>
>> Thanks,
>> Michal
> 
> Hi, Michal:
> 
> Regarding your question: Did you mean that the task was frozen, received
> another signal to wake up, but should have remained frozen instead of
> entering the running state?
> 
> For this scenario, the solution I've found is that the task can only
> break out of the frozen state when its cgroup is thawed. The code
> modification would look like the following, and we'll need to add the
> cgroup_thawed(p) function:
> 

Sorry, the code should look like:

--- 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

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

[Peter Zijlstra]

On Fri, Jul 04, 2025 at 11:11:52AM +0800, Chen Ridong wrote:

Your patches are mangled; please educate your MUA.

> --- 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.

>                 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?

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

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

[Chen Ridong]

On 2025/7/4 18:25, Chen Ridong wrote:
> 
> 
> 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.
> 

Hi, Peter, Tim

I was looking at the WARN_ON_ONCE(freezing(p)) check in __thaw_task and started wondering: since we
already have !frozen(p) check, is this warning still needed? If we can remove it, maybe reverting
commit cff5f49d433f ("cgroup_freezer: cgroup_freezing: Check if not frozen") would be a better approach.

What do you think?

Best regards,
Ridong

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

[Peter Zijlstra]

On Mon, Jul 07, 2025 at 12:02:47PM +0800, Chen Ridong wrote:

> >> 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

I suppose this is me not quite knowing how this cgroup freezer works;
how does it race? what code marks the task 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.
> > 
> 
> Hi, Peter, Tim
> 
> I was looking at the WARN_ON_ONCE(freezing(p)) check in __thaw_task
> and started wondering: since we already have !frozen(p) check, is this
> warning still needed? If we can remove it, maybe reverting commit
> cff5f49d433f ("cgroup_freezer: cgroup_freezing: Check if not frozen")
> would be a better approach.

I suppose that is possible; modern sensibilities require we write that
function something like so:

void __thaw_task(struct task_struct *p)
{
	guard(spinlock_irqsave)(&freezer_lock);
	if (frozen(p) && !task_call_func(p, __restore_freezer_state, NULL))
		wake_up_state(p, TASK_FROZEN);
}

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

[Chen Ridong]

On 2025/7/7 18:10, Peter Zijlstra wrote:
> On Mon, Jul 07, 2025 at 12:02:47PM +0800, Chen Ridong wrote:
> 
>>>> 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
> 
> I suppose this is me not quite knowing how this cgroup freezer works;
> how does it race? what code marks the task frozen?
> 

Hi.Peter,
Thank you for your reply.

Below is the race condition scenario:

get_signal				read freezer.state
try_to_freeze
__refrigerator				freezer_read
					update_if_frozen
WRITE_ONCE(current->__state, TASK_FROZEN);			
					// The task is set to frozen(now, frozen(task) == ture).
					// we suppose other tasks are all frozen.
					// set cgroup frozen when all tasks are frozen
					freezer->state |= CGROUP_FROZEN;
// freezing(current) will return false,
// since cgroup is frozen(not freezing)
break out
__set_current_state(TASK_RUNNING);
//bug: the task is set to running, but it should be frozen.

Please let me know if you have any questions.

>>>       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.
>>>
>>
>> Hi, Peter, Tim
>>
>> I was looking at the WARN_ON_ONCE(freezing(p)) check in __thaw_task
>> and started wondering: since we already have !frozen(p) check, is this
>> warning still needed? If we can remove it, maybe reverting commit
>> cff5f49d433f ("cgroup_freezer: cgroup_freezing: Check if not frozen")
>> would be a better approach.
> 
> I suppose that is possible; modern sensibilities require we write that
> function something like so:
> 
> void __thaw_task(struct task_struct *p)
> {
> 	guard(spinlock_irqsave)(&freezer_lock);
> 	if (frozen(p) && !task_call_func(p, __restore_freezer_state, NULL))
> 		wake_up_state(p, TASK_FROZEN);
> }

Glad to heard that.

Best regards,
Ridong

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

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

On Fri, Jul 04, 2025 at 11:02:52AM +0800, Chen Ridong <chenridong@huaweicloud.com> wrote:
> Regarding your question: Did you mean that the task was frozen, received
> another signal to wake up, but should have remained frozen instead of
> entering the running state?

My response was a knee-jerk after seeing the move inside the loop.
Since the task is in __refrigerator(), it would've been already frozen,
so the 2nd (and more) checks should be OK and it wouldn't escape
freezing despite the concurrent reader.

A task in __refrigerator() shouldn't be woken up by a signal, no? So
your original conservative fix might have been sufficient afterall.
(A conservative fix is what I'd strive for here, given it's the legacy
cgroup freezer.)

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Chen Ridong]

On 2025/7/8 0:38, Michal Koutný wrote:
> On Fri, Jul 04, 2025 at 11:02:52AM +0800, Chen Ridong <chenridong@huaweicloud.com> wrote:
>> Regarding your question: Did you mean that the task was frozen, received
>> another signal to wake up, but should have remained frozen instead of
>> entering the running state?
> 
> My response was a knee-jerk after seeing the move inside the loop.
> Since the task is in __refrigerator(), it would've been already frozen,
> so the 2nd (and more) checks should be OK and it wouldn't escape
> freezing despite the concurrent reader.
> 
> A task in __refrigerator() shouldn't be woken up by a signal, no? So
> your original conservative fix might have been sufficient afterall.
> (A conservative fix is what I'd strive for here, given it's the legacy
> cgroup freezer.)
> 
> Thanks,
> Michal

Thank you, Michal.

try_to_freeze
  if (likely(!freezing(current))) // This guarantees cgroup is not frozen.
	return false;
  __refrigerator
    freezing(current) // The cgroup can't be frozen unless 'current' is frozen.
		      // With the original logic, 'current' cannot escape freezing.

I agree that the original conservative fix is indeed sufficient

However, I'd like to highlight a behavioral change introduced by commit cff5f49d433f
("cgroup_freezer: cgroup_freezing: Check if not frozen"). Before this change, most callers of
freezing(p) would receive true when the cgroup was frozen, whereas now they receive false.

My concern is that the state where freezing(p) returns true (i.e., the cgroup is freezing but not
yet frozen) should be transient. I'm not entirely sure whether all callers of freezing(p) (except
__thaw_task) expect or handle this state correctly. Do the callers want the intermediate freezing
state? I am not sure...

For example, patch 37fb58a7273726e59f9429c89ade5116083a213d ("cgroup,freezer: fix incomplete
freezing when attaching tasks") appears to address an issue that might also be related to commit
cff5f49d433f. This makes me wonder if other callers of freezing(p) could be affected in ways we
haven't yet identified.

Given this, I'm considering whether we should revert commit cff5f49d433f, provided we can safely
remove the WARN_ON_ONCE(freezing(p)) check in __thaw_task. I'd appreciate your thoughts on this
approach.

Best regards,
Ridong

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

[Peter Zijlstra]

On Mon, Jul 07, 2025 at 07:32:59PM +0800, Chen Ridong wrote:

> Below is the race condition scenario:
> 
> get_signal				read freezer.state
> try_to_freeze
> __refrigerator				freezer_read
> 					update_if_frozen
> WRITE_ONCE(current->__state, TASK_FROZEN);			
> 					// The task is set to frozen(now, frozen(task) == ture).
> 					// we suppose other tasks are all frozen.
> 					// set cgroup frozen when all tasks are frozen
> 					freezer->state |= CGROUP_FROZEN;

Ooh, yes, now I see. Somehow I kept missing update_if_frozen().
Sometimes reading is hard :-) Thanks!

> // freezing(current) will return false,
> // since cgroup is frozen(not freezing)
> break out
> __set_current_state(TASK_RUNNING);
> //bug: the task is set to running, but it should be frozen.

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

[Tim Van Patten]

I am (admittedly) more of a naive observer here than anything, and far
from an expert, but my
understanding matches what the code says:

* freezing(): Check if there is a request to freeze a process
* frozen(): Check if a process has been frozen

> Before this change, most callers of
> freezing(p) would receive true when the cgroup was frozen, whereas now they receive false.
>
> My concern is that the state where freezing(p) returns true (i.e., the cgroup is freezing but not
> yet frozen) should be transient. I'm not entirely sure whether all callers of freezing(p) (except
> __thaw_task) expect or handle this state correctly. Do the callers want the intermediate freezing
> state? I am not sure...

I agree that there's not a clear way to be certain what the caller's
intent was, so all we have is the code.
Based on that, my interpretation is that anyone calling freezing() is
interested in the transient state (there
was a request, but it hasn't completed yet) and any callers treating
freezing() as frozen() are incorrect.
They should be calling frozen() instead (or possibly also) based on
the documentation and naming.

> Given this, I'm considering whether we should revert commit cff5f49d433f, provided we can safely
> remove the WARN_ON_ONCE(freezing(p)) check in __thaw_task. I'd appreciate your thoughts on this
> approach.

At the end of the day my goal was to clean up the warning since it was
breaking tests, so if we remove that
as part of the revert then we're still in the same state. I disagree
that we should revert though, since the
code now matches the documentation and naming, in my opinion. However,
I'll have to defer to the experts
for the final decision, since they're ultimately responsible for any
changes here.

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

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 1258 bytes --]

On Mon, Jul 07, 2025 at 12:02:47PM +0800, Chen Ridong <chenridong@huaweicloud.com> wrote:
> I was looking at the WARN_ON_ONCE(freezing(p)) check in __thaw_task and started wondering: since we
> already have !frozen(p) check, is this warning still needed? If we can remove it, maybe reverting
> commit cff5f49d433f ("cgroup_freezer: cgroup_freezing: Check if not frozen") would be a better approach.

I think freezing(p) (the part of global freezer) and cgroup_freezing(p)
should be consistent with each other. AFAICS, the former predicate is
derived from pm_freezing and that is set all the time between
freeze_processes() and thaw_processes(), i.e. it stands for both the
transition (freezing) and goal (frozen).
With that, the warning in __thaw_task() is incorrect and the solution
might be the revert + drop the warning.

(Or transfer the logic from cff5f49d433f only to the warning like
@@ -204,7 +204,7 @@ void __thaw_task(struct task_struct *p)
        unsigned long flags;

        spin_lock_irqsave(&freezer_lock, flags);
-       if (WARN_ON_ONCE(freezing(p)))
+       if (WARN_ON_ONCE(freezing(p) && !frozen(p)))
                goto unlock;

but I'm not sure the warning buys us anything in either case.)

0.02€,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Chen Ridong]

On 2025/7/10 23:44, Michal Koutný wrote:
> On Mon, Jul 07, 2025 at 12:02:47PM +0800, Chen Ridong <chenridong@huaweicloud.com> wrote:
>> I was looking at the WARN_ON_ONCE(freezing(p)) check in __thaw_task and started wondering: since we
>> already have !frozen(p) check, is this warning still needed? If we can remove it, maybe reverting
>> commit cff5f49d433f ("cgroup_freezer: cgroup_freezing: Check if not frozen") would be a better approach.
> 
> I think freezing(p) (the part of global freezer) and cgroup_freezing(p)
> should be consistent with each other. AFAICS, the former predicate is
> derived from pm_freezing and that is set all the time between
> freeze_processes() and thaw_processes(), i.e. it stands for both the
> transition (freezing) and goal (frozen).
> With that, the warning in __thaw_task() is incorrect and the solution
> might be the revert + drop the warning.
> 

I'd suggest reverting the commit and dropping the warning. What are your thoughts on this, Peter?

Best regards,
Ridong