Subject: thermal: Fix potential race condition in suspend/resume

[PATCH] Subject: thermal: Fix potential race condition in suspend/resume

Posted by Bo Ye 1 year ago

From: "yugang.wang" <yugang.wang@mediatek.com>

Body:
This patch fixes a race condition during system resume. It occurs if
the system is exiting a suspend state and a user is trying to
register/unregister a thermal zone concurrently. The root cause is
that both actions access the `thermal_tz_list`.

In detail:

1. At PM_POST_SUSPEND during the resume, the system reads all thermal
   zones in `thermal_tz_list`, then resets and updates their
   temperatures.
2. When registering/unregistering a thermal zone, the
   `thermal_tz_list` gets manipulated.

These two actions might occur concurrently, causing a race condition.
To solve this issue, we introduce a mutex lock to protect
`thermal_tz_list` from being modified while it's being read and
updated during the resume from suspend.

Kernel oops excerpt related to this fix:

[ 5201.869845] [T316822] pc: [0xffffffeb7d4876f0] mutex_lock+0x34/0x170
[ 5201.869856] [T316822] lr: [0xffffffeb7ca98a84] thermal_pm_notify+0xd4/0x26c
[... cut for brevity ...]
[ 5201.871061] [T316822]  suspend_prepare+0x150/0x470
[ 5201.871067] [T316822]  enter_state+0x84/0x6f4
[ 5201.871076] [T316822]  state_store+0x15c/0x1e8

Change-Id: Ifdbdecba17093f91eab7e36ce04b46d311ca6568
Signed-off-by: yugang.wang <yugang.wang@mediatek.com>
Signed-off-by: Bo Ye <bo.ye@mediatek.com>
---
 drivers/thermal/thermal_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 8717a3343512..a7a18ed57b6d 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1529,12 +1529,14 @@ static int thermal_pm_notify(struct notifier_block *nb,
 	case PM_POST_HIBERNATION:
 	case PM_POST_RESTORE:
 	case PM_POST_SUSPEND:
+		mutex_lock(&thermal_list_lock);
 		atomic_set(&in_suspend, 0);
 		list_for_each_entry(tz, &thermal_tz_list, node) {
 			thermal_zone_device_init(tz);
 			thermal_zone_device_update(tz,
 						   THERMAL_EVENT_UNSPECIFIED);
 		}
+		mutex_unlock(&thermal_list_lock);
 		break;
 	default:
 		break;
-- 
2.17.0

Re: [PATCH] Subject: thermal: Fix potential race condition in suspend/resume

Posted by Daniel Lezcano 11 months, 1 week ago

On 16/09/2023 13:33, Bo Ye wrote:
> From: "yugang.wang" <yugang.wang@mediatek.com>
> 
> Body:
> This patch fixes a race condition during system resume. It occurs if
> the system is exiting a suspend state and a user is trying to
> register/unregister a thermal zone concurrently. The root cause is
> that both actions access the `thermal_tz_list`.

I'm not sure the tasks are already thawed during POST_RESTORE, so no 
user can unload a driver and then reaching the race window.

Is that an observed issue?


> In detail:
> 
> 1. At PM_POST_SUSPEND during the resume, the system reads all thermal
>     zones in `thermal_tz_list`, then resets and updates their
>     temperatures.
> 2. When registering/unregistering a thermal zone, the
>     `thermal_tz_list` gets manipulated.
> 
> These two actions might occur concurrently, causing a race condition.
> To solve this issue, we introduce a mutex lock to protect
> `thermal_tz_list` from being modified while it's being read and
> updated during the resume from suspend.
> 
> Kernel oops excerpt related to this fix:
> 
> [ 5201.869845] [T316822] pc: [0xffffffeb7d4876f0] mutex_lock+0x34/0x170
> [ 5201.869856] [T316822] lr: [0xffffffeb7ca98a84] thermal_pm_notify+0xd4/0x26c
> [... cut for brevity ...]
> [ 5201.871061] [T316822]  suspend_prepare+0x150/0x470
> [ 5201.871067] [T316822]  enter_state+0x84/0x6f4
> [ 5201.871076] [T316822]  state_store+0x15c/0x1e8
> 
> Change-Id: Ifdbdecba17093f91eab7e36ce04b46d311ca6568
> Signed-off-by: yugang.wang <yugang.wang@mediatek.com>
> Signed-off-by: Bo Ye <bo.ye@mediatek.com>
> ---
>   drivers/thermal/thermal_core.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 8717a3343512..a7a18ed57b6d 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1529,12 +1529,14 @@ static int thermal_pm_notify(struct notifier_block *nb,
>   	case PM_POST_HIBERNATION:
>   	case PM_POST_RESTORE:
>   	case PM_POST_SUSPEND:
> +		mutex_lock(&thermal_list_lock);
>   		atomic_set(&in_suspend, 0);
>   		list_for_each_entry(tz, &thermal_tz_list, node) {
>   			thermal_zone_device_init(tz);
>   			thermal_zone_device_update(tz,
>   						   THERMAL_EVENT_UNSPECIFIED);
>   		}
> +		mutex_unlock(&thermal_list_lock);
>   		break;
>   	default:
>   		break;

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] Subject: thermal: Fix potential race condition in suspend/resume

Posted by Rafael J. Wysocki 11 months, 1 week ago

On Thu, Oct 12, 2023 at 5:39 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 16/09/2023 13:33, Bo Ye wrote:
> > From: "yugang.wang" <yugang.wang@mediatek.com>
> >
> > Body:
> > This patch fixes a race condition during system resume. It occurs if
> > the system is exiting a suspend state and a user is trying to
> > register/unregister a thermal zone concurrently. The root cause is
> > that both actions access the `thermal_tz_list`.
>
> I'm not sure the tasks are already thawed during POST_RESTORE, so no
> user can unload a driver and then reaching the race window.

Yes, they are.

> Is that an observed issue?

Good question, but the patch looks correct to me.

> > In detail:
> >
> > 1. At PM_POST_SUSPEND during the resume, the system reads all thermal
> >     zones in `thermal_tz_list`, then resets and updates their
> >     temperatures.
> > 2. When registering/unregistering a thermal zone, the
> >     `thermal_tz_list` gets manipulated.
> >
> > These two actions might occur concurrently, causing a race condition.
> > To solve this issue, we introduce a mutex lock to protect
> > `thermal_tz_list` from being modified while it's being read and
> > updated during the resume from suspend.
> >
> > Kernel oops excerpt related to this fix:
> >
> > [ 5201.869845] [T316822] pc: [0xffffffeb7d4876f0] mutex_lock+0x34/0x170
> > [ 5201.869856] [T316822] lr: [0xffffffeb7ca98a84] thermal_pm_notify+0xd4/0x26c
> > [... cut for brevity ...]
> > [ 5201.871061] [T316822]  suspend_prepare+0x150/0x470
> > [ 5201.871067] [T316822]  enter_state+0x84/0x6f4
> > [ 5201.871076] [T316822]  state_store+0x15c/0x1e8
> >
> > Change-Id: Ifdbdecba17093f91eab7e36ce04b46d311ca6568
> > Signed-off-by: yugang.wang <yugang.wang@mediatek.com>
> > Signed-off-by: Bo Ye <bo.ye@mediatek.com>
> > ---
> >   drivers/thermal/thermal_core.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> > index 8717a3343512..a7a18ed57b6d 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -1529,12 +1529,14 @@ static int thermal_pm_notify(struct notifier_block *nb,
> >       case PM_POST_HIBERNATION:
> >       case PM_POST_RESTORE:
> >       case PM_POST_SUSPEND:
> > +             mutex_lock(&thermal_list_lock);
> >               atomic_set(&in_suspend, 0);
> >               list_for_each_entry(tz, &thermal_tz_list, node) {
> >                       thermal_zone_device_init(tz);
> >                       thermal_zone_device_update(tz,
> >                                                  THERMAL_EVENT_UNSPECIFIED);
> >               }
> > +             mutex_unlock(&thermal_list_lock);
> >               break;
> >       default:
> >               break;
>
> --

Re: [PATCH] thermal: Fix potential race condition in suspend/resume

Posted by Bo Ye (叶波) 11 months, 1 week ago

On Sat, 2023-09-16 at 19:33 +0800, Bo Ye wrote:

Correct mail title format: remove "Subject:" from mail title.

> From: "yugang.wang" <yugang.wang@mediatek.com>
> 
> Body:
> This patch fixes a race condition during system resume. It occurs if
> the system is exiting a suspend state and a user is trying to
> register/unregister a thermal zone concurrently. The root cause is
> that both actions access the `thermal_tz_list`.
> 
> In detail:
> 
> 1. At PM_POST_SUSPEND during the resume, the system reads all thermal
>    zones in `thermal_tz_list`, then resets and updates their
>    temperatures.
> 2. When registering/unregistering a thermal zone, the
>    `thermal_tz_list` gets manipulated.
> 
> These two actions might occur concurrently, causing a race condition.
> To solve this issue, we introduce a mutex lock to protect
> `thermal_tz_list` from being modified while it's being read and
> updated during the resume from suspend.
> 
> Kernel oops excerpt related to this fix:
> 
> [ 5201.869845] [T316822] pc: [0xffffffeb7d4876f0]
> mutex_lock+0x34/0x170
> [ 5201.869856] [T316822] lr: [0xffffffeb7ca98a84]
> thermal_pm_notify+0xd4/0x26c
> [... cut for brevity ...]
> [ 5201.871061] [T316822]  suspend_prepare+0x150/0x470
> [ 5201.871067] [T316822]  enter_state+0x84/0x6f4
> [ 5201.871076] [T316822]  state_store+0x15c/0x1e8
> 
> Change-Id: Ifdbdecba17093f91eab7e36ce04b46d311ca6568
> Signed-off-by: yugang.wang <yugang.wang@mediatek.com>
> Signed-off-by: Bo Ye <bo.ye@mediatek.com>
> ---
>  drivers/thermal/thermal_core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c
> index 8717a3343512..a7a18ed57b6d 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1529,12 +1529,14 @@ static int thermal_pm_notify(struct
> notifier_block *nb,
>  	case PM_POST_HIBERNATION:
>  	case PM_POST_RESTORE:
>  	case PM_POST_SUSPEND:
> +		mutex_lock(&thermal_list_lock);
>  		atomic_set(&in_suspend, 0);
>  		list_for_each_entry(tz, &thermal_tz_list, node) {
>  			thermal_zone_device_init(tz);
>  			thermal_zone_device_update(tz,
>  						   THERMAL_EVENT_UNSPEC
> IFIED);
>  		}
> +		mutex_unlock(&thermal_list_lock);
>  		break;
>  	default:
>  		break;

Re: [PATCH] thermal: Fix potential race condition in suspend/resume

Posted by Bo Ye (叶波) 11 months ago

Yes, it is observed issue.
Firstly, it needs to be clarified that this issue occurs in a real-
world environment. By analyzing the logs, we inferred that the issue
occurred just as the system was entering suspend mode, and the user was
switching the thermal policy (this action causes all thermal zones to
unregister/register). In addition, we conducted degradation tests and
also reproduced this issue. The specific method is to first switch the
thermal policy through a command, and then immediately put the system
into suspend state through another command. This method can also
reproduce the issue.

On Thu, 2023-10-12 at 07:35 +0000, Bo Ye (叶波) wrote:
> On Sat, 2023-09-16 at 19:33 +0800, Bo Ye wrote:
> 
> Correct mail title format: remove "Subject:" from mail title.
> 
> > From: "yugang.wang" <yugang.wang@mediatek.com>
> > 
> > Body:
> > This patch fixes a race condition during system resume. It occurs
> > if
> > the system is exiting a suspend state and a user is trying to
> > register/unregister a thermal zone concurrently. The root cause is
> > that both actions access the `thermal_tz_list`.
> > 
> > In detail:
> > 
> > 1. At PM_POST_SUSPEND during the resume, the system reads all
> > thermal
> >    zones in `thermal_tz_list`, then resets and updates their
> >    temperatures.
> > 2. When registering/unregistering a thermal zone, the
> >    `thermal_tz_list` gets manipulated.
> > 
> > These two actions might occur concurrently, causing a race
> > condition.
> > To solve this issue, we introduce a mutex lock to protect
> > `thermal_tz_list` from being modified while it's being read and
> > updated during the resume from suspend.
> > 
> > Kernel oops excerpt related to this fix:
> > 
> > [ 5201.869845] [T316822] pc: [0xffffffeb7d4876f0]
> > mutex_lock+0x34/0x170
> > [ 5201.869856] [T316822] lr: [0xffffffeb7ca98a84]
> > thermal_pm_notify+0xd4/0x26c
> > [... cut for brevity ...]
> > [ 5201.871061] [T316822]  suspend_prepare+0x150/0x470
> > [ 5201.871067] [T316822]  enter_state+0x84/0x6f4
> > [ 5201.871076] [T316822]  state_store+0x15c/0x1e8
> > 
> > Change-Id: Ifdbdecba17093f91eab7e36ce04b46d311ca6568
> > Signed-off-by: yugang.wang <yugang.wang@mediatek.com>
> > Signed-off-by: Bo Ye <bo.ye@mediatek.com>
> > ---
> >  drivers/thermal/thermal_core.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/thermal/thermal_core.c
> > b/drivers/thermal/thermal_core.c
> > index 8717a3343512..a7a18ed57b6d 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -1529,12 +1529,14 @@ static int thermal_pm_notify(struct
> > notifier_block *nb,
> >  	case PM_POST_HIBERNATION:
> >  	case PM_POST_RESTORE:
> >  	case PM_POST_SUSPEND:
> > +		mutex_lock(&thermal_list_lock);
> >  		atomic_set(&in_suspend, 0);
> >  		list_for_each_entry(tz, &thermal_tz_list, node) {
> >  			thermal_zone_device_init(tz);
> >  			thermal_zone_device_update(tz,
> >  						   THERMAL_EVENT_UNSPEC
> > IFIED);
> >  		}
> > +		mutex_unlock(&thermal_list_lock);
> >  		break;
> >  	default:
> >  		break;

Re: [PATCH] thermal: Fix potential race condition in suspend/resume

Posted by Rafael J. Wysocki 11 months ago

On Mon, Oct 23, 2023 at 3:20 AM Bo Ye (叶波) <Bo.Ye@mediatek.com> wrote:
>
> Yes, it is observed issue.

It does happen, so it's not just "potential" and the subject of the
patch is slightly misleading.  Please adjust it.

> Firstly, it needs to be clarified that this issue occurs in a real-
> world environment. By analyzing the logs, we inferred that the issue
> occurred just as the system was entering suspend mode, and the user was
> switching the thermal policy (this action causes all thermal zones to
> unregister/register). In addition, we conducted degradation tests and
> also reproduced this issue. The specific method is to first switch the
> thermal policy through a command, and then immediately put the system
> into suspend state through another command. This method can also
> reproduce the issue.

OK, so please add this information to the patch changelog.

> On Thu, 2023-10-12 at 07:35 +0000, Bo Ye (叶波) wrote:
> > On Sat, 2023-09-16 at 19:33 +0800, Bo Ye wrote:
> >
> > Correct mail title format: remove "Subject:" from mail title.
> >
> > > From: "yugang.wang" <yugang.wang@mediatek.com>
> > >
> > > Body:
> > > This patch fixes a race condition during system resume. It occurs
> > > if
> > > the system is exiting a suspend state and a user is trying to
> > > register/unregister a thermal zone concurrently. The root cause is
> > > that both actions access the `thermal_tz_list`.
> > >
> > > In detail:
> > >
> > > 1. At PM_POST_SUSPEND during the resume, the system reads all
> > > thermal
> > >    zones in `thermal_tz_list`, then resets and updates their
> > >    temperatures.
> > > 2. When registering/unregistering a thermal zone, the
> > >    `thermal_tz_list` gets manipulated.
> > >
> > > These two actions might occur concurrently, causing a race
> > > condition.
> > > To solve this issue, we introduce a mutex lock to protect
> > > `thermal_tz_list` from being modified while it's being read and
> > > updated during the resume from suspend.
> > >
> > > Kernel oops excerpt related to this fix:
> > >
> > > [ 5201.869845] [T316822] pc: [0xffffffeb7d4876f0]
> > > mutex_lock+0x34/0x170
> > > [ 5201.869856] [T316822] lr: [0xffffffeb7ca98a84]
> > > thermal_pm_notify+0xd4/0x26c
> > > [... cut for brevity ...]
> > > [ 5201.871061] [T316822]  suspend_prepare+0x150/0x470
> > > [ 5201.871067] [T316822]  enter_state+0x84/0x6f4
> > > [ 5201.871076] [T316822]  state_store+0x15c/0x1e8

Well, the connection between the above log snippet and the issue
addressed by the patch is rather hard to establish.  Please include
more of the oops information.

> > >
> > > Change-Id: Ifdbdecba17093f91eab7e36ce04b46d311ca6568
> > > Signed-off-by: yugang.wang <yugang.wang@mediatek.com>
> > > Signed-off-by: Bo Ye <bo.ye@mediatek.com>
> > > ---
> > >  drivers/thermal/thermal_core.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/thermal/thermal_core.c
> > > b/drivers/thermal/thermal_core.c
> > > index 8717a3343512..a7a18ed57b6d 100644
> > > --- a/drivers/thermal/thermal_core.c
> > > +++ b/drivers/thermal/thermal_core.c
> > > @@ -1529,12 +1529,14 @@ static int thermal_pm_notify(struct
> > > notifier_block *nb,
> > >     case PM_POST_HIBERNATION:
> > >     case PM_POST_RESTORE:
> > >     case PM_POST_SUSPEND:
> > > +           mutex_lock(&thermal_list_lock);
> > >             atomic_set(&in_suspend, 0);

It is not clear to me why the above statement needs to be under the lock.

> > >             list_for_each_entry(tz, &thermal_tz_list, node) {
> > >                     thermal_zone_device_init(tz);
> > >                     thermal_zone_device_update(tz,
> > >                                                THERMAL_EVENT_UNSPEC
> > > IFIED);
> > >             }
> > > +           mutex_unlock(&thermal_list_lock);
> > >             break;
> > >     default:
> > >             break;