Ian Campbell
2012-Jun-26 09:47 UTC
[PATCH] xen: add assertions 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 scribble over the heap. Add a couple of asserts to catch this in
the future.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Keir (Xen.org) <keir@xen.org>
Cc: Jan Beulich <JBeulich@suse.com>
---
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);
for_each_cpu(i, online)
{
+ ASSERT(i < nr_cpus);
if ( cpumask_test_cpu(i, &cpu_exclude_map) )
continue;
if ( (i == cpumask_first(per_cpu(cpu_sibling_mask, i))) &&
--
1.7.9.1
Jan Beulich
2012-Jun-26 10:16 UTC
Re: [PATCH] xen: add assertions in default_vcpu0_location to protect against broken masks
>>> On 26.06.12 at 11:47, Ian Campbell <ian.campbell@citrix.com> 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@citrix.com> > Cc: Keir (Xen.org) <keir@xen.org> > Cc: Jan Beulich <JBeulich@suse.com> > --- > 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).> 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. Jan> if ( cpumask_test_cpu(i, &cpu_exclude_map) ) > continue; > if ( (i == cpumask_first(per_cpu(cpu_sibling_mask, i))) && > -- > 1.7.9.1
Keir Fraser
2012-Jun-26 12:46 UTC
Re: [PATCH] xen: add assertions in default_vcpu0_location to protect against broken masks
On 26/06/2012 10:47, "Ian Campbell" <ian.campbell@citrix.com> 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.I''m happy with either or both the assertions you add. Acked-by: Keir Fraser <keir@xen.org>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Cc: Keir (Xen.org) <keir@xen.org> > Cc: Jan Beulich <JBeulich@suse.com> > --- > 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); > for_each_cpu(i, online) > { > + ASSERT(i < nr_cpus); > if ( cpumask_test_cpu(i, &cpu_exclude_map) ) > continue; > if ( (i == cpumask_first(per_cpu(cpu_sibling_mask, i))) &&
Ian Campbell
2012-Jun-26 13:17 UTC
Re: [PATCH] xen: add assertions in default_vcpu0_location to protect against broken masks
On Tue, 2012-06-26 at 11:16 +0100, Jan Beulich wrote:> >>> On 26.06.12 at 11:47, Ian Campbell <ian.campbell@citrix.com> 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@citrix.com> > > Cc: Keir (Xen.org) <keir@xen.org> > > Cc: Jan Beulich <JBeulich@suse.com> > > --- > > 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.
Jan Beulich
2012-Jun-26 13:54 UTC
Re: [PATCH] xen: add assertions in default_vcpu0_location to protect against broken masks
>>> On 26.06.12 at 15:17, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Tue, 2012-06-26 at 11:16 +0100, Jan Beulich wrote: >> >>> On 26.06.12 at 11:47, Ian Campbell <ian.campbell@citrix.com> 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@citrix.com> >> > Cc: Keir (Xen.org) <keir@xen.org> >> > Cc: Jan Beulich <JBeulich@suse.com> >> > --- >> > 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
Ian Campbell
2012-Jun-26 14:16 UTC
Re: [PATCH] xen: add assertions in default_vcpu0_location to protect against broken masks
On Tue, 2012-06-26 at 14:54 +0100, Jan Beulich wrote:> >>> On 26.06.12 at 15:17, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Tue, 2012-06-26 at 11:16 +0100, Jan Beulich wrote: > >> >>> On 26.06.12 at 11:47, Ian Campbell <ian.campbell@citrix.com> 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@citrix.com> > >> > Cc: Keir (Xen.org) <keir@xen.org> > >> > Cc: Jan Beulich <JBeulich@suse.com> > >> > --- > >> > 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@citrix.com> 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@citrix.com> Cc: Keir (Xen.org) <keir@xen.org> Cc: Jan Beulich <JBeulich@suse.com> --- 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