[PATCH 0/2] freezer,sched: do not restore saved_state of a thawed task

[PATCH 0/2] freezer,sched: do not restore saved_state of a thawed task

[Elliot Berman]

This series applies couple fixes to commit 8f0eed4a78a8 ("freezer,sched:
Use saved_state to reduce some spurious wakeups") which was found while
testing with legacy cgroup freezer. My original testing was only with
system-wide freezer. We found that thaw_task could be called on a task
which was already frozen. Prior to commit 8f0eed4a78a8 ("freezer,sched:
Use saved_state to reduce some spurious wakeups"), this wasn't an issue
as kernel would try to wake up TASK_FROZEN, which wouldn't match the
thawed task state, and no harm done to task. After commit 8f0eed4a78a8
("freezer,sched: Use saved_state to reduce some spurious wakeups"), it
was possible to overwrite the state of thawed task.

Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
Elliot Berman (2):
      freezer,sched: do not restore saved_state of a thawed task
      freezer,sched: clean saved_state when restoring it during thaw

 kernel/freezer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
---
base-commit: 6d7e4782bcf549221b4ccfffec2cf4d1a473f1a3
change-id: 20231108-freezer-state-multiple-thaws-7a3a8d9dadb3

Best regards,
-- 
Elliot Berman <quic_eberman@quicinc.com>

[PATCH 0/2] freezer,sched: do not restore saved_state of a thawed task

[Elliot Berman]

This series applies couple fixes to commit 8f0eed4a78a8 ("freezer,sched:
Use saved_state to reduce some spurious wakeups") which was found while
testing with legacy cgroup freezer. My original testing was only with
system-wide freezer. We found that thaw_task could be called on a task
which was already frozen. Prior to commit 8f0eed4a78a8 ("freezer,sched:
Use saved_state to reduce some spurious wakeups"), this wasn't an issue
as kernel would try to wake up TASK_FROZEN, which wouldn't match the
thawed task state, and no harm done to task. After commit 8f0eed4a78a8
("freezer,sched: Use saved_state to reduce some spurious wakeups"), it
was possible to overwrite the state of thawed task.

To: Rafael J. Wysocki <rafael@kernel.org>
To: Pavel Machek <pavel@ucw.cz>
To: Ingo Molnar <mingo@kernel.org>
To: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc:  <linux-arm-msm@vger.kernel.org>
Cc: Pavan Kondeti <quic_pkondeti@quicinc.com>
Cc: Aiqun Yu (Maria) <quic_aiquny@quicinc.com>
Cc:  <linux-pm@vger.kernel.org>
Cc:  <linux-kernel@vger.kernel.org>
Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>

Originally sent to only linux-arm-msm, resending to correct authors.
- Link to v1: https://lore.kernel.org/r/20231120-freezer-state-multiple-thaws-v1-0-a4c453f50745@quicinc.com

---
Elliot Berman (2):
      freezer,sched: do not restore saved_state of a thawed task
      freezer,sched: clean saved_state when restoring it during thaw

 kernel/freezer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
---
base-commit: 6d7e4782bcf549221b4ccfffec2cf4d1a473f1a3
change-id: 20231108-freezer-state-multiple-thaws-7a3a8d9dadb3

Best regards,
-- 
Elliot Berman <quic_eberman@quicinc.com>

[PATCH 1/2] freezer,sched: do not restore saved_state of a thawed task

[Elliot Berman]

It is possible for a task to be thawed multiple times when mixing the
*legacy* cgroup freezer and system-wide freezer. To do this, freeze the
cgroup, do system-wide freeze/thaw, then thaw the cgroup. When this
happens, then a stale saved_state can be written to the task's state
and cause task to hang indefinitely. Fix this by only trying to thaw
tasks that are actually frozen.

This change also has the marginal benefit avoiding unnecessary
wake_up_state(p, TASK_FROZEN) if we know the task is already thawed.
There is not possibility of time-of-compare/time-of-use race when we skip
the wake_up_state because entering/exiting TASK_FROZEN is guarded by
freezer_lock.

Fixes: 8f0eed4a78a8 ("freezer,sched: Use saved_state to reduce some spurious wakeups")
Reviewed-by: Abhijeet Dharmapurikar <quic_adharmap@quicinc.com>
Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 kernel/freezer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index c450fa8b8b5e..759006a9a910 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -201,7 +201,7 @@ void __thaw_task(struct task_struct *p)
 	if (WARN_ON_ONCE(freezing(p)))
 		goto unlock;
 
-	if (task_call_func(p, __restore_freezer_state, NULL))
+	if (!frozen(p) || task_call_func(p, __restore_freezer_state, NULL))
 		goto unlock;
 
 	wake_up_state(p, TASK_FROZEN);

-- 
2.41.0

[PATCH 2/2] freezer,sched: clean saved_state when restoring it during thaw

[Elliot Berman]

Clean saved_state after using it during thaw. Cleaning the saved_state
allows us to avoid some unnecessary branches in ttwu_state_match.

Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 kernel/freezer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index 759006a9a910..f57aaf96b829 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -187,6 +187,7 @@ static int __restore_freezer_state(struct task_struct *p, void *arg)
 
 	if (state != TASK_RUNNING) {
 		WRITE_ONCE(p->__state, state);
+		p->saved_state = TASK_RUNNING;
 		return 1;
 	}
 

-- 
2.41.0

Re: [PATCH 0/2] freezer,sched: do not restore saved_state of a thawed task

[Aiqun(Maria) Yu]

On 11/21/2023 1:36 AM, Elliot Berman wrote:
> This series applies couple fixes to commit 8f0eed4a78a8 ("freezer,sched:
> Use saved_state to reduce some spurious wakeups") which was found while
> testing with legacy cgroup freezer. My original testing was only with
> system-wide freezer. We found that thaw_task could be called on a task
> which was already frozen. Prior to commit 8f0eed4a78a8 ("freezer,sched:
> Use saved_state to reduce some spurious wakeups"), this wasn't an issue
> as kernel would try to wake up TASK_FROZEN, which wouldn't match the
> thawed task state, and no harm done to task. After commit 8f0eed4a78a8
> ("freezer,sched: Use saved_state to reduce some spurious wakeups"), it
> was possible to overwrite the state of thawed task.
> 
> To: Rafael J. Wysocki <rafael@kernel.org>
> To: Pavel Machek <pavel@ucw.cz>
> To: Ingo Molnar <mingo@kernel.org>
> To: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc:  <linux-arm-msm@vger.kernel.org>
> Cc: Pavan Kondeti <quic_pkondeti@quicinc.com>
> Cc: Aiqun Yu (Maria) <quic_aiquny@quicinc.com>
> Cc:  <linux-pm@vger.kernel.org>
> Cc:  <linux-kernel@vger.kernel.org>
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
Shall we add Fixed tag and Cc: stable@vger.kernel.org ?
Since it is fixing a stable user thread hung issue.
> 
> Originally sent to only linux-arm-msm, resending to correct authors.
> - Link to v1: https://lore.kernel.org/r/20231120-freezer-state-multiple-thaws-v1-0-a4c453f50745@quicinc.com
> 
> ---
> Elliot Berman (2):
>        freezer,sched: do not restore saved_state of a thawed task
>        freezer,sched: clean saved_state when restoring it during thaw
> 
>   kernel/freezer.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> ---
> base-commit: 6d7e4782bcf549221b4ccfffec2cf4d1a473f1a3
> change-id: 20231108-freezer-state-multiple-thaws-7a3a8d9dadb3
> 
> Best regards,

-- 
Thx and BRs,
Aiqun(Maria) Yu

Re: [PATCH 0/2] freezer,sched: do not restore saved_state of a thawed task

[Peter Zijlstra]

On Tue, Nov 28, 2023 at 05:12:00PM +0800, Aiqun(Maria) Yu wrote:
> On 11/21/2023 1:36 AM, Elliot Berman wrote:
> > This series applies couple fixes to commit 8f0eed4a78a8 ("freezer,sched:
> > Use saved_state to reduce some spurious wakeups") which was found while
> > testing with legacy cgroup freezer. My original testing was only with
> > system-wide freezer. We found that thaw_task could be called on a task
> > which was already frozen. Prior to commit 8f0eed4a78a8 ("freezer,sched:
> > Use saved_state to reduce some spurious wakeups"), this wasn't an issue
> > as kernel would try to wake up TASK_FROZEN, which wouldn't match the
> > thawed task state, and no harm done to task. After commit 8f0eed4a78a8
> > ("freezer,sched: Use saved_state to reduce some spurious wakeups"), it
> > was possible to overwrite the state of thawed task.
> > 
> > To: Rafael J. Wysocki <rafael@kernel.org>
> > To: Pavel Machek <pavel@ucw.cz>
> > To: Ingo Molnar <mingo@kernel.org>
> > To: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Cc:  <linux-arm-msm@vger.kernel.org>
> > Cc: Pavan Kondeti <quic_pkondeti@quicinc.com>
> > Cc: Aiqun Yu (Maria) <quic_aiquny@quicinc.com>
> > Cc:  <linux-pm@vger.kernel.org>
> > Cc:  <linux-kernel@vger.kernel.org>
> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> Shall we add Fixed tag and Cc: stable@vger.kernel.org ?
> Since it is fixing a stable user thread hung issue.

If you want this routed through urgent, then yes.

Re: [PATCH 0/2] freezer,sched: do not restore saved_state of a thawed task

[Elliot Berman]

On 11/28/2023 11:31 AM, Peter Zijlstra wrote:
> On Tue, Nov 28, 2023 at 05:12:00PM +0800, Aiqun(Maria) Yu wrote:
>> On 11/21/2023 1:36 AM, Elliot Berman wrote:
>>> This series applies couple fixes to commit 8f0eed4a78a8 ("freezer,sched:
>>> Use saved_state to reduce some spurious wakeups") which was found while
>>> testing with legacy cgroup freezer. My original testing was only with
>>> system-wide freezer. We found that thaw_task could be called on a task
>>> which was already frozen. Prior to commit 8f0eed4a78a8 ("freezer,sched:
>>> Use saved_state to reduce some spurious wakeups"), this wasn't an issue
>>> as kernel would try to wake up TASK_FROZEN, which wouldn't match the
>>> thawed task state, and no harm done to task. After commit 8f0eed4a78a8
>>> ("freezer,sched: Use saved_state to reduce some spurious wakeups"), it
>>> was possible to overwrite the state of thawed task.
>>>
>>> To: Rafael J. Wysocki <rafael@kernel.org>
>>> To: Pavel Machek <pavel@ucw.cz>
>>> To: Ingo Molnar <mingo@kernel.org>
>>> To: Peter Zijlstra (Intel) <peterz@infradead.org>
>>> Cc:  <linux-arm-msm@vger.kernel.org>
>>> Cc: Pavan Kondeti <quic_pkondeti@quicinc.com>
>>> Cc: Aiqun Yu (Maria) <quic_aiquny@quicinc.com>
>>> Cc:  <linux-pm@vger.kernel.org>
>>> Cc:  <linux-kernel@vger.kernel.org>
>>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
>> Shall we add Fixed tag and Cc: stable@vger.kernel.org ?
>> Since it is fixing a stable user thread hung issue.
> 
> If you want this routed through urgent, then yes.

Fixes tag is added to https://lore.kernel.org/all/20231120-freezer-state-multiple-thaws-v1-1-f2e1dd7ce5a2@quicinc.com/

Is CC'ing stable what triggers routing through urgent?

Thanks,
Elliot

Re: [PATCH 0/2] freezer,sched: do not restore saved_state of a thawed task

[Peter Zijlstra]

On Tue, Nov 28, 2023 at 11:33:23AM -0600, Elliot Berman wrote:
> 
> 
> On 11/28/2023 11:31 AM, Peter Zijlstra wrote:
> > On Tue, Nov 28, 2023 at 05:12:00PM +0800, Aiqun(Maria) Yu wrote:
> >> On 11/21/2023 1:36 AM, Elliot Berman wrote:
> >>> This series applies couple fixes to commit 8f0eed4a78a8 ("freezer,sched:
> >>> Use saved_state to reduce some spurious wakeups") which was found while
> >>> testing with legacy cgroup freezer. My original testing was only with
> >>> system-wide freezer. We found that thaw_task could be called on a task
> >>> which was already frozen. Prior to commit 8f0eed4a78a8 ("freezer,sched:
> >>> Use saved_state to reduce some spurious wakeups"), this wasn't an issue
> >>> as kernel would try to wake up TASK_FROZEN, which wouldn't match the
> >>> thawed task state, and no harm done to task. After commit 8f0eed4a78a8
> >>> ("freezer,sched: Use saved_state to reduce some spurious wakeups"), it
> >>> was possible to overwrite the state of thawed task.
> >>>
> >>> To: Rafael J. Wysocki <rafael@kernel.org>
> >>> To: Pavel Machek <pavel@ucw.cz>
> >>> To: Ingo Molnar <mingo@kernel.org>
> >>> To: Peter Zijlstra (Intel) <peterz@infradead.org>
> >>> Cc:  <linux-arm-msm@vger.kernel.org>
> >>> Cc: Pavan Kondeti <quic_pkondeti@quicinc.com>
> >>> Cc: Aiqun Yu (Maria) <quic_aiquny@quicinc.com>
> >>> Cc:  <linux-pm@vger.kernel.org>
> >>> Cc:  <linux-kernel@vger.kernel.org>
> >>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> >> Shall we add Fixed tag and Cc: stable@vger.kernel.org ?
> >> Since it is fixing a stable user thread hung issue.
> > 
> > If you want this routed through urgent, then yes.
> 
> Fixes tag is added to https://lore.kernel.org/all/20231120-freezer-state-multiple-thaws-v1-1-f2e1dd7ce5a2@quicinc.com/
> 
> Is CC'ing stable what triggers routing through urgent?

Fixes tag should be enough, lemme go find that other copy then.. so much
email :/