diff mbox series

[v8,3/7] leds: rgb: leds-ktd202x: Initialize mutex earlier

Message ID 20240502211425.8678-4-hdegoede@redhat.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series KTD2026 indicator LED for X86 Xiaomi Pad2 | expand

Commit Message

Hans de Goede May 2, 2024, 9:14 p.m. UTC
The mutex must be initialized before the LED class device is registered
otherwise there is a race where it may get used before it is initialized:

 DEBUG_LOCKS_WARN_ON(lock->magic != lock)
 WARNING: CPU: 2 PID: 2045 at kernel/locking/mutex.c:587 __mutex_lock
 ...
 RIP: 0010:__mutex_lock+0x7db/0xc10
 ...
 set_brightness_delayed_set_brightness.part.0+0x17/0x60
 set_brightness_delayed+0xf1/0x100
 process_one_work+0x222/0x5a0

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/rgb/leds-ktd202x.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Andy Shevchenko May 3, 2024, 3:43 a.m. UTC | #1
On Fri, May 3, 2024 at 12:14 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The mutex must be initialized before the LED class device is registered
> otherwise there is a race where it may get used before it is initialized:
>
>  DEBUG_LOCKS_WARN_ON(lock->magic != lock)
>  WARNING: CPU: 2 PID: 2045 at kernel/locking/mutex.c:587 __mutex_lock
>  ...
>  RIP: 0010:__mutex_lock+0x7db/0xc10
>  ...
>  set_brightness_delayed_set_brightness.part.0+0x17/0x60
>  set_brightness_delayed+0xf1/0x100
>  process_one_work+0x222/0x5a0

...

> +       mutex_init(&chip->mutex);

devm_mutex_init() ?

...

There is an immutable branch (in case of this series going behind LED
subsystem):
https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/log/?h=ib-leds-locking-6.10
Hans de Goede May 3, 2024, 6:53 a.m. UTC | #2
Hi,

On 5/3/24 5:43 AM, Andy Shevchenko wrote:
> On Fri, May 3, 2024 at 12:14 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> The mutex must be initialized before the LED class device is registered
>> otherwise there is a race where it may get used before it is initialized:
>>
>>  DEBUG_LOCKS_WARN_ON(lock->magic != lock)
>>  WARNING: CPU: 2 PID: 2045 at kernel/locking/mutex.c:587 __mutex_lock
>>  ...
>>  RIP: 0010:__mutex_lock+0x7db/0xc10
>>  ...
>>  set_brightness_delayed_set_brightness.part.0+0x17/0x60
>>  set_brightness_delayed+0xf1/0x100
>>  process_one_work+0x222/0x5a0
> 
> ...
> 
>> +       mutex_init(&chip->mutex);
> 
> devm_mutex_init() ?

That is not in Torvald's tree yet.

Regards,

Hans
Lee Jones May 3, 2024, 7:03 a.m. UTC | #3
On Fri, 03 May 2024, Hans de Goede wrote:

> Hi,
> 
> On 5/3/24 5:43 AM, Andy Shevchenko wrote:
> > On Fri, May 3, 2024 at 12:14 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> The mutex must be initialized before the LED class device is registered
> >> otherwise there is a race where it may get used before it is initialized:
> >>
> >>  DEBUG_LOCKS_WARN_ON(lock->magic != lock)
> >>  WARNING: CPU: 2 PID: 2045 at kernel/locking/mutex.c:587 __mutex_lock
> >>  ...
> >>  RIP: 0010:__mutex_lock+0x7db/0xc10
> >>  ...
> >>  set_brightness_delayed_set_brightness.part.0+0x17/0x60
> >>  set_brightness_delayed+0xf1/0x100
> >>  process_one_work+0x222/0x5a0
> > 
> > ...
> > 
> >> +       mutex_init(&chip->mutex);
> > 
> > devm_mutex_init() ?
> 
> That is not in Torvald's tree yet.

Neither is this.  :)

Since we're nearly at -rc7, I think it's safe to say you have time.
Hans de Goede May 3, 2024, 1:57 p.m. UTC | #4
Hi,

On 5/3/24 9:03 AM, Lee Jones wrote:
> On Fri, 03 May 2024, Hans de Goede wrote:
> 
>> Hi,
>>
>> On 5/3/24 5:43 AM, Andy Shevchenko wrote:
>>> On Fri, May 3, 2024 at 12:14 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> The mutex must be initialized before the LED class device is registered
>>>> otherwise there is a race where it may get used before it is initialized:
>>>>
>>>>  DEBUG_LOCKS_WARN_ON(lock->magic != lock)
>>>>  WARNING: CPU: 2 PID: 2045 at kernel/locking/mutex.c:587 __mutex_lock
>>>>  ...
>>>>  RIP: 0010:__mutex_lock+0x7db/0xc10
>>>>  ...
>>>>  set_brightness_delayed_set_brightness.part.0+0x17/0x60
>>>>  set_brightness_delayed+0xf1/0x100
>>>>  process_one_work+0x222/0x5a0
>>>
>>> ...
>>>
>>>> +       mutex_init(&chip->mutex);
>>>
>>> devm_mutex_init() ?
>>
>> That is not in Torvald's tree yet.
> 
> Neither is this.  :)
> 
> Since we're nearly at -rc7, I think it's safe to say you have time.

Ok I'll prepare a v9 with this addressed and Andy's Reviewed-by
added.

Regards,

Hans
diff mbox series

Patch

diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
index 60ca6ec34336..77247a98fe66 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -572,21 +572,25 @@  static int ktd202x_probe(struct i2c_client *client)
 		return ret;
 	}
 
+	mutex_init(&chip->mutex);
+
 	ret = ktd202x_probe_fw(chip);
 	if (ret < 0) {
 		regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
-		return ret;
+		goto destroy_mutex;
 	}
 
 	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
 	if (ret) {
 		dev_err_probe(dev, ret, "Failed to disable regulators.\n");
-		return ret;
+		goto destroy_mutex;
 	}
 
-	mutex_init(&chip->mutex);
-
 	return 0;
+
+destroy_mutex:
+	mutex_destroy(&chip->mutex);
+	return ret;
 }
 
 static void ktd202x_remove(struct i2c_client *client)