| 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: Fri, 22 Nov 2013 11:37:45 +0100
From: Juri Lelli <juri.lelli@...il.com>
To: Steven Rostedt <rostedt@...dmis.org>
CC: peterz@...radead.org, tglx@...utronix.de, mingo@...hat.com,
oleg@...hat.com, fweisbec@...il.com, darren@...art.com,
johan.eker@...csson.com, p.faure@...tech.ch,
linux-kernel@...r.kernel.org, claudio@...dence.eu.com,
michael@...rulasolutions.com, fchecconi@...il.com,
tommaso.cucinotta@...up.it, nicola.manica@...i.unitn.it,
luca.abeni@...tn.it, dhaval.giani@...il.com, hgu1972@...il.com,
paulmck@...ux.vnet.ibm.com, raistlin@...ux.it,
insop.song@...il.com, liming.wang@...driver.com, jkacur@...hat.com,
harald.gustafsson@...csson.com, vincent.guittot@...aro.org,
bruce.ashfield@...driver.com
Subject: Re: [PATCH] rtmutex: Fix compare of waiter prio and task prio
On 11/21/2013 06:52 PM, Steven Rostedt wrote:
> The conversion of the rt_mutex from using plist to rbtree eliminated
> the use of the waiter->list_entry.prio, and instead used directly the
> waiter->task->prio.
>
> The problem with this is that the priority inheritance code relies on
> the prio of the waiter being stored is different from the task's prio.
> The change didn't take into account waiter->task == task, which makes
> the compares of:
>
> if (waiter->task->prio == task->prio)
>
> rather pointless, since they will always be the same:
>
> task->pi_blocked_on = waiter;
> waiter->task = task;
>
> When deadlock detection is not being used (for internal users of
> rt_mutex_lock(); things other than futex), the code relies on
> the prio associated to the waiter being different than the prio
> associated to the task.
>
> Another use case where this is critical, is when a task that is
> blocked on an rt_mutex has its priority increased by a separate task.
> Then the compare in rt_mutex_adjust_pi() (called from
> sched_setscheduler()), returns without doing anything. This is because
> it checks if the priority of the task is different than the priority of
> its waiter.
>
> The simple solution is to add a prio member to the rt_mutex_waiter
> structure that associates the priority to the waiter that is separate
> from the task.
>
> I created a test program that tests this case:
>
> http://rostedt.homelinux.com/code/pi_mutex_test.c
>
> (too big to include in a change log) I'll work on getting this test
> into other projects like LTP and the kernel (perf test?)
>
> Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
>
> Index: linux-rt.git/kernel/rtmutex.c
> ===================================================================
> --- linux-rt.git.orig/kernel/rtmutex.c
> +++ linux-rt.git/kernel/rtmutex.c
> @@ -197,7 +197,7 @@ int rt_mutex_getprio(struct task_struct
> if (likely(!task_has_pi_waiters(task)))
> return task->normal_prio;
>
> - return min(task_top_pi_waiter(task)->task->prio,
> + return min(task_top_pi_waiter(task)->prio,
> task->normal_prio);
> }
>
> @@ -336,7 +336,7 @@ static int rt_mutex_adjust_prio_chain(st
> * When deadlock detection is off then we check, if further
> * priority adjustment is necessary.
> */
> - if (!detect_deadlock && waiter->task->prio == task->prio)
> + if (!detect_deadlock && waiter->prio == task->prio)
> goto out_unlock_pi;
>
> lock = waiter->lock;
> @@ -358,7 +358,7 @@ static int rt_mutex_adjust_prio_chain(st
>
> /* Requeue the waiter */
> rt_mutex_dequeue(lock, waiter);
> - waiter->task->prio = task->prio;
> + waiter->prio = task->prio;
> rt_mutex_enqueue(lock, waiter);
>
> /* Release the task */
> @@ -456,7 +456,7 @@ static int try_to_take_rt_mutex(struct r
> * 3) it is top waiter
> */
> if (rt_mutex_has_waiters(lock)) {
> - if (task->prio >= rt_mutex_top_waiter(lock)->task->prio) {
> + if (task->prio >= rt_mutex_top_waiter(lock)->prio) {
> if (!waiter || waiter != rt_mutex_top_waiter(lock))
> return 0;
> }
> @@ -516,7 +516,8 @@ static int task_blocks_on_rt_mutex(struc
> __rt_mutex_adjust_prio(task);
> waiter->task = task;
> waiter->lock = lock;
> -
> + waiter->prio = task->prio;
> +
> /* Get the top priority waiter on the lock */
> if (rt_mutex_has_waiters(lock))
> top_waiter = rt_mutex_top_waiter(lock);
> @@ -661,7 +662,7 @@ void rt_mutex_adjust_pi(struct task_stru
> raw_spin_lock_irqsave(&task->pi_lock, flags);
>
> waiter = task->pi_blocked_on;
> - if (!waiter || (waiter->task->prio == task->prio &&
> + if (!waiter || (waiter->prio == task->prio &&
> !dl_prio(task->prio))) {
> raw_spin_unlock_irqrestore(&task->pi_lock, flags);
> return;
> Index: linux-rt.git/kernel/rtmutex_common.h
> ===================================================================
> --- linux-rt.git.orig/kernel/rtmutex_common.h
> +++ linux-rt.git/kernel/rtmutex_common.h
> @@ -54,6 +54,7 @@ struct rt_mutex_waiter {
> struct pid *deadlock_task_pid;
> struct rt_mutex *deadlock_lock;
> #endif
> + int prio;
> };
>
> /*
>
Thanks! But, now that waiters have their own prio, don't we need to
enqueue them using that?
Something like:
rtmutex: enqueue waiters by their prio
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index a2c8ee8..2e960a2 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -96,13 +96,16 @@ static inline int
rt_mutex_waiter_less(struct rt_mutex_waiter *left,
struct rt_mutex_waiter *right)
{
- if (left->task->prio < right->task->prio)
+ if (left->prio < right->prio)
return 1;
/*
- * If both tasks are dl_task(), we check their deadlines.
+ * If both waiters have dl_prio(), we check the deadlines of the
+ * associated tasks.
+ * If left waiter has a dl_prio(), and we didn't return 1 above,
+ * then right waiter has a dl_prio() too.
*/
- if (dl_prio(left->task->prio) && dl_prio(right->task->prio))
+ if (dl_prio(left->prio))
return (left->task->dl.deadline < right->task->dl.deadline);
return 0;
Thanks,
- Juri
--
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