Re: [Xen-devel] [PATCH] xen: add assertions in default_vcpu0_location to protect against broken masks

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-06-26 at 14:54 +0100, Jan Beulich wrote:
> >>> On 26.06.12 at 15:17, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> > On Tue, 2012-06-26 at 11:16 +0100, Jan Beulich wrote:
> >> >>> On 26.06.12 at 11:47, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
> >> > When setting up the cpu sibling/etc masks on ARM I accidentally and
> >> > incorrectly omitted a CPU from it's own sibling mask which caused this
> >> > function to scribble over the heap. Add a couple of asserts to catch 
> >> > this 
> > in
> >> > the future.
> >> > 
> >> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> >> > Cc: Keir (Xen.org) <keir@xxxxxxx>
> >> > Cc: Jan Beulich <JBeulich@xxxxxxxx>
> >> > ---
> >> >  xen/common/domctl.c |    2 ++
> >> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >> > 
> >> > diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> >> > index 9f1a9ad..c1acd1d 100644
> >> > --- a/xen/common/domctl.c
> >> > +++ b/xen/common/domctl.c
> >> > @@ -190,10 +190,12 @@ static unsigned int 
> >> > default_vcpu0_location(cpumask_t 
> >> > *online)
> >> >       */
> >> >      cpumask_copy(&cpu_exclude_map, per_cpu(cpu_sibling_mask, 0));
> >> >      cpu = cpumask_first(&cpu_exclude_map);
> >> > +    ASSERT(cpu < nr_cpus);
> >> >      if ( cpumask_weight(&cpu_exclude_map) > 1 )
> >> >          cpu = cpumask_next(cpu, &cpu_exclude_map);
> >> 
> >> You may want to add another ASSERT() here in case you care
> >> about the difference between nr_cpus and nr_cpu_ids. Or move
> >> the ASSERT() here if the difference is insignificant (as I would
> >> think), as cpumask_next() invokes cpumask_check() (see also
> >> below).
> > 
> > It is possible to get through this code without calling
> > cpumask_next(cpu, ..) though, I think, due to the if.
> > 
> > Perhaps
> >     ASSERT(cpu < nr_cpu_ids);
> > after the if would be the best option?
> 
> That's what I meant with "here" in my first response.

Sorry, I thought you meant inside the if, for some reason...

> 
> >> >      for_each_cpu(i, online)
> >> >      {
> >> > +        ASSERT(i < nr_cpus);
> >> 
> >> This one is almost redundant with the one in cpumask_check()
> >> called from cpumask_test_cpu() (and the difference between
> >> using nr_cpu_ids and nr_cpus doesn't seem relevant here), so
> >> I'd recommend going with just the single earlier addition.
> > 
> > If mask is invalid you can the first iteration with the bad i (from
> > cpumask_first, which doesn't have any checks), which may scribble off
> > the end of the cnt array.
> 
> Immediately after the ASSERT() here there is an invocation of
> cpumask_test_cpu(), which itself uses cpumask_check(). That's
> what the added ASSERT() is redundant with.

Ah, I was confused because it was on a different mask -- but of course
for the purposes of that assert it doesn't matter which mask it is.

> > I'm not sure why I wasn't hitting an assert from the subsequent
> > cpumask_next, but I wasn't.
> > 
> > If you are still convinced this ASSERT isn't worth it I'll drop it, I've
> > found my bug now and it'd be a pretty uncommon one to repeat...
> 
> Please do.

Patch below.

My memory wrt heap corruption was faulty -- actually it was just that
this function was returning a cpu > nr_cpu_ids which caused a later
function to access an invalid per-cpu segment -- which just looked like
heap corruption when I first started investigating it. I've updated the
comment to reflect that.

8<---------------------------------------------------------------

>From 02d59965931509b9eb8e1e69504a0b09cc9765c5 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@xxxxxxxxxx>
Date: Tue, 26 Jun 2012 09:44:34 +0000
Subject: [PATCH] xen: add assertion in default_vcpu0_location to protect
 against broken masks

When setting up the cpu sibling/etc masks on ARM I accidentally and
incorrectly omitted a CPU from it's own sibling mask which caused this
function to return an invalid cpu number which caused errors later when we
tried to access per_cpu data for that invalid cpu.

Add an assert to catch this in the future.

Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
Cc: Keir (Xen.org) <keir@xxxxxxx>
Cc: Jan Beulich <JBeulich@xxxxxxxx>
---
 xen/common/domctl.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 9f1a9ad..7ca6b08 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -192,6 +192,7 @@ static unsigned int default_vcpu0_location(cpumask_t 
*online)
     cpu = cpumask_first(&cpu_exclude_map);
     if ( cpumask_weight(&cpu_exclude_map) > 1 )
         cpu = cpumask_next(cpu, &cpu_exclude_map);
+    ASSERT(cpu < nr_cpu_ids);
     for_each_cpu(i, online)
     {
         if ( cpumask_test_cpu(i, &cpu_exclude_map) )
-- 
1.7.9.1





_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel