| Message ID | 20210922114432.1093340-1-penny.zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: fix broken tainted value in mark_page_free | expand |
Hi Penny, > On 22 Sep 2021, at 12:44, Penny Zheng <penny.zheng@arm.com> wrote: > > Commit 540a637c3410780b519fc055f432afe271f642f8 defines a new > helper mark_page_free to extract common codes, while it accidently > breaks the local variable "tainted". > > This patch fix it by letting mark_page_free() return bool of whether the > page is offlined and rename local variable "tainted" to "pg_offlined". > > Coverity ID: 1491872 > > Fixes: 540a637c3410780b519fc055f432afe271f642f8 > Signed-off-by: Penny Zheng <penny.zheng@arm.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > --- > v2 changes: > - rename local variable "tainted" to "pg_offlined", and make it bool > --- > xen/common/page_alloc.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 6142c7bb6a..5801358b4b 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -1380,8 +1380,10 @@ bool scrub_free_pages(void) > return node_to_scrub(false) != NUMA_NO_NODE; > } > > -static void mark_page_free(struct page_info *pg, mfn_t mfn) > +static bool mark_page_free(struct page_info *pg, mfn_t mfn) > { > + bool pg_offlined = false; > + > ASSERT(mfn_x(mfn) == mfn_x(page_to_mfn(pg))); > > /* > @@ -1405,7 +1407,7 @@ static void mark_page_free(struct page_info *pg, mfn_t mfn) > case PGC_state_offlining: > pg->count_info = (pg->count_info & PGC_broken) | > PGC_state_offlined; > - tainted = 1; > + pg_offlined = true; > break; > > default: > @@ -1425,6 +1427,8 @@ static void mark_page_free(struct page_info *pg, mfn_t mfn) > /* This page is not a guest frame any more. */ > page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner */ > set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY); > + > + return pg_offlined; > } > > /* Free 2^@order set of pages. */ > @@ -1433,7 +1437,7 @@ static void free_heap_pages( > { > unsigned long mask; > mfn_t mfn = page_to_mfn(pg); > - unsigned int i, node = phys_to_nid(mfn_to_maddr(mfn)), tainted = 0; > + unsigned int i, node = phys_to_nid(mfn_to_maddr(mfn)), pg_offlined = 0; > unsigned int zone = page_to_zone(pg); > > ASSERT(order <= MAX_ORDER); > @@ -1443,7 +1447,8 @@ static void free_heap_pages( > > for ( i = 0; i < (1 << order); i++ ) > { > - mark_page_free(&pg[i], mfn_add(mfn, i)); > + if ( mark_page_free(&pg[i], mfn_add(mfn, i)) ) > + pg_offlined = 1; > > if ( need_scrub ) > { > @@ -1517,7 +1522,7 @@ static void free_heap_pages( > > page_list_add_scrub(pg, node, zone, order, pg->u.free.first_dirty); > > - if ( tainted ) > + if ( pg_offlined ) > reserve_offlined_page(pg); > > spin_unlock(&heap_lock); > -- > 2.25.1 >
On 22.09.2021 13:48, Bertrand Marquis wrote: >> On 22 Sep 2021, at 12:44, Penny Zheng <penny.zheng@arm.com> wrote: >> >> Commit 540a637c3410780b519fc055f432afe271f642f8 defines a new >> helper mark_page_free to extract common codes, while it accidently >> breaks the local variable "tainted". >> >> This patch fix it by letting mark_page_free() return bool of whether the >> page is offlined and rename local variable "tainted" to "pg_offlined". >> >> Coverity ID: 1491872 >> >> Fixes: 540a637c3410780b519fc055f432afe271f642f8 >> Signed-off-by: Penny Zheng <penny.zheng@arm.com> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Acked-by: Jan Beulich <jbeulich@suse.com> Albeit I would have wished that ... >> @@ -1433,7 +1437,7 @@ static void free_heap_pages( >> { >> unsigned long mask; >> mfn_t mfn = page_to_mfn(pg); >> - unsigned int i, node = phys_to_nid(mfn_to_maddr(mfn)), tainted = 0; >> + unsigned int i, node = phys_to_nid(mfn_to_maddr(mfn)), pg_offlined = 0; ... this would have become properly bool as well. Jan
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 6142c7bb6a..5801358b4b 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -1380,8 +1380,10 @@ bool scrub_free_pages(void) return node_to_scrub(false) != NUMA_NO_NODE; } -static void mark_page_free(struct page_info *pg, mfn_t mfn) +static bool mark_page_free(struct page_info *pg, mfn_t mfn) { + bool pg_offlined = false; + ASSERT(mfn_x(mfn) == mfn_x(page_to_mfn(pg))); /* @@ -1405,7 +1407,7 @@ static void mark_page_free(struct page_info *pg, mfn_t mfn) case PGC_state_offlining: pg->count_info = (pg->count_info & PGC_broken) | PGC_state_offlined; - tainted = 1; + pg_offlined = true; break; default: @@ -1425,6 +1427,8 @@ static void mark_page_free(struct page_info *pg, mfn_t mfn) /* This page is not a guest frame any more. */ page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner */ set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY); + + return pg_offlined; } /* Free 2^@order set of pages. */ @@ -1433,7 +1437,7 @@ static void free_heap_pages( { unsigned long mask; mfn_t mfn = page_to_mfn(pg); - unsigned int i, node = phys_to_nid(mfn_to_maddr(mfn)), tainted = 0; + unsigned int i, node = phys_to_nid(mfn_to_maddr(mfn)), pg_offlined = 0; unsigned int zone = page_to_zone(pg); ASSERT(order <= MAX_ORDER); @@ -1443,7 +1447,8 @@ static void free_heap_pages( for ( i = 0; i < (1 << order); i++ ) { - mark_page_free(&pg[i], mfn_add(mfn, i)); + if ( mark_page_free(&pg[i], mfn_add(mfn, i)) ) + pg_offlined = 1; if ( need_scrub ) { @@ -1517,7 +1522,7 @@ static void free_heap_pages( page_list_add_scrub(pg, node, zone, order, pg->u.free.first_dirty); - if ( tainted ) + if ( pg_offlined ) reserve_offlined_page(pg); spin_unlock(&heap_lock);
Commit 540a637c3410780b519fc055f432afe271f642f8 defines a new helper mark_page_free to extract common codes, while it accidently breaks the local variable "tainted". This patch fix it by letting mark_page_free() return bool of whether the page is offlined and rename local variable "tainted" to "pg_offlined". Coverity ID: 1491872 Fixes: 540a637c3410780b519fc055f432afe271f642f8 Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- v2 changes: - rename local variable "tainted" to "pg_offlined", and make it bool --- xen/common/page_alloc.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)