2004-10-22 20:02:54

by Chris Wright

Subject: [PATCH] delay rq_lock acquisition in setscheduler

Doing access control checks with rq_lock held can cause deadlock when
audit messages are created (via printk or audit infrastructure) which
trigger a wakeup and deadlock, as noted by both SELinux and SubDomain
folks. This patch will let the security checks happen w/out lock held,
then re-sample the p->policy in case it was raced. Originally from John
Johansen <[email protected]>, reworked by me. AFAIK, this version
drew no objections from Ingo or Andrea. Please let me know if there's
any issue with the patch.

From: John Johansen <[email protected]>
Signed-off-by: Chris Wright <[email protected]>

===== kernel/sched.c 1.367 vs edited =====
--- 1.367/kernel/sched.c 2004-10-18 22:26:52 -07:00
+++ edited/kernel/sched.c 2004-10-21 09:41:23 -07:00
@@ -3038,7 +3038,7 @@
{
struct sched_param lp;
int retval = -EINVAL;
- int oldprio;
+ int oldprio, oldpolicy = -1;
prio_array_t *array;
unsigned long flags;
runqueue_t *rq;
@@ -3060,23 +3060,17 @@

retval = -ESRCH;
if (!p)
- goto out_unlock_tasklist;
-
- /*
- * To be able to change p->policy safely, the apropriate
- * runqueue lock must be held.
- */
- rq = task_rq_lock(p, &flags);
-
+ goto out_unlock;
+recheck:
+ /* double check policy once rq lock held */
if (policy < 0)
- policy = p->policy;
+ policy = oldpolicy = p->policy;
else {
retval = -EINVAL;
if (policy != SCHED_FIFO && policy != SCHED_RR &&
policy != SCHED_NORMAL)
goto out_unlock;
}
-
/*
* Valid priorities for SCHED_FIFO and SCHED_RR are
* 1..MAX_USER_RT_PRIO-1, valid priority for SCHED_NORMAL is 0.
@@ -3098,7 +3092,17 @@
retval = security_task_setscheduler(p, policy, &lp);
if (retval)
goto out_unlock;
-
+ /*
+ * To be able to change p->policy safely, the apropriate
+ * runqueue lock must be held.
+ */
+ rq = task_rq_lock(p, &flags);
+ /* recheck policy now with rq lock held */
+ if (unlikely(oldpolicy != -1 && oldpolicy != p->policy)) {
+ policy = oldpolicy = -1;
+ task_rq_unlock(rq, &flags);
+ goto recheck;
+ }
array = p->array;
if (array)
deactivate_task(p, task_rq(p));
@@ -3118,12 +3122,9 @@
} else if (TASK_PREEMPTS_CURR(p, rq))
resched_task(rq->curr);
}
-
-out_unlock:
task_rq_unlock(rq, &flags);
-out_unlock_tasklist:
+out_unlock:
read_unlock_irq(&tasklist_lock);
-
out_nounlock:
return retval;
}

2004-10-23 10:45:41

by Ingo Molnar

Subject: Re: [PATCH] delay rq_lock acquisition in setscheduler

* Chris Wright <[email protected]> wrote:

> Doing access control checks with rq_lock held can cause deadlock when
> audit messages are created (via printk or audit infrastructure) which
> trigger a wakeup and deadlock, as noted by both SELinux and SubDomain
> folks. This patch will let the security checks happen w/out lock held,
> then re-sample the p->policy in case it was raced. Originally from John
> Johansen <[email protected]>, reworked by me. AFAIK, this version
> drew no objections from Ingo or Andrea. Please let me know if there's
> any issue with the patch.
>
> From: John Johansen <[email protected]>
> Signed-off-by: Chris Wright <[email protected]>

Acked-by: Ingo Molnar <[email protected]>

Ingo