Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <13cd5524-0d40-4f07-b542-002b79b37533@salutedevices.com>
Date:   Sat, 25 Nov 2023 03:47:41 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 0/8] devm_led_classdev_register() usage problem
Content-Language: en-US
To:     Andy Shevchenko <andriy.shevchenko@intel.com>
CC:     <pavel@ucw.cz>, <lee@kernel.org>, <vadimp@nvidia.com>,
        <mpe@ellerman.id.au>, <npiggin@gmail.com>,
        <christophe.leroy@csgroup.eu>, <linux-leds@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linuxppc-dev@lists.ozlabs.org>,
        "jic23@kernel.org" <jic23@kernel.org>, <kernel@salutedevices.com>
References: <20231025130737.2015468-1-gnstark@salutedevices.com>
 <ZWDBOfpsC5AVT8bX@smile.fi.intel.com>
From:   George Stark <gnstark@salutedevices.com>
In-Reply-To: <ZWDBOfpsC5AVT8bX@smile.fi.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello Andy

Thanks for the review.

On 11/24/23 18:28, Andy Shevchenko wrote:
> On Wed, Oct 25, 2023 at 04:07:29PM +0300, George Stark wrote:
>> Lots of drivers use devm_led_classdev_register() to register their led objects
>> and let the kernel free those leds at the driver's remove stage.
>> It can lead to a problem due to led_classdev_unregister()
>> implementation calls led_set_brightness() to turn off the led.
>> led_set_brightness() may call one of the module's brightness_set callbacks.
>> If that callback uses module's resources allocated without using devm funcs()
>> then those resources will be already freed at module's remove() callback and
>> we may have use-after-free situation.
>>
>> Here is an example:
>>
>> module_probe()
>> {
>>      devm_led_classdev_register(module_brightness_set_cb);
>>      mutex_init(&mutex);
>> }
>>
>> module_brightness_set_cb()
>> {
>>      mutex_lock(&mutex);
>>      do_set_brightness();
>>      mutex_unlock(&mutex);
>> }
>>
>> module_remove()
>> {
>>      mutex_destroy(&mutex);
>> }
>>
>> at rmmod:
>> module_remove()
>>      ->mutex_destroy(&mutex);
>> devres_release_all()
>>      ->led_classdev_unregister();
>>          ->led_set_brightness();
>>              ->module_brightness_set_cb();
>>                   ->mutex_lock(&mutex);  /* use-after-free */
>>
>> I think it's an architectural issue and should be discussed thoroughly.
>> Some thoughts about fixing it as a start:
>> 1) drivers can use devm_led_classdev_unregister() to explicitly free leds before
>> dependend resources are freed. devm_led_classdev_register() remains being useful
>> to simplify probe implementation.
>> As a proof of concept I examined all drivers from drivers/leds and prepared
>> patches where it's needed. Sometimes it was not as clean as just calling
>> devm_led_classdev_unregister() because several drivers do not track
>> their leds object at all - they can call devm_led_classdev_register() and drop the
>> returned pointer. In that case I used devres group API.
>>
>> Drivers outside drivers/leds should be checked too after discussion.
>>
>> 2) remove led_set_brightness from led_classdev_unregister() and force the drivers
>> to turn leds off at shutdown. May be add check that led's brightness is 0
>> at led_classdev_unregister() and put a warning to dmesg if it's not.
>> Actually in many cases it doesn't really need to turn off the leds manually one-by-one
>> if driver shutdowns whole led controller. For the last case to disable the warning
>> new flag can be brought in e.g LED_AUTO_OFF_AT_SHUTDOWN (similar to LED_RETAIN_AT_SHUTDOWN).
> 
> NAK.
> 
> Just fix the drivers by wrapping mutex_destroy() into devm, There are many
> doing so. You may be brave enough to introduce devm_mutex_init() somewhere
> in include/linux/device*
> 

Just one thing about mutex_destroy(). It seems like there's no single 
opinion on should it be called in 100% cases e.g. in remove() paths.
For example in iio subsystem Jonathan suggests it can be dropped in 
simple cases: https://www.spinics.net/lists/linux-iio/msg73423.html

So the question is can we just drop mutex_destroy() in module's remove() 
callback here if that mutex is needed for devm subsequent callbacks?

-- 
Best regards
George