[05/12] libxenguest: complete loops in xc_map_domain_meminfo()

Commit Message

Jan Beulich June 25, 2021, 1:19 p.m. UTC

minfo->p2m_size may have more than 31 significant bits. Change the
induction variable to unsigned long, and (largely for signed-ness
consistency) a helper variable to unsigned int.

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Andrew Cooper June 25, 2021, 6:30 p.m. UTC | #1

On 25/06/2021 14:19, Jan Beulich wrote:
> minfo->p2m_size may have more than 31 significant bits. Change the
> induction variable to unsigned long, and (largely for signed-ness
> consistency) a helper variable to unsigned int.
>
> Signed-off-by: Jan Beulich <[e-mail]>
>
> --- a/tools/libs/guest/xg_domain.c
> +++ b/tools/libs/guest/xg_domain.c
> @@ -40,7 +40,7 @@ int xc_map_domain_meminfo(xc_interface *
>      xc_dominfo_t info;
>      shared_info_any_t *live_shinfo;
>      xen_capabilities_info_t xen_caps = "";
> -    int i;
> +    unsigned long i;
>  
>      /* Only be initialized once */
>      if ( minfo->pfn_type || minfo->p2m_table )
> @@ -116,12 +116,12 @@ int xc_map_domain_meminfo(xc_interface *
>      /* Retrieve PFN types in batches */
>      for ( i = 0; i < minfo->p2m_size ; i+=1024 )
>      {
> -        int count = ((minfo->p2m_size - i ) > 1024 ) ?
> -                        1024: (minfo->p2m_size - i);
> +        unsigned int count = ((minfo->p2m_size - i) > 1024) ?
> +                             1024 : (minfo->p2m_size - i);

min().

Otherwise, Acked-by: Andrew Cooper <[e-mail]>

This whole infrastructure is almost abandoned, and broken.  Its used by
xen-mfndump (debugging only) and xen-hptool mem-offline.

The mem-offline functionally cannot possibly work usefully.  It is PV
only, despite not having an HVM check, and in particular reads the dead
page in an attempt to restore the contents elsewhere.  There is also no
thought given to writes from outside sources, such as DMA from
passthrough or a different dom0 foreign mapping.

This is perhaps ok as an academic demonstration of "can I shuffle memory
behind an alive VM in ideal circumstances", but will be killed by the
dom0 kernel if you ever try running it to resolve a real memory error on
a VM, because there is no possibility of recovering the data.

The mem-offline functionality needs deleting.  It isn't production
ready, and can't credibly be made so.

~Andrew

Jan Beulich June 28, 2021, 8:53 a.m. UTC | #2

On 25.06.2021 20:30, Andrew Cooper wrote:
> On 25/06/2021 14:19, Jan Beulich wrote:
>> minfo->p2m_size may have more than 31 significant bits. Change the
>> induction variable to unsigned long, and (largely for signed-ness
>> consistency) a helper variable to unsigned int.
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
>>
>> --- a/tools/libs/guest/xg_domain.c
>> +++ b/tools/libs/guest/xg_domain.c
>> @@ -40,7 +40,7 @@ int xc_map_domain_meminfo(xc_interface *
>>      xc_dominfo_t info;
>>      shared_info_any_t *live_shinfo;
>>      xen_capabilities_info_t xen_caps = "";
>> -    int i;
>> +    unsigned long i;
>>  
>>      /* Only be initialized once */
>>      if ( minfo->pfn_type || minfo->p2m_table )
>> @@ -116,12 +116,12 @@ int xc_map_domain_meminfo(xc_interface *
>>      /* Retrieve PFN types in batches */
>>      for ( i = 0; i < minfo->p2m_size ; i+=1024 )
>>      {
>> -        int count = ((minfo->p2m_size - i ) > 1024 ) ?
>> -                        1024: (minfo->p2m_size - i);
>> +        unsigned int count = ((minfo->p2m_size - i) > 1024) ?
>> +                             1024 : (minfo->p2m_size - i);
> 
> min().

min() using 1024UL or MIN()? (I'll use the former unless you tell
me otherwise.)

> Otherwise, Acked-by: Andrew Cooper <[e-mail]>

Thanks.

> This whole infrastructure is almost abandoned, and broken.  Its used by
> xen-mfndump (debugging only) and xen-hptool mem-offline.
> 
> The mem-offline functionally cannot possibly work usefully.  It is PV
> only, despite not having an HVM check, and in particular reads the dead
> page in an attempt to restore the contents elsewhere.  There is also no
> thought given to writes from outside sources, such as DMA from
> passthrough or a different dom0 foreign mapping.
> 
> This is perhaps ok as an academic demonstration of "can I shuffle memory
> behind an alive VM in ideal circumstances", but will be killed by the
> dom0 kernel if you ever try running it to resolve a real memory error on
> a VM, because there is no possibility of recovering the data.
> 
> The mem-offline functionality needs deleting.  It isn't production
> ready, and can't credibly be made so.

I definitely agree; I'm merely trying to address an anomaly found
while auditing the code for certain properties, without any claim
that afterwards any of this would really work.

Jan

Patch

--- a/tools/libs/guest/xg_domain.c
+++ b/tools/libs/guest/xg_domain.c
@@ -40,7 +40,7 @@  int xc_map_domain_meminfo(xc_interface *
     xc_dominfo_t info;
     shared_info_any_t *live_shinfo;
     xen_capabilities_info_t xen_caps = "";
-    int i;
+    unsigned long i;
 
     /* Only be initialized once */
     if ( minfo->pfn_type || minfo->p2m_table )
@@ -116,12 +116,12 @@  int xc_map_domain_meminfo(xc_interface *
     /* Retrieve PFN types in batches */
     for ( i = 0; i < minfo->p2m_size ; i+=1024 )
     {
-        int count = ((minfo->p2m_size - i ) > 1024 ) ?
-                        1024: (minfo->p2m_size - i);
+        unsigned int count = ((minfo->p2m_size - i) > 1024) ?
+                             1024 : (minfo->p2m_size - i);
 
         if ( xc_get_pfn_type_batch(xch, domid, count, minfo->pfn_type + i) )
         {
-            PERROR("Could not get %d-eth batch of PFN types", (i+1)/1024);
+            PERROR("Could not get batch %lu of PFN types", (i + 1) / 1024);
             goto failed;
         }
     }