| 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: Tue, 21 Nov 2017 11:56:57 +0100
From: Luca Abeni <luca.abeni@...tannapisa.it>
To: Jiri Kosina <jikos@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>,
Daniel Bristot de Oliveira <bristot@...hat.com>,
Juri Lelli <juri.lelli@....com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Mathieu Poirier <mathieu.poirier@...aro.org>,
Mike Galbraith <efault@....de>,
Steven Rostedt <rostedt@...dmis.org>,
Thomas Gleixner <tglx@...utronix.de>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched/deadline: Use bools for the state flags
Hello,
On Tue, 21 Nov 2017 11:44:05 +0100 (CET)
Jiri Kosina <jikos@...nel.org> wrote:
> From: Jiri Kosina <jkosina@...e.cz>
>
> Commit
>
> 799ba82de01e ("sched/deadline: Use C bitfields for the state flags")
>
> converted state flags into one-bit signed int. Signed one-bit type can be
> either 0 or -1, which is going to cause a problem once 1 is assigned to it
> and then the value later tested against 1.
I think a different fix has just been committed to tip:
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=aa5222e92f8000ed3c1c38dddf11c83222aadfb3
Luca
>
> The current code is okay, as all the checks are (non-)zero check, but I
> believe that we'd rather be safe than sorry here; remove the fragility by
> converting the state flags to bool.
>
> This also silences annoying sparse complaints about this very issue when
> compiling any code that includes sched.h.
>
> Fixes: 799ba82de01e ("sched/deadline: Use C bitfields for the state flags")
> Cc: luca abeni <luca.abeni@...tannapisa.it>
> Cc: Peter Zijlstra (Intel) <peterz@...radead.org>
> Cc: Daniel Bristot de Oliveira <bristot@...hat.com>
> Cc: Juri Lelli <juri.lelli@....com>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
> Cc: Mike Galbraith <efault@....de>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Signed-off-by: Jiri Kosina <jkosina@...e.cz>
> ---
> include/linux/sched.h | 8 ++++----
> kernel/sched/core.c | 8 ++++----
> kernel/sched/deadline.c | 32 ++++++++++++++++----------------
> 3 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index a5dc7c98b0a2..b19fa1b96726 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -473,10 +473,10 @@ struct sched_dl_entity {
> * conditions between the inactive timer handler and the wakeup
> * code.
> */
> - int dl_throttled : 1;
> - int dl_boosted : 1;
> - int dl_yielded : 1;
> - int dl_non_contending : 1;
> + bool dl_throttled;
> + bool dl_boosted;
> + bool dl_yielded;
> + bool dl_non_contending;
>
> /*
> * Bandwidth enforcement timer. Each -deadline task has its
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 75554f366fd3..f9bbf0f55e0e 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3738,20 +3738,20 @@ void rt_mutex_setprio(struct task_struct *p, struct task_struct *pi_task)
> if (dl_prio(prio)) {
> if (!dl_prio(p->normal_prio) ||
> (pi_task && dl_entity_preempt(&pi_task->dl, &p->dl))) {
> - p->dl.dl_boosted = 1;
> + p->dl.dl_boosted = true;
> queue_flag |= ENQUEUE_REPLENISH;
> } else
> - p->dl.dl_boosted = 0;
> + p->dl.dl_boosted = false;
> p->sched_class = &dl_sched_class;
> } else if (rt_prio(prio)) {
> if (dl_prio(oldprio))
> - p->dl.dl_boosted = 0;
> + p->dl.dl_boosted = false;
> if (oldprio < prio)
> queue_flag |= ENQUEUE_HEAD;
> p->sched_class = &rt_sched_class;
> } else {
> if (dl_prio(oldprio))
> - p->dl.dl_boosted = 0;
> + p->dl.dl_boosted = false;
> if (rt_prio(oldprio))
> p->rt.timeout = 0;
> p->sched_class = &fair_sched_class;
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 2473736c7616..78cb00ae8b2f 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -133,7 +133,7 @@ void dl_change_utilization(struct task_struct *p, u64 new_bw)
> rq = task_rq(p);
> if (p->dl.dl_non_contending) {
> sub_running_bw(p->dl.dl_bw, &rq->dl);
> - p->dl.dl_non_contending = 0;
> + p->dl.dl_non_contending = false;
> /*
> * If the timer handler is currently running and the
> * timer cannot be cancelled, inactive_task_timer()
> @@ -251,7 +251,7 @@ static void task_non_contending(struct task_struct *p)
> return;
> }
>
> - dl_se->dl_non_contending = 1;
> + dl_se->dl_non_contending = true;
> get_task_struct(p);
> hrtimer_start(timer, ns_to_ktime(zerolag_time), HRTIMER_MODE_REL);
> }
> @@ -271,7 +271,7 @@ static void task_contending(struct sched_dl_entity *dl_se, int flags)
> add_rq_bw(dl_se->dl_bw, dl_rq);
>
> if (dl_se->dl_non_contending) {
> - dl_se->dl_non_contending = 0;
> + dl_se->dl_non_contending = false;
> /*
> * If the timer handler is currently running and the
> * timer cannot be cancelled, inactive_task_timer()
> @@ -676,9 +676,9 @@ static void replenish_dl_entity(struct sched_dl_entity *dl_se,
> }
>
> if (dl_se->dl_yielded)
> - dl_se->dl_yielded = 0;
> + dl_se->dl_yielded = false;
> if (dl_se->dl_throttled)
> - dl_se->dl_throttled = 0;
> + dl_se->dl_throttled = false;
> }
>
> /*
> @@ -1051,7 +1051,7 @@ static inline void dl_check_constrained_dl(struct sched_dl_entity *dl_se)
> dl_time_before(rq_clock(rq), dl_next_period(dl_se))) {
> if (unlikely(dl_se->dl_boosted || !start_dl_timer(p)))
> return;
> - dl_se->dl_throttled = 1;
> + dl_se->dl_throttled = true;
> if (dl_se->runtime > 0)
> dl_se->runtime = 0;
> }
> @@ -1154,7 +1154,7 @@ static void update_curr_dl(struct rq *rq)
>
> throttle:
> if (dl_runtime_exceeded(dl_se) || dl_se->dl_yielded) {
> - dl_se->dl_throttled = 1;
> + dl_se->dl_throttled = true;
> __dequeue_task_dl(rq, curr, 0);
> if (unlikely(dl_se->dl_boosted || !start_dl_timer(curr)))
> enqueue_task_dl(rq, curr, ENQUEUE_REPLENISH);
> @@ -1206,7 +1206,7 @@ static enum hrtimer_restart inactive_task_timer(struct hrtimer *timer)
> if (p->state == TASK_DEAD && dl_se->dl_non_contending) {
> sub_running_bw(p->dl.dl_bw, dl_rq_of_se(&p->dl));
> sub_rq_bw(p->dl.dl_bw, dl_rq_of_se(&p->dl));
> - dl_se->dl_non_contending = 0;
> + dl_se->dl_non_contending = false;
> }
>
> raw_spin_lock(&dl_b->lock);
> @@ -1216,14 +1216,14 @@ static enum hrtimer_restart inactive_task_timer(struct hrtimer *timer)
>
> goto unlock;
> }
> - if (dl_se->dl_non_contending == 0)
> + if (!dl_se->dl_non_contending)
> goto unlock;
>
> sched_clock_tick();
> update_rq_clock(rq);
>
> sub_running_bw(dl_se->dl_bw, &rq->dl);
> - dl_se->dl_non_contending = 0;
> + dl_se->dl_non_contending = false;
> unlock:
> task_rq_unlock(rq, p, &rf);
> put_task_struct(p);
> @@ -1492,7 +1492,7 @@ static void yield_task_dl(struct rq *rq)
> * it and the bandwidth timer will wake it up and will give it
> * new scheduling parameters (thanks to dl_yielded=1).
> */
> - rq->curr->dl.dl_yielded = 1;
> + rq->curr->dl.dl_yielded = true;
>
> update_rq_clock(rq);
> update_curr_dl(rq);
> @@ -1565,7 +1565,7 @@ static void migrate_task_rq_dl(struct task_struct *p)
> raw_spin_lock(&rq->lock);
> if (p->dl.dl_non_contending) {
> sub_running_bw(p->dl.dl_bw, &rq->dl);
> - p->dl.dl_non_contending = 0;
> + p->dl.dl_non_contending = false;
> /*
> * If the timer handler is currently running and the
> * timer cannot be cancelled, inactive_task_timer()
> @@ -2232,7 +2232,7 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
> * while SCHED_OTHER in the meanwhile.
> */
> if (p->dl.dl_non_contending)
> - p->dl.dl_non_contending = 0;
> + p->dl.dl_non_contending = false;
>
> /*
> * Since this might be the only -deadline task on the rq,
> @@ -2563,9 +2563,9 @@ void __dl_clear_params(struct task_struct *p)
> dl_se->dl_bw = 0;
> dl_se->dl_density = 0;
>
> - dl_se->dl_throttled = 0;
> - dl_se->dl_yielded = 0;
> - dl_se->dl_non_contending = 0;
> + dl_se->dl_throttled = false;
> + dl_se->dl_yielded = false;
> + dl_se->dl_non_contending = false;
> }
>
> bool dl_param_changed(struct task_struct *p, const struct sched_attr *attr)
>
Powered by blists - more mailing lists