| From 14917f84df627b355d2df7770768952b2fb799e7 Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Thu, 25 Feb 2021 17:56:56 +0000 |
| Subject: sched/fair: Fix shift-out-of-bounds in load_balance() |
| |
| From: Valentin Schneider <valentin.schneider@arm.com> |
| |
| [ Upstream commit 39a2a6eb5c9b66ea7c8055026303b3aa681b49a5 ] |
| |
| Syzbot reported a handful of occurrences where an sd->nr_balance_failed can |
| grow to much higher values than one would expect. |
| |
| A successful load_balance() resets it to 0; a failed one increments |
| it. Once it gets to sd->cache_nice_tries + 3, this *should* trigger an |
| active balance, which will either set it to sd->cache_nice_tries+1 or reset |
| it to 0. However, in case the to-be-active-balanced task is not allowed to |
| run on env->dst_cpu, then the increment is done without any further |
| modification. |
| |
| This could then be repeated ad nauseam, and would explain the absurdly high |
| values reported by syzbot (86, 149). VincentG noted there is value in |
| letting sd->cache_nice_tries grow, so the shift itself should be |
| fixed. That means preventing: |
| |
| """ |
| If the value of the right operand is negative or is greater than or equal |
| to the width of the promoted left operand, the behavior is undefined. |
| """ |
| |
| Thus we need to cap the shift exponent to |
| BITS_PER_TYPE(typeof(lefthand)) - 1. |
| |
| I had a look around for other similar cases via coccinelle: |
| |
| @expr@ |
| position pos; |
| expression E1; |
| expression E2; |
| @@ |
| ( |
| E1 >> E2@pos |
| | |
| E1 >> E2@pos |
| ) |
| |
| @cst depends on expr@ |
| position pos; |
| expression expr.E1; |
| constant cst; |
| @@ |
| ( |
| E1 >> cst@pos |
| | |
| E1 << cst@pos |
| ) |
| |
| @script:python depends on !cst@ |
| pos << expr.pos; |
| exp << expr.E2; |
| @@ |
| # Dirty hack to ignore constexpr |
| if exp.upper() != exp: |
| coccilib.report.print_report(pos[0], "Possible UB shift here") |
| |
| The only other match in kernel/sched is rq_clock_thermal() which employs |
| sched_thermal_decay_shift, and that exponent is already capped to 10, so |
| that one is fine. |
| |
| Fixes: 5a7f55590467 ("sched/fair: Relax constraint on task's load during load balance") |
| Reported-by: syzbot+d7581744d5fd27c9fbe1@syzkaller.appspotmail.com |
| Signed-off-by: Valentin Schneider <valentin.schneider@arm.com> |
| Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> |
| Signed-off-by: Ingo Molnar <mingo@kernel.org> |
| Link: http://lore.kernel.org/r/000000000000ffac1205b9a2112f@google.com |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| kernel/sched/fair.c | 3 +-- |
| kernel/sched/sched.h | 7 +++++++ |
| 2 files changed, 8 insertions(+), 2 deletions(-) |
| |
| diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c |
| index 828978320e44..d9182af98988 100644 |
| --- a/kernel/sched/fair.c |
| +++ b/kernel/sched/fair.c |
| @@ -7760,8 +7760,7 @@ static int detach_tasks(struct lb_env *env) |
| * scheduler fails to find a good waiting task to |
| * migrate. |
| */ |
| - |
| - if ((load >> env->sd->nr_balance_failed) > env->imbalance) |
| + if (shr_bound(load, env->sd->nr_balance_failed) > env->imbalance) |
| goto next; |
| |
| env->imbalance -= load; |
| diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h |
| index 282a6bbaacd7..d52c6bb6ed7d 100644 |
| --- a/kernel/sched/sched.h |
| +++ b/kernel/sched/sched.h |
| @@ -204,6 +204,13 @@ static inline void update_avg(u64 *avg, u64 sample) |
| *avg += diff / 8; |
| } |
| |
| +/* |
| + * Shifting a value by an exponent greater *or equal* to the size of said value |
| + * is UB; cap at size-1. |
| + */ |
| +#define shr_bound(val, shift) \ |
| + (val >> min_t(typeof(shift), shift, BITS_PER_TYPE(typeof(val)) - 1)) |
| + |
| /* |
| * !! For sched_setattr_nocheck() (kernel) only !! |
| * |
| -- |
| 2.30.2 |
| |