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: <alpine.DEB.2.02.1403111147540.18573@ionos.tec.linutronix.de>
Date:	Tue, 11 Mar 2014 11:48:49 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Juri Lelli <juri.lelli@...il.com>
cc:	Steven Rostedt <rostedt@...dmis.org>,
	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 Tue, 11 Mar 2014, Juri Lelli wrote:
> 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.

Correct.
 
> 
> 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;

Right, you beat me.

>  	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 :/).

Steve, can you please send an updated one?

Thanks,

	tglx
--
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/