linux-kernel - Re: [RFC PATCH 1/6] sched/fair: check_preempt_wakeup: Fix assumption on the default policy

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: <342487362.26930.1474409041257.JavaMail.zimbra@efficios.com>
Date:   Tue, 20 Sep 2016 22:04:01 +0000 (UTC)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Julien Desfossez <jdesfossez@...icios.com>,
        Peter Zijlstra <peterz@...radead.org>,
        rostedt <rostedt@...dmis.org>, Ingo Molnar <mingo@...hat.com>,
        daolivei <daolivei@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/6] sched/fair: check_preempt_wakeup: Fix
 assumption on the default policy

----- On Sep 20, 2016, at 4:49 PM, Thomas Gleixner tglx@...utronix.de wrote:

> On Fri, 16 Sep 2016, Julien Desfossez wrote:
> 
>> Tasks with RT or deadline scheduling class may inherit from a task with
>> a "fair" scheduling class.
> 
> This makes no sense. A RT/DL task can never inherit anything from a sched
> fair task. That would be inverted priority inheritance.
> 
>> This priority inheritance changes the scheduling class, but not the task
>> "policy" field.
>>
>> Therefore, the fair scheduler should not assume that policy !=
>> SCHED_NORMAL is the same as (policy == SCHED_BATCH || policy ==
>> SCHED_IDLE), because the policy could also be SCHED_RR, SCHED_FIFO, or
>> SCHED_DEADLINE.
>> 
>> The incorrect comparison in check_preempt_wakeup makes RR, FIFO and
>> DEADLINE tasks which inherit from a fair task behave as if they were
>> IDLE or BATCH tasks, thus awaiting the following tick before preempting
>> the current task.
> 
> This is just wrong.
> 
> Priority/deadline inheritance elevates a fair task to RR/FIFO/DL, i.e. to
> the scheduling class of the task which is blocked on a resource held by the
> fair task.
> 
> The check_preempt_curr() callback of a scheduling class is only invoked
> when the freshly woken task is in the same scheduling class as the task
> which is currently on the cpu.
> 
> So which problem are you actually solving?

So what is then puzzling us is this:

rt_mutex_setprio()

        if (dl_prio(prio)) {
                struct task_struct *pi_task = rt_mutex_get_top_task(p);
                if (!dl_prio(p->normal_prio) ||
                    (pi_task && dl_entity_preempt(&pi_task->dl, &p->dl))) {
                        p->dl.dl_boosted = 1;
                        queue_flag |= ENQUEUE_REPLENISH;
                } else
                        p->dl.dl_boosted = 0;
                p->sched_class = &dl_sched_class;
        } else if (rt_prio(prio)) {
                if (dl_prio(oldprio))
                        p->dl.dl_boosted = 0;
                if (oldprio < prio)
                        queue_flag |= ENQUEUE_HEAD;
                p->sched_class = &rt_sched_class;
        } else {
                if (dl_prio(oldprio))
                        p->dl.dl_boosted = 0;
                if (rt_prio(oldprio))
                        p->rt.timeout = 0;
                p->sched_class = &fair_sched_class;
        }

So in the 3rd block, this is the case where we inherit a
new prio which is neither LD nor RT, so it's "fair".

If we try to assign a fair prio to a task of DL or RT
prio, the dl_boosted is set to 0, or the rt timeout is
set to 0. However, we do change the sched_class of the
target task to &fair_sched_class.

This code path seems to imply that a RT or DL task can
change sched_class to "fair". Indeed, it makes no sense,
so I have the feeling we're missing something important
here.


> 
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> Cc: Steven Rostedt (Red Hat) <rostedt@...dmis.org>
>> Cc: Thomas Gleixner <tglx@...utronix.de>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
>> Signed-off-by: Julien Desfossez <jdesfossez@...icios.com>
> 
> Who wrote the patch?

Julien is the author.

Thanks,

Mathieu

> 
> Thanks,
> 
> 	tglx

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com