linux-kernel - Re: [bug report] sched/fair: Prefer prev cpu in asymmetric wakeup path

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] [day] [month] [year] [list]

Date:   Fri, 13 Nov 2020 14:49:59 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Valentin Schneider <valentin.schneider@....com>,
        linux-kernel@...r.kernel.org
Subject: Re: [bug report] sched/fair: Prefer prev cpu in asymmetric wakeup
 path

On Fri, Nov 13, 2020 at 09:56:37AM +0100, Vincent Guittot wrote:
> Hi Dan,
> 
> Le vendredi 13 nov. 2020 à 11:46:57 (+0300), Dan Carpenter a écrit :
> > Hello Vincent Guittot,
> > 
> > The patch b4c9c9f15649: "sched/fair: Prefer prev cpu in asymmetric
> > wakeup path" from Oct 29, 2020, leads to the following static checker
> > warning:
> > 
> > 	kernel/sched/fair.c:6249 select_idle_sibling()
> > 	error: uninitialized symbol 'task_util'.
> > 
> > kernel/sched/fair.c
> >   6233  static int select_idle_sibling(struct task_struct *p, int prev, int target)
> >   6234  {
> >   6235          struct sched_domain *sd;
> >   6236          unsigned long task_util;
> >   6237          int i, recent_used_cpu;
> >   6238  
> >   6239          /*
> >   6240           * On asymmetric system, update task utilization because we will check
> >   6241           * that the task fits with cpu's capacity.
> >   6242           */
> > 
> > The original comment was a bit more clear...  Perhaps "On asymmetric
> > system[s], [record the] task utilization because we will check that the
> > task [can be done within] the cpu's capacity."
> 
> The comment "update task utilization because we will check ..." refers to
> sync_entity_load_avg()
> 
> > 
> >   6243          if (static_branch_unlikely(&sched_asym_cpucapacity)) {
> >   6244                  sync_entity_load_avg(&p->se);
> >   6245                  task_util = uclamp_task_util(p);
> >   6246          }
> > 
> > "task_util" is not initialized on the else path.
> 
> no need because it will not be used
> 
> > 
> >   6247  
> >   6248          if ((available_idle_cpu(target) || sched_idle_cpu(target)) &&
> >   6249              asym_fits_capacity(task_util, target))
> >                                        ^^^^^^^^^
> > Uninitialized variable warning.
> 
> asym_fits_capacity includes the same condition as above when we set task_util
> so task_util can't be used unintialize
> 
> static inline bool asym_fits_capacity(int task_util, int cpu)
> {
> 	if (static_branch_unlikely(&sched_asym_cpucapacity))
> 		return fits_capacity(task_util, capacity_of(cpu));
> 
> 	return true;

It's an interesting question, because unless the compiler makes this
inline, then it will lead to a KASan/syzbot warning at runtime.  The
function is, of course, marked as inline but the compiler, also of
course,  generally ignores those hints (use __always_inline if you want
the compiler to pay attention).  On the other hand, the compiler will
still probably inline it...  So this is *probably* not going to
lead to a runtime warning.

regards,
dan carpenter