Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
MIME-Version: 1.0
References: <20230916113327.85693-1-bo.ye@mediatek.com> <a3255da4-b2af-4403-af68-3067a5fd49bf@linaro.org>
In-Reply-To: <a3255da4-b2af-4403-af68-3067a5fd49bf@linaro.org>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 12 Oct 2023 19:03:28 +0200
Message-ID: <CAJZ5v0i0Yk+juOQthy2dh89L9RdOBRHcSk43KLEGMeJszTXBVQ@mail.gmail.com>
Subject: Re: [PATCH] Subject: thermal: Fix potential race condition in suspend/resume
To:     Daniel Lezcano <daniel.lezcano@linaro.org>
Cc:     Bo Ye <bo.ye@mediatek.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Amit Kucheria <amitk@kernel.org>,
        Zhang Rui <rui.zhang@intel.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        yugang.wang@mediatek.com, yongdong.zhang@mediatek.com,
        browse.zhang@mediatek.com, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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