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: <57481134.1030404@samsung.com>
Date:	Fri, 27 May 2016 11:19:48 +0200
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	Rhyland Klein <rklein@...dia.com>,
	Jon Hunter <jonathanh@...dia.com>,
	Thierry Reding <treding@...dia.com>,
	Sebastian Reichel <sre@...nel.org>,
	David Woodhouse <dwmw2@...radead.org>,
	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>
Cc:	Stephen Warren <swarren@...dotorg.org>,
	Alexandre Courbot <gnurou@...il.com>,
	linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver

On 05/27/2016 10:37 AM, Krzysztof Kozlowski wrote:
>> And you might be completely correct, that is something that can only
>> happen specifically with the bq27xxx driver. In which case, making the
>> fix there should be the fix. I just know from the commit log (and some
>> previous work with power supply drivers) that the case of get_property
>> being called during registration has caused problems before. That's why
>> I am trying to make sure we cover the generic case if it exists. Using
>> scheduled work is common for power_supplies to regularly update their
>> status.
>>
>> As for your proposed patches for bq27xxx, I think the latest one you
>> suggested (@12:36PM EST) with the change for
>> battery_update->battery_poll as well makes the most sense for bq27xxx. I
>> would like to point out though that if we patch this, the cache won't be
>> populated for the first TEMP request, which has the same end effect as
>> the patch I proposed to power_supply_read_temp. I believe both will
>> return 0 for the temp.
>>
>> I think that patch would work just fine in place of what I suggested for
>> this specific crash.
> 
> Hello all,
> 
> Indeed I was struggling with similar issue in bq27x00_battery. The issue
> was introduced by... me :(  when moving the ownership of power supply
> structure from driver to the core. However IMHO my change exposed the
> fundamental problem with power supply.
> 
> Anyway a fix for this issue was:
> 7f1a57fdd6cb6e7b (power_supply: Fix possible NULL pointer dereference on
> early uevent)
> AFAIU, this fix no longer fixes all the issues, right?
> 
> As for the fundamental problem, the power supply core should not call
> back the driver (get_property()) until the probe ends. Even if the
> di->bat was initialized, some other fields of driver could not be set
> yet. In general, the probe did not end so we should avoid calling driver
> internal functions.
> 
> In this particular problem:
> 1. Fix for the driver (!di->bat) is okay... but it won't solve the
> problem in general.
> 2. I think the core should handle it somehow...

I was thinking about some more generic solutions for that. Few ideas:
1. Split the power_supply_register() into register + manual call to
power_supply_changed(). Each driver will have to call the
power_supply_changed() when it is ready to do it. After that call, it is
expected that driver provides everything for power supply (it can
receive callbacks).

2. Since 7f1a57fdd6cb ("power_supply: Fix possible NULL pointer
dereference on early uevent") the power_supply_changed() is called from
a deferred work. Separate thread. We can introduce (in the core only) a
mutex:
	power_supply_deferred_register_work()
	{
		psy->mutex_lock();
		power_supply_changed(psy);
		psy->mutex_unlock();
	}
and add it also to all of API:
	power_supply_get_property() {
		psy->mutex_lock();
		psy->get_property();
		psy->mutex_unlock();
	}
The changes would be limited only to the core but we will introduce
strict locking over all of the psy callbacks.

3. We can go back to previous API, leaving the allocation done by the core:
	some_drv_probe() {
		err = power_supply_register(&some_drv->psy...);
	}


I think the second solution seems to be the most self-contained and robust.

Best regards,
Krzysztof

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ