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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan


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