linux-kernel - Re: [PATCH v2] sched: Fix broken setscheduler()

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: <20140311103545.fc7f0f6b1bf0eb1d3391c85c@gmail.com>
Date:	Tue, 11 Mar 2014 10:35:45 +0100
From:	Juri Lelli <juri.lelli@...il.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <peterz@...radead.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH v2] sched: Fix broken setscheduler()

On Mon, 10 Mar 2014 17:37:31 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:

> On Mon, 10 Mar 2014 22:18:56 +0100 (CET)
> Thomas Gleixner <tglx@...utronix.de> wrote:
> 
> 
> > Lemme look at it tomorrow again with an awake brain. This seems to be
> > some forward porting hickup which needs a closer look. Just look at
> 
> Yep, I talked with Sebastian on IRC and that seems to be the case.
> 
> > the 3.10-rt version of this:
> > 
> > @@ -3825,20 +3826,25 @@ static struct task_struct *find_process_by_pid(pid_t pid)
> >  	return pid ? find_task_by_vpid(pid) : current;
> >  }
> >  
> > -/* Actually do priority change: must hold rq lock. */
> > -static void
> > -__setscheduler(struct rq *rq, struct task_struct *p, int policy, int prio)
> > +static void __setscheduler_params(struct task_struct *p, int policy, int prio)
> >  {
> >  	p->policy = policy;
> >  	p->rt_priority = prio;
> >  	p->normal_prio = normal_prio(p);
> > +	set_load_weight(p);
> > +}
> > 
> > That code has changed significantly probably due to the EDF merge. We
> > need to figure out whether there is more damage due to that.
> 
> Yeah, when I looked at the -rt version, it appeared to have my fix
> already. But in reality, the forward port broke it. Here's the problem
> part of the commit:
> 
> +       set_load_weight(p);
> +}
>  
> -       p->normal_prio = normal_prio(p);
> -       p->prio = rt_mutex_getprio(p);
> 
> Your patch never deleted the above two. And it kept them in the
> locations that I placed them in, in my patch.
> 

Oh, and you have to have also something like this

@@ -3271,7 +3271,8 @@ static int __sched_setscheduler(struct task_struct *p,
 				const struct sched_attr *attr,
 				bool user)
 {
-	int newprio = MAX_RT_PRIO - 1 - attr->sched_priority;
+	int newprio = dl_policy(attr->sched_policy) ? MAX_DL_PRIO - 1 :
+		      MAX_RT_PRIO - 1 - attr->sched_priority;
 	int retval, oldprio, oldpolicy = -1, on_rq, running;
 	int policy = attr->sched_policy;
 	unsigned long flags;

or you can fail to become DL if you are currently boosted by RT, as
attr->sched_priority == 0 for DL tasks. Then rt_mutex_check_prio ()
returns 1 (just update params) if setting DL params for a task already
boosted by another DL. But this seems to be ok, as we are already
outside enforcement in this situation. (This is going to be trickier
with proxy exec, though :/).

Thanks,

- Juri

> -- Steve
> 
> 
> +/* Actually do priority change: must hold pi & rq lock. */
> +static void __setscheduler(struct rq *rq, struct task_struct *p,
> +                          const struct sched_attr *attr)
> +{
> 
> 
--
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/