linux-kernel - Re: Scheduler regression from caffcdd8d27ba78730d5540396ce72ad022aff2c

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140717123502.GL19379@twins.programming.kicks-ass.net>
Date:	Thu, 17 Jul 2014 14:35:02 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Dietmar Eggemann <dietmar.eggemann@....com>
Cc:	Bruno Wolff III <bruno@...ff.to>, Josh Boyer <jwboyer@...hat.com>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: Scheduler regression from
 caffcdd8d27ba78730d5540396ce72ad022aff2c

On Thu, Jul 17, 2014 at 01:23:51PM +0200, Dietmar Eggemann wrote:
> On 17/07/14 11:04, Peter Zijlstra wrote:
> >On Thu, Jul 17, 2014 at 10:57:55AM +0200, Dietmar Eggemann wrote:
> >>There is also the possibility that the memory for sched_group sg is not
> >>(completely) zeroed out:
> >>
> >>   sg = kzalloc_node(sizeof(struct sched_group) + cpumask_size(),
> >>			GFP_KERNEL, cpu_to_node(j));
> >>
> >>
> >>   struct sched_group {
> >>	...
> >>	 * NOTE: this field is variable length. (Allocated dynamically
> >>	 * by attaching extra space to the end of the structure,
> >>	 * depending on how many CPUs the kernel has booted up with)
> >>	 */
> >>	unsigned long cpumask[0];
> >
> >well kZalloc should Zero the entire allocated size, and the specified
> >size very much includes the cpumask size as per:
> >   sizeof(struct sched_group) + cpumask_size()
> 
> Yes, I think so too.
> 
> >
> >But yeah, I'm also a bit puzzled why this goes bang. Makes we worry we
> >scribble it somewhere or so.
> >
> 
> But then, this must be happening in build_sched_domains() between
> __visit_domain_allocation_hell()->__sdt_alloc() and build_sched_groups().
> 
> 
> Couldn't catch this phenomena by adding a fake SMT level (just a copy of the
> real MC level) to my ARM TC2 (dual cluster dual/triple core, no
> hyper-threading) to provoke sd degenerate. It does not show the issue and MC
> level gets degenerated nicely. Might not be the real example since SMT and
> MC are using the same cpu mask here).

Yeah, obviously my machines didn't trigger this either, and afaik none
of Ingo's did either.

In any case, can someone who can trigger this run with the below; its
'clean' for me, but supposedly you'll trigger a FAIL somewhere.

It includes the cpumask_clear() so your machines should boot, albeit
with a noisy dmesg :-)

---
 kernel/sched/core.c | 16 ++++++++++++++++
 lib/vsprintf.c      |  5 +++++
 2 files changed, 21 insertions(+)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 7bc599dc4aa4..1c140057db12 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5857,6 +5857,17 @@ build_sched_groups(struct sched_domain *sd, int cpu)
 			continue;
 
 		group = get_group(i, sdd, &sg);
+
+		if (!cpumask_empty(sched_group_cpus(sg)))
+			printk("%s: FAIL\n", __func__);
+
+		printk("%s: got group %p with cpus: %pc\n",
+				__func__,
+				sg,
+				sched_group_cpus(sg));
+
+		cpumask_clear(sched_group_cpus(sg));
+
 		cpumask_setall(sched_group_mask(sg));
 
 		for_each_cpu(j, span) {
@@ -6418,6 +6429,11 @@ static int __sdt_alloc(const struct cpumask *cpu_map)
 			if (!sg)
 				return -ENOMEM;
 
+			printk("%s: allocated %p with cpus: %pc\n",
+					__func__,
+					sg,
+					sched_group_cpus(sg));
+
 			sg->next = sg;
 
 			*per_cpu_ptr(sdd->sg, j) = sg;
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 6fe2c84eb055..ac22c46fd6d0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -28,6 +28,7 @@
 #include <linux/ioport.h>
 #include <linux/dcache.h>
 #include <linux/cred.h>
+#include <linux/cpumask.h>
 #include <net/addrconf.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
@@ -1250,6 +1251,7 @@ int kptr_restrict __read_mostly;
  *           (default assumed to be phys_addr_t, passed by reference)
  * - 'd[234]' For a dentry name (optionally 2-4 last components)
  * - 'D[234]' Same as 'd' but for a struct file
+ * - 'c' For a cpumask list
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
@@ -1389,6 +1391,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return dentry_name(buf, end,
 				   ((const struct file *)ptr)->f_path.dentry,
 				   spec, fmt);
+	case 'c':
+		return buf + cpulist_scnprintf(buf, end - buf, ptr);
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
@@ -1635,6 +1639,7 @@ int format_decode(const char *fmt, struct printf_spec *spec)
  *   case.
  * %*ph[CDN] a variable-length hex string with a separator (supports up to 64
  *           bytes of the input)
+ * %pc print a cpumask as comma-separated list
  * %n is ignored
  *
  * ** Please update Documentation/printk-formats.txt when making changes **

Content of type "application/pgp-signature" skipped