| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0iVPCtPjyyNeNM2uuJYZbMwJQxYEXA7=50dtB+q=6rQ6Q@mail.gmail.com>
Date: Wed, 24 Sep 2025 19:22:47 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: GuangFei Luo <luogf2025@....com>
Cc: rafael@...nel.org, dan.carpenter@...aro.org, linux-acpi@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v6] ACPI: battery: prevent sysfs_add_battery re-entry on
rapid events
On Wed, Sep 24, 2025 at 4:11 PM GuangFei Luo <luogf2025@....com> wrote:
>
> > On Wed, Sep 24, 2025 at 12:38 AM GuangFei Luo <luogf2025@....com> wrote:
> > >
> > > > On Tuesday, September 23, 2025 7:12:03 PM CEST Rafael J. Wysocki wrote:
> > > > > On Tue, Sep 23, 2025 at 6:14 PM GuangFei Luo <luogf2025@....com> wrote:
> > > > > >
> > > > > > The functions battery_hook_add_battery(), battery_hook_remove_battery(),
> > > > > > and sysfs_remove_battery() already acquire locks, so their internal
> > > > > > accesses are safe.
> > > > >
> > > > > In fact, there are two locks in use, battery->sysfs_lock and
> > > > > hook_mutex. The latter is used for managing hooks and the former is
> > > > > only used by sysfs_remove_battery(), so it only prevents that function
> > > > > from racing with another instance of itself.
> > > > >
> > > > > I would suggest using battery->sysfs_lock for protecting battery->bat
> > > > > in general.
> > > > >
> > > > > > acpi_battery_refresh() does check battery->bat, but its child
> > > > > > functions (sysfs_add_battery() and sysfs_remove_battery()) already
> > > > > > handle locking.
> > > > >
> > > > > What locking? Before the $subject patch, sysfs_add_battery() doesn't
> > > > > do any locking at all AFAICS.
> > > > >
> > > > > > In acpi_battery_notify(), battery->bat has no lock. However, the
> > > > > > check of battery->bat is at the very end of the function. During
> > > > > > earlier calls, battery->bat has already been protected by locks, so
> > > > > > re-entry will not cause issues.
> > > > >
> > > > > All of the battery->bat checks and the code depending on them need to
> > > > > go under the same lock. I'd use battery->sysfs_lock for this as
> > > > > already mentioned above.
> > > >
> > > > So my (untested) version of this fix is appended.
> > > >
> > > > Note that it explicitly prevents acpi_battery_notify() from racing with
> > > > addition/removal, PM notifications, and resume.
> > > >
> > > > ---
> > > > drivers/acpi/battery.c | 36 +++++++++++++++++++++++-------------
> > > > 1 file changed, 23 insertions(+), 13 deletions(-)
> > > >
> > > > --- a/drivers/acpi/battery.c
> > > > +++ b/drivers/acpi/battery.c
> > > > @@ -92,7 +92,7 @@ enum {
> > > >
> > > > struct acpi_battery {
> > > > struct mutex lock;
> > > > - struct mutex sysfs_lock;
> > > > + struct mutex update_lock;
> > > > struct power_supply *bat;
> > > > struct power_supply_desc bat_desc;
> > > > struct acpi_device *device;
> > > > @@ -904,15 +904,12 @@ static int sysfs_add_battery(struct acpi
> > > >
> > > > static void sysfs_remove_battery(struct acpi_battery *battery)
> > > > {
> > > > - mutex_lock(&battery->sysfs_lock);
> > > > - if (!battery->bat) {
> > > > - mutex_unlock(&battery->sysfs_lock);
> > > > + if (!battery->bat)
> > > > return;
> > > > - }
> > > > +
> > > > battery_hook_remove_battery(battery);
> > > > power_supply_unregister(battery->bat);
> > > > battery->bat = NULL;
> > > > - mutex_unlock(&battery->sysfs_lock);
> > > > }
> > > >
> > > > static void find_battery(const struct dmi_header *dm, void *private)
> > > > @@ -1072,6 +1069,9 @@ static void acpi_battery_notify(acpi_han
> > > >
> > > > if (!battery)
> > > > return;
> > > > +
> > > > + guard(mutex)(&battery->update_lock);
> > > > +
> > > > old = battery->bat;
> > > > /*
> > > > * On Acer Aspire V5-573G notifications are sometimes triggered too
> > > > @@ -1094,21 +1094,22 @@ static void acpi_battery_notify(acpi_han
> > > > }
> > > >
> > > > static int battery_notify(struct notifier_block *nb,
> > > > - unsigned long mode, void *_unused)
> > > > + unsigned long mode, void *_unused)
> > > > {
> > > > struct acpi_battery *battery = container_of(nb, struct acpi_battery,
> > > > pm_nb);
> > > > - int result;
> > > >
> > > > - switch (mode) {
> > > > - case PM_POST_HIBERNATION:
> > > > - case PM_POST_SUSPEND:
> > > > + if (mode == PM_POST_SUSPEND || mode == PM_POST_HIBERNATION) {
> > > > + guard(mutex)(&battery->update_lock);
> > > > +
> > > > if (!acpi_battery_present(battery))
> > > > return 0;
> > > >
> > > > if (battery->bat) {
> > > > acpi_battery_refresh(battery);
> > > > } else {
> > > > + int result;
> > > > +
> > > > result = acpi_battery_get_info(battery);
> > > > if (result)
> > > > return result;
> > > > @@ -1120,7 +1121,6 @@ static int battery_notify(struct notifie
> > > >
> > > > acpi_battery_init_alarm(battery);
> > > > acpi_battery_get_state(battery);
> > > > - break;
> > > > }
> > > >
> > > > return 0;
> > > > @@ -1198,6 +1198,8 @@ static int acpi_battery_update_retry(str
> > > > {
> > > > int retry, ret;
> > > >
> > > > + guard(mutex)(&battery->update_lock);
> > > > +
> > > > for (retry = 5; retry; retry--) {
> > > > ret = acpi_battery_update(battery, false);
> > > > if (!ret)
> > > > @@ -1230,7 +1232,7 @@ static int acpi_battery_add(struct acpi_
> > > > if (result)
> > > > return result;
> > > >
> > > > - result = devm_mutex_init(&device->dev, &battery->sysfs_lock);
> > > > + result = devm_mutex_init(&device->dev, &battery->update_lock);
> > > > if (result)
> > > > return result;
> > > >
> > > > @@ -1262,6 +1264,8 @@ fail_pm:
> > > > device_init_wakeup(&device->dev, 0);
> > > > unregister_pm_notifier(&battery->pm_nb);
> > > > fail:
> > > > + guard(mutex)(&battery->update_lock);
> > > > +
> > > > sysfs_remove_battery(battery);
> > > >
> > > > return result;
> > > > @@ -1281,6 +1285,9 @@ static void acpi_battery_remove(struct a
> > > >
> > > > device_init_wakeup(&device->dev, 0);
> > > > unregister_pm_notifier(&battery->pm_nb);
> > > > +
> > > > + guard(mutex)(&battery->update_lock);
> > > > +
> > > > sysfs_remove_battery(battery);
> > > > }
> > > >
> > > > @@ -1297,6 +1304,9 @@ static int acpi_battery_resume(struct de
> > > > return -EINVAL;
> > > >
> > > > battery->update_time = 0;
> > > > +
> > > > + guard(mutex)(&battery->update_lock);
> > > > +
> > > > acpi_battery_update(battery, true);
> > > > return 0;
> > > > }
> > >
> > > Thanks for the detailed explanation and the updated version of the fix.
> > >
> > > I will test your suggested changes on my platform.
> > > After verification, I will send a v7 based on your suggestion.
> >
> > Please just verify and I'll add a changelog and subject to the patch
> > and submit it.
> >
> > Thanks!
>
> I have tested your updated patch on my laptop with battery hot-plug scenarios.
> Everything looks normal and I did not observe any issues.
Thanks for the confirmation!
Powered by blists - more mailing lists