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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0e4f8499-97b1-49a7-b5af-11203cf625d7@wanadoo.fr>
Date: Thu, 25 Jul 2024 23:34:14 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: vipulkumar.samar@...com, viresh.kumar@...aro.org,
 linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
 kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 1/2] Input: spear-keyboard - Fix a double put in
 spear_kbd_remove()

Le 25/07/2024 à 22:52, Dmitry Torokhov a écrit :
> Hi Christophe,
> 
> On Thu, Jul 25, 2024 at 10:46:49PM +0200, Christophe JAILLET wrote:
>> The 'input_dev' is a managed resource allocated with
>> devm_input_allocate_device(), so there is no need to call
>> input_unregister_device() in the remove function.
>>
>> In fact, this call was correctly removed in commit 6102752eb354 ("Input:
>> spear-keyboard - switch to using managed resources"), but silently
>> re-introduced later in the commit in Fixes.
> 
> This change is incorrect as it leads to an active and enabled clock
> being unprepared to early. We need to unregister input device which in
> turn will call spear_kbd_close() if needed which will disable the clock
> in question. Only after that we can unprepare it.
> 
> There is also no double put as input core will recognize that input
> device was unregistered explicitly and will not attempt to unregister it
> 2nd time through devm:

Got it.

Thanks for the review and the detailed explanation.
Sorry for the noise.

I'll resend as asked in patch 2/2, if saving some lines of code makes 
enough sense for you.
But as said in the cover letter, if there is no issue, I'm not sure it 
worth the time for an old driver.

CJ

> 
> void input_unregister_device(struct input_dev *dev)
> {
> 	if (dev->devres_managed) {
> 		WARN_ON(devres_destroy(dev->dev.parent,
> 					devm_input_device_unregister,
> 					devm_input_device_match,
> 					dev));
> 		__input_unregister_device(dev);
> 		/*
> 		 * We do not do input_put_device() here because it will be done
> 		 * when 2nd devres fires up.
> 		 */
> 	} else {
> 		__input_unregister_device(dev);
> 		input_put_device(dev);
> 	}
> }
> 
>>
>> Fixes: 9336648978c2 ("Input: spear-keyboard - add clk_{un}prepare() support")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>> ---
>> Compile tested-only
>> ---
>>   drivers/input/keyboard/spear-keyboard.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
>> index 557d00a667ce..5d9fc8dc9433 100644
>> --- a/drivers/input/keyboard/spear-keyboard.c
>> +++ b/drivers/input/keyboard/spear-keyboard.c
>> @@ -276,7 +276,6 @@ static void spear_kbd_remove(struct platform_device *pdev)
>>   {
>>   	struct spear_kbd *kbd = platform_get_drvdata(pdev);
>>   
>> -	input_unregister_device(kbd->input);
>>   	clk_unprepare(kbd->clk);
>>   }
>>   
>> -- 
>> 2.45.2
>>
> 
> Thanks.
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ