| 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
| ||
|
Date: Wed, 2 May 2018 10:45:52 +0530
From: "Kohli, Gaurav" <gkohli@...eaurora.org>
To: Peter Zijlstra <peterz@...radead.org>
Cc: tglx@...utronix.de, mpe@...erman.id.au, mingo@...nel.org,
bigeasy@...utronix.de, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org,
Neeraj Upadhyay <neeraju@...eaurora.org>,
Will Deacon <will.deacon@....com>,
Oleg Nesterov <oleg@...hat.com>
Subject: Re: [PATCH v1] kthread/smpboot: Serialize kthread parking against
wakeup
On 5/1/2018 6:49 PM, Peter Zijlstra wrote:
>> On 5/1/2018 5:01 PM, Peter Zijlstra wrote:
>
>>> Let me ponder what the best solution is, it's a bit of a mess.
>
> So there's:
>
> - TASK_PARKED, which we can make a special state; this solves the
> problem because then wait_task_inactive() is guaranteed to see
> TASK_PARKED and DTRT.
Yes this should work, as it takes pi-lock and blocking kthread_parkme.
>
> - complete(&kthread->parked), which we can do inside schedule(); this
> solves the problem because then kthread_park() will not return early
> and the task really is blocked.
I think complete will not help, as problem is like below :
Control Thread CPUHP thread
cpuhp_thread_fun
Wake control thread
complete(&st->done);
takedown_cpu
kthread_park
set_bit(KTHREAD_SHOULD_PARK
Here cpuhp is looping,
//success case
Generally when issue is not
coming
it schedule out by below :
ht->thread_should_run(td->cpu
scheduler
//failure case
before schedule
loop check
(kthread_should_park()
enter here as PARKED set
wake_up_process(k)
__kthread_parkme
complete(&self->parked);
SETS RUNNING
schedule
wait_for_completion(&kthread->parked);
So even we protect complete, it may see TASK_RUNNING as final state.
That's why we took pi-lock , So wake_up_process either see TASK_RUNNING
so return or it will exit early before parkme call.
Please correct me , if i misunderstood it. With pi-lock approach we are
not seeing issue. SO you first solution should fix this(put parked in
special state).
Regards
Gaurav
>
> and I'm fairly sure I thought of a 3rd way to cure things, but now that
> I'm writing things down I cannot seem to remember :/ -- could be we muck
> with wait_task_inactive().
>
> In any case, I hate all of them, but I think the completion one is the
> least horrible because it gives the strongest guarantees and cleans up
> most. But promoting TASK_PARKED to special certainly is the earier
> patch.
>
> The below boots, but that's about all I did with it. Opinions?
>
> ---
>
> --- a/include/linux/kthread.h
> +++ b/include/linux/kthread.h
> @@ -62,6 +62,7 @@ void *kthread_probe_data(struct task_str
> int kthread_park(struct task_struct *k);
> void kthread_unpark(struct task_struct *k);
> void kthread_parkme(void);
> +void kthread_park_complete(struct task_struct *k);
>
> int kthreadd(void *unused);
> extern struct task_struct *kthreadd_task;
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -55,7 +55,6 @@ enum KTHREAD_BITS {
> KTHREAD_IS_PER_CPU = 0,
> KTHREAD_SHOULD_STOP,
> KTHREAD_SHOULD_PARK,
> - KTHREAD_IS_PARKED,
> };
>
> static inline void set_kthread_struct(void *kthread)
> @@ -177,14 +176,12 @@ void *kthread_probe_data(struct task_str
>
> static void __kthread_parkme(struct kthread *self)
> {
> - __set_current_state(TASK_PARKED);
> - while (test_bit(KTHREAD_SHOULD_PARK, &self->flags)) {
> - if (!test_and_set_bit(KTHREAD_IS_PARKED, &self->flags))
> - complete(&self->parked);
> + for (;;) {
> + set_current_state(TASK_PARKED);
> + if (!test_bit(KTHREAD_SHOULD_PARK, &self->flags))
> + break;
> schedule();
> - __set_current_state(TASK_PARKED);
> }
> - clear_bit(KTHREAD_IS_PARKED, &self->flags);
> __set_current_state(TASK_RUNNING);
> }
>
> @@ -194,6 +191,11 @@ void kthread_parkme(void)
> }
> EXPORT_SYMBOL_GPL(kthread_parkme);
>
> +void kthread_park_complete(struct task_struct *k)
> +{
> + complete(&to_kthread(k)->parked);
> +}
> +
> static int kthread(void *_create)
> {
> /* Copy data: it's on kthread's stack */
> @@ -450,22 +452,15 @@ void kthread_unpark(struct task_struct *
> {
> struct kthread *kthread = to_kthread(k);
>
> - clear_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
> /*
> - * We clear the IS_PARKED bit here as we don't wait
> - * until the task has left the park code. So if we'd
> - * park before that happens we'd see the IS_PARKED bit
> - * which might be about to be cleared.
> + * Newly created kthread was parked when the CPU was offline.
> + * The binding was lost and we need to set it again.
> */
> - if (test_and_clear_bit(KTHREAD_IS_PARKED, &kthread->flags)) {
> - /*
> - * Newly created kthread was parked when the CPU was offline.
> - * The binding was lost and we need to set it again.
> - */
> - if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags))
> - __kthread_bind(k, kthread->cpu, TASK_PARKED);
> - wake_up_state(k, TASK_PARKED);
> - }
> + if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags))
> + __kthread_bind(k, kthread->cpu, TASK_PARKED);
> +
> + clear_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
> + wake_up_state(k, TASK_PARKED);
> }
> EXPORT_SYMBOL_GPL(kthread_unpark);
>
> @@ -488,12 +483,13 @@ int kthread_park(struct task_struct *k)
> if (WARN_ON(k->flags & PF_EXITING))
> return -ENOSYS;
>
> - if (!test_bit(KTHREAD_IS_PARKED, &kthread->flags)) {
> - set_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
> - if (k != current) {
> - wake_up_process(k);
> - wait_for_completion(&kthread->parked);
> - }
> + if (WARN_ON_ONCE(test_bit(KTHREAD_SHOULD_PARK, &kthread->flags)))
> + return -EBUSY;
> +
> + set_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
> + if (k != current) {
> + wake_up_process(k);
> + wait_for_completion(&kthread->parked);
> }
>
> return 0;
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7,6 +7,8 @@
> */
> #include "sched.h"
>
> +#include <linux/kthread.h>
> +
> #include <asm/switch_to.h>
> #include <asm/tlb.h>
>
> @@ -2718,20 +2720,28 @@ static struct rq *finish_task_switch(str
> membarrier_mm_sync_core_before_usermode(mm);
> mmdrop(mm);
> }
> - if (unlikely(prev_state == TASK_DEAD)) {
> - if (prev->sched_class->task_dead)
> - prev->sched_class->task_dead(prev);
> + if (unlikely(prev_state & (TASK_DEAD|TASK_PARKED))) {
> + switch (prev_state) {
> + case TASK_DEAD:
> + if (prev->sched_class->task_dead)
> + prev->sched_class->task_dead(prev);
> +
> + /*
> + * Remove function-return probe instances associated with this
> + * task and put them back on the free list.
> + */
> + kprobe_flush_task(prev);
>
> - /*
> - * Remove function-return probe instances associated with this
> - * task and put them back on the free list.
> - */
> - kprobe_flush_task(prev);
> + /* Task is done with its stack. */
> + put_task_stack(prev);
>
> - /* Task is done with its stack. */
> - put_task_stack(prev);
> + put_task_struct(prev);
> + break;
>
> - put_task_struct(prev);
> + case TASK_PARKED:
> + kthread_park_complete(prev);
> + break;
> + }
> }
>
> tick_nohz_task_switch();
>
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
Powered by blists - more mailing lists