linux-kernel - Re: [PATCH v4] power_supply: power_supply_read_temp only if use

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]

Date:	Tue, 14 Jun 2016 13:42:09 -0400
From:	Rhyland Klein <rklein@...dia.com>
To:	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Sebastian Reichel <sre@...nel.org>,
	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
	"David Woodhouse" <dwmw2@...radead.org>
CC:	<linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	Jon Hunter <jonathanh@...dia.com>, <stable@...r.kernel.org>
Subject: Re: [PATCH v4] power_supply: power_supply_read_temp only if use_cnt >
 0

On 6/10/2016 5:43 AM, Krzysztof Kozlowski wrote:
> On 06/09/2016 11:28 PM, Rhyland Klein wrote:
>> Change power_supply_read_temp() to use power_supply_get_property()
>> so that it will check the use_cnt and ensure it is > 0. The use_cnt
>> will be incremented at the end of __power_supply_register, so this
>> will block to case where get_property can be called before the supply
>> is fully registered. This fixes the issue show in the stack below:
>>
>> [    1.452598] power_supply_read_temp+0x78/0x80
>> [    1.458680] thermal_zone_get_temp+0x5c/0x11c
>> [    1.464765] thermal_zone_device_update+0x34/0xb4
>> [    1.471195] thermal_zone_device_register+0x87c/0x8cc
>> [    1.477974] __power_supply_register+0x364/0x424
>> [    1.484317] power_supply_register_no_ws+0x10/0x18
>> [    1.490833] bq27xxx_battery_setup+0x10c/0x164
>> [    1.497003] bq27xxx_battery_i2c_probe+0xd0/0x1b0
>> [    1.503435] i2c_device_probe+0x174/0x240
>> [    1.509172] driver_probe_device+0x1fc/0x29c
>> [    1.515167] __driver_attach+0xa4/0xa8
>> [    1.520643] bus_for_each_dev+0x58/0x98
>> [    1.526204] driver_attach+0x20/0x28
>> [    1.531505] bus_add_driver+0x1c8/0x22c
>> [    1.537067] driver_register+0x68/0x108
>> [    1.542630] i2c_register_driver+0x38/0x7c
>> [    1.548457] bq27xxx_battery_i2c_driver_init+0x18/0x20
>> [    1.555321] do_one_initcall+0x38/0x12c
>> [    1.560886] kernel_init_freeable+0x148/0x1ec
>> [    1.566972] kernel_init+0x10/0xfc
>> [    1.572101] ret_from_fork+0x10/0x40
>>
>> Also make the same change to ps_get_max_charge_cntl_limit() and
>> ps_get_cur_chrage_cntl_limit() to be safe. Lastly, change the return
>> value of power_supply_get_property() to -EAGAIN from -ENODEV if
>> use_cnt <= 0.
>>
>> Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core")
>> Cc: stable@...r.kernel.org
>> Signed-off-by: Rhyland Klein <rklein@...dia.com>
>> ---
>> v4:
>>  - Fixed return check conditions
>>  - Removed change to power_supply_get_property return code
>>  - removed whitespace change
>>
>> v3:
>>  - Changed calls to ->get_property() to use common
>>    power_supply_get_property()
>>  - reworded description, added "Fixes" line
>>  - Changed return value of power_supply_get_property() to -EAGAIN
>>
>> v2:
>>  - Added cc stable
>>  - changed return to -EAGAIN in case of use_cnt < 1
>>  - Removed WARNING
>>  - return value check added in additional patch:
>>    https://patchwork.kernel.org/patch/9158805/
>>
>>  drivers/power/power_supply_core.c | 27 ++++++++++++++++-----------
>>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> Looks okay:
> 
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
> 
> Best regards,
> Krzysztof
> 
> 

I sent another patch which is a follow up to this one, which changes the
return value of power_supply_get_property() to return -EAGAIN if it is
called during __power_supply_register(). Turns out the thermal framework
(at least) will treat EAGAIN differently and since we were on the fence
about what to return, it made sense to go ahead and do that to make
everyone happy.

https://lkml.org/lkml/2016/6/13/943

-rhyland


-- 
nvpublic