* [PATCH 0/1] ACPI: platform-profile: Fix possible deadlock in platform_profile_remove()
@ 2021-01-25 19:09 Hans de Goede
2021-01-25 19:09 ` [PATCH 1/1] " Hans de Goede
0 siblings, 1 reply; 4+ messages in thread
From: Hans de Goede @ 2021-01-25 19:09 UTC (permalink / raw)
To: Rafael J . Wysocki
Cc: Hans de Goede, Mark Pearson, Bastien Nocera, linux-acpi,
platform-driver-x86
Hi Rafael,
Sorry for throwing another patch into the mix for the acpi-platform
branch. I always build my local kernels with lockdep enabled and
while testing an unrelated acpi_thinkpad patch I did "rmmod acpi_thinkpad"
and lockdep pointed out this potential deadlock.
The fix is simple enough, although it does rely on the assumption
that drivers will only call platform_profile_remove() after they
have first successfully called platform_profile_register().
See the actual patch for more details.
Regards,
Hans
Hans de Goede (1):
ACPI: platform-profile: Fix possible deadlock in platform_profile_remove()
drivers/acpi/platform_profile.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/1] ACPI: platform-profile: Fix possible deadlock in platform_profile_remove()
2021-01-25 19:09 [PATCH 0/1] ACPI: platform-profile: Fix possible deadlock in platform_profile_remove() Hans de Goede
@ 2021-01-25 19:09 ` Hans de Goede
2021-01-25 19:39 ` [External] " Mark RH Pearson
2021-01-27 17:49 ` Rafael J. Wysocki
0 siblings, 2 replies; 4+ messages in thread
From: Hans de Goede @ 2021-01-25 19:09 UTC (permalink / raw)
To: Rafael J . Wysocki
Cc: Hans de Goede, Mark Pearson, Bastien Nocera, linux-acpi,
platform-driver-x86
After a rmmod thinkpad_acpi, lockdep pointed out this possible deadlock:
Our _show and _store sysfs attr functions get called with the kn->active
lock held for the sysfs attr and then take the profile_lock.
sysfs_remove_group() also takes the kn->active lock for the sysfs attr,
so if we call it with the profile_lock held, then we get an ABBA deadlock.
platform_profile_remove() must only be called by drivers which have
first *successfully* called platform_profile_register(). Anything else
is a driver bug. So the check for cur_profile being set before calling
sysfs_remove_group() is not necessary and it can be dropped.
It is safe to call sysfs_remove_group() without holding the profile_lock
since the attr-group group cannot be re-added until after we clear
cur_profile.
Change platform_profile_remove() to only hold the profile_lock while
clearing the cur_profile, fixing the deadlock.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/acpi/platform_profile.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 80e9df427eb8..4a59c5993bde 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -164,13 +164,9 @@ EXPORT_SYMBOL_GPL(platform_profile_register);
int platform_profile_remove(void)
{
- mutex_lock(&profile_lock);
- if (!cur_profile) {
- mutex_unlock(&profile_lock);
- return -ENODEV;
- }
-
sysfs_remove_group(acpi_kobj, &platform_profile_group);
+
+ mutex_lock(&profile_lock);
cur_profile = NULL;
mutex_unlock(&profile_lock);
return 0;
--
2.29.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [External] [PATCH 1/1] ACPI: platform-profile: Fix possible deadlock in platform_profile_remove()
2021-01-25 19:09 ` [PATCH 1/1] " Hans de Goede
@ 2021-01-25 19:39 ` Mark RH Pearson
2021-01-27 17:49 ` Rafael J. Wysocki
1 sibling, 0 replies; 4+ messages in thread
From: Mark RH Pearson @ 2021-01-25 19:39 UTC (permalink / raw)
To: Hans de Goede, Rafael J . Wysocki
Cc: Bastien Nocera, linux-acpi, platform-driver-x86
> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Monday, January 25, 2021 2:09 PM
>
> After a rmmod thinkpad_acpi, lockdep pointed out this possible deadlock:
>
> Our _show and _store sysfs attr functions get called with the kn->active
> lock held for the sysfs attr and then take the profile_lock.
> sysfs_remove_group() also takes the kn->active lock for the sysfs attr,
> so if we call it with the profile_lock held, then we get an ABBA deadlock.
>
> platform_profile_remove() must only be called by drivers which have
> first *successfully* called platform_profile_register(). Anything else
> is a driver bug. So the check for cur_profile being set before calling
> sysfs_remove_group() is not necessary and it can be dropped.
>
> It is safe to call sysfs_remove_group() without holding the profile_lock
> since the attr-group group cannot be re-added until after we clear
> cur_profile.
>
> Change platform_profile_remove() to only hold the profile_lock while
> clearing the cur_profile, fixing the deadlock.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/acpi/platform_profile.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 80e9df427eb8..4a59c5993bde 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -164,13 +164,9 @@ EXPORT_SYMBOL_GPL(platform_profile_register);
>
> int platform_profile_remove(void)
> {
> - mutex_lock(&profile_lock);
> - if (!cur_profile) {
> - mutex_unlock(&profile_lock);
> - return -ENODEV;
> - }
> -
> sysfs_remove_group(acpi_kobj, &platform_profile_group);
> +
> + mutex_lock(&profile_lock);
> cur_profile = NULL;
> mutex_unlock(&profile_lock);
> return 0;
> --
> 2.29.2
Thanks Hans - good catch! Looks good to me.
Mark
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] ACPI: platform-profile: Fix possible deadlock in platform_profile_remove()
2021-01-25 19:09 ` [PATCH 1/1] " Hans de Goede
2021-01-25 19:39 ` [External] " Mark RH Pearson
@ 2021-01-27 17:49 ` Rafael J. Wysocki
1 sibling, 0 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2021-01-27 17:49 UTC (permalink / raw)
To: Hans de Goede
Cc: Rafael J . Wysocki, Mark Pearson, Bastien Nocera,
ACPI Devel Maling List, Platform Driver
On Tue, Jan 26, 2021 at 3:10 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> After a rmmod thinkpad_acpi, lockdep pointed out this possible deadlock:
>
> Our _show and _store sysfs attr functions get called with the kn->active
> lock held for the sysfs attr and then take the profile_lock.
> sysfs_remove_group() also takes the kn->active lock for the sysfs attr,
> so if we call it with the profile_lock held, then we get an ABBA deadlock.
>
> platform_profile_remove() must only be called by drivers which have
> first *successfully* called platform_profile_register(). Anything else
> is a driver bug. So the check for cur_profile being set before calling
> sysfs_remove_group() is not necessary and it can be dropped.
>
> It is safe to call sysfs_remove_group() without holding the profile_lock
> since the attr-group group cannot be re-added until after we clear
> cur_profile.
>
> Change platform_profile_remove() to only hold the profile_lock while
> clearing the cur_profile, fixing the deadlock.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Applied on top of the previous platform-profile material, thanks!
> ---
> drivers/acpi/platform_profile.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 80e9df427eb8..4a59c5993bde 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -164,13 +164,9 @@ EXPORT_SYMBOL_GPL(platform_profile_register);
>
> int platform_profile_remove(void)
> {
> - mutex_lock(&profile_lock);
> - if (!cur_profile) {
> - mutex_unlock(&profile_lock);
> - return -ENODEV;
> - }
> -
> sysfs_remove_group(acpi_kobj, &platform_profile_group);
> +
> + mutex_lock(&profile_lock);
> cur_profile = NULL;
> mutex_unlock(&profile_lock);
> return 0;
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-27 17:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-25 19:09 [PATCH 0/1] ACPI: platform-profile: Fix possible deadlock in platform_profile_remove() Hans de Goede
2021-01-25 19:09 ` [PATCH 1/1] " Hans de Goede
2021-01-25 19:39 ` [External] " Mark RH Pearson
2021-01-27 17:49 ` Rafael J. Wysocki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.