| Message ID | 20210125190909.4384-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | ACPI: platform-profile: Fix possible deadlock in platform_profile_remove() | expand |
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 >
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;
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(-)