[PATCH] xen: fix broken tainted value in mark_page_free

[PATCH] xen: fix broken tainted value in mark_page_free

[Penny Zheng]

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(-)

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

Re: [PATCH] xen: fix broken tainted value in mark_page_free

[Bertrand Marquis]

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
> 

Re: [PATCH] xen: fix broken tainted value in mark_page_free

[Jan Beulich]

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

Re: [PATCH] xen: fix broken tainted value in mark_page_free

[Jan Beulich]

On 17.09.2021 04:48, Penny Zheng wrote:
> Commit 540a637c3410780b519fc055f432afe271f642f8 defines a new
> helper mark_page_free to extract common codes, but it broke the
> local variable "tainted", revealed by Coverity ID 1491872.

Instead of mentioning the ID here, please ...

> This patch let mark_page_free() return boolean value of variable
> "tainted" and rename local variable "tainted" to "pg_tainted"
> to tell the difference from the global variable of the same name.

add a tag-like instance here, as we do elsewhere:

Coverity ID: 1491872

> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>  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 b9441cb06f..c6f48f7a97 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)
>  {
> +    unsigned int pg_tainted = 0;

bool please, considering ...

> @@ -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_tainted = 1;
>          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_tainted;
>  }

... this. Also both here and ...

> @@ -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_tainted = 0;

... here I don't see why you stick to using "tainted" in the name
when the goal is to ...

> @@ -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_tainted )
>          reserve_offlined_page(pg);

... control the call to this function: "offline" or "offlined"
would seem quite a bit more to the point, getting us further away
from the global meaning of "tainted".

Also please follow patch submission rules: To the list, with (all)
maintainers Cc-ed.

Jan

[PATCH] xen: fix broken tainted value in mark_page_free

[Penny Zheng]

Commit 540a637c3410780b519fc055f432afe271f642f8 defines a new
helper mark_page_free to extract common codes, but it broke the
local variable "tainted", revealed by Coverity ID 1491872.

This patch let mark_page_free() return boolean value of variable
"tainted" and rename local variable "tainted" to "pg_tainted"
to tell the difference from the global variable of the same name.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 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 b9441cb06f..c6f48f7a97 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)
 {
+    unsigned int pg_tainted = 0;
+
     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_tainted = 1;
         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_tainted;
 }
 
 /* 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_tainted = 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_tainted = 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_tainted )
         reserve_offlined_page(pg);
 
     spin_unlock(&heap_lock);
-- 
2.25.1