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 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?

> 
> >      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.

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...

Ian.


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