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: <5447AFB2.4010701@roeck-us.net>
Date:	Wed, 22 Oct 2014 06:22:58 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	Philippe Rétornaz <philippe.retornaz@...il.com>,
	linux-kernel@...r.kernel.org
CC:	linux-pm@...r.kernel.org, Alan Cox <gnomes@...rguk.ukuu.org.uk>,
	Alexander Graf <agraf@...e.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Heiko Stuebner <heiko@...ech.de>,
	Lee Jones <lee.jones@...aro.org>,
	Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Romain Perier <romain.perier@...il.com>
Subject: Re: [PATCH v2 01/47] kernel: Add support for poweroff handler call
 chain

On 10/22/2014 01:02 AM, Philippe Rétornaz wrote:
> Le 21/10/2014 15:29, Guenter Roeck a écrit :
>> On 10/20/2014 11:46 PM, Philippe Rétornaz wrote:
>>> Hello
>>>
>>> [...]
>>>> - Use raw notifiers protected by spinlocks instead of atomic notifiers
>>> [...]
>>>
>>>> +/**
>>>> + *    do_kernel_power_off - Execute kernel poweroff handler call chain
>>>> + *
>>>> + *    Calls functions registered with register_power_off_handler.
>>>> + *
>>>> + *    Expected to be called from machine_power_off as last step of
>>>> + *    the poweroff sequence.
>>>> + *
>>>> + *    Powers off the system immediately if a poweroff handler function
>>>> + *    has been registered. Otherwise does nothing.
>>>> + */
>>>> +void do_kernel_power_off(void)
>>>> +{
>>>> +    spin_lock(&power_off_handler_lock);
>>>> +    raw_notifier_call_chain(&power_off_handler_list, 0, NULL);
>>>> +    spin_unlock(&power_off_handler_lock);
>>>> +}
>>>
>>> I don't get it. You are still in atomic context inside the poweroff
>>> callback
>>> since you lock it with a spinlock.
> [...]
>>>
>>> Why not using the blocking_notifier_* family ?
>>> It will lock with a read-write semaphore under which you can sleep.
>>>
>>> For instance, twl4030_power_off will sleep, since it is doing I2C access.
>>> So you cannot call it in atomic context.
>>>
>>
>> Learning something new all the time. I assumed that spin_lock (unlike
>> spin_lock_irqsafe) would not run in atomic context.
>>
>> I did not want to use a sleeping lock because I am not sure if that
>> works for all architectures; some disable (local) interrupts before
>> calling the function (eg arm and arm64), and I don't want to change
>> that semantics.
>
> You're right and it even disable all others CPUs (if any).
> I don't understand why it needs to disable local interrupts since the
> code path to pm_power_off is simply doing:
>
> syscall -> migrate to reboot cpu -> disable local interrupt -> disable others cpu -> pm_power_off().
>
> I don't understand why we cannot re-enable interrupts right before pm_power_off().
> And it looks like that all pm_power_off callbacks which can sleep are broken.
> I still wonder how i2c communication can works without local interrupts ... it looks
> like somebody is re-enabling them (or the code was never run)
>
Good question. Or the code was never run under arm, or the drivers have a polling
fallback if interrupts are disabled.

>> I have another idea how to get there without changing the lock situation
>> while executing the call chain - just set a flag indicating that it is
>> running and execute it without lock. Would that work ?
>
> I don't think inventing a new locking mechanism is a good solution.
> We need first to know for sure if we can sleep or not in pm_power_off.
> If yes then we need to re-enable local interrupts and we can use a mutex.
> If not then the atomic notifier is fine and a lots of drivers are wrong.
>

I had another look into the kernel, checking all the callers of machine_power_off().
In some cases, the function is called directly from interrupt handlers, meaning
there is no guarantee that interrupts are enabled to start with when it is called.
So there is more to fix if the semantics of pm_power_off requires interrupts
to be enabled.

My current solution is to use a spinlock with disabled interrupts during
poweroff handler registration and unregistration, and no lock at all while
executing the call chain. That is not perfect, but it retains the current
situation and thus guarantees that existing code works as before. This
disconnects the sleep vs. no sleep problem from the current patch series,
and we can solve that problem separately.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ