Re: [BUGFIX][PATCH 3/4] hvm_save_one: return correct data.

[Don Slutz <dslutz@verizon.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2608 bytes --]

On 12/16/13 03:17, Jan Beulich wrote:
>>>> On 15.12.13 at 17:51, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 15/12/2013 00:29, Don Slutz wrote:
>>> I think I have corrected all coding errors (please check again). And
>>> done all requested changes.  I did add the reviewed by (not sure if I
>>> should since this changes a large part of the patch, but they are all
>>> what Jan said).
>>>
>>> I have unit tested it and it appears to work the same as the previous
>>> version (as expected).
>>>
>>> Here is the new version, also attached.
>>>
>>>  From e0e8f5246ba492b153884cea93bfe753f1b0782e Mon Sep 17 00:00:00 2001
>>> From: Don Slutz <dslutz@verizon.com>
>>> Date: Tue, 12 Nov 2013 08:22:53 -0500
>>> Subject: [PATCH v2 3/4] hvm_save_one: return correct data.
>>>
>>> It is possible that hvm_sr_handlers[typecode].save does not use all
>>> the provided room.  In that case, using:
>>>
>>>     instance * hvm_sr_handlers[typecode].size
>>>
>>> does not select the correct instance.  Add code to search for the
>>> correct instance.
>>>
>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> but this fairs no better at selecting the correct subset in the case
>> that less data than hvm_sr_handlers[typecode].size is written by
>> hvm_sr_handlers[typecode].save.
> Oh, yes, indeed.
>
>> It always increments by 'size' bytes, and will only copy the data back
>> if the bytes under desc->instance happen to match the instance we are
>> looking for.
>>
>> The only solution I can see is that for the per-vcpu records, the save
>> functions get refactored to take an instance ID, and only save their
>> specific instance.
> I don't see why you shouldn't be able to look at the descriptor
> instead - that one does have the correct size, doesn't it?
>
> Jan
>
Attached is v3 of this.  It is basically a merge of patch #3 and patch #4 with cleanups.

This is what I said in:

http://lists.xen.org/archives/html/xen-devel/2013-12/msg02216.html

and Andrew replied in:

http://lists.xen.org/archives/html/xen-devel/2013-12/msg02217.html

and the RFC is:

http://lists.xen.org/archives/html/xen-devel/2013-12/msg02223.html

to which:

http://lists.xen.org/archives/html/xen-devel/2013-12/msg02270.html

(from this):

        IMHO this is obviously not 4.4 material at this stage. Apart from
        anything else we've been managing to release with these short comings
        for many years.

    Indeed. -George

I feel that the attached bugfix patch is simple enough to make it into 4.4 and also be back ported to stable branches.

    -Don Slutz







[-- Attachment #1.2: Type: text/html, Size: 4354 bytes --]

[-- Attachment #2: 0001-hvm_save_one-return-correct-data.patch --]
[-- Type: text/x-patch, Size: 2370 bytes --]

>From 07897bd0d4a680df03421c0eab96cfa41de2d9f6 Mon Sep 17 00:00:00 2001
From: Don Slutz <dslutz@verizon.com>
Date: Tue, 12 Nov 2013 08:22:53 -0500
Subject: [BUGFIX][PATCH v3 1/1] hvm_save_one: return correct data.

It is possible that hvm_sr_handlers[typecode].save does not use all
the provided room.  Also it can use variable sized records.  In both
cases, using:

   instance * hvm_sr_handlers[typecode].size

does not select the correct instance.  Add code to search for the
correct instance.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
changes v2 to v3: merge in patch #4.
changes v1 to v2: fix coding style and coding issues.

 xen/common/hvm/save.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/xen/common/hvm/save.c b/xen/common/hvm/save.c
index de76ada..a7e0edc 100644
--- a/xen/common/hvm/save.c
+++ b/xen/common/hvm/save.c
@@ -98,9 +98,6 @@ int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance,
     else 
         sz = hvm_sr_handlers[typecode].size;
     
-    if ( (instance + 1) * hvm_sr_handlers[typecode].size > sz )
-        return -EINVAL;
-
     ctxt.size = sz;
     ctxt.data = xmalloc_bytes(sz);
     if ( !ctxt.data )
@@ -112,13 +109,26 @@ int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance,
                d->domain_id, typecode);
         rv = -EFAULT;
     }
-    else if ( copy_to_guest(handle,
-                            ctxt.data 
-                            + (instance * hvm_sr_handlers[typecode].size) 
-                            + sizeof (struct hvm_save_descriptor), 
-                            hvm_sr_handlers[typecode].size
-                            - sizeof (struct hvm_save_descriptor)) )
-        rv = -EFAULT;
+    else
+    {
+        uint32_t off;
+        struct hvm_save_descriptor *desc;
+
+        rv = -EBADSLT;
+        for ( off = 0; off < ctxt.cur; off += desc->length )
+        {
+            desc = (void *)ctxt.data + off;
+            /* Move past header */
+            off +=  sizeof(*desc);
+            if ( instance == desc->instance )
+            {
+                rv = 0;
+                if ( copy_to_guest(handle, ctxt.data + off, desc->length) )
+                    rv = -EFAULT;
+                break;
+            }
+        }
+    }
 
     xfree(ctxt.data);
     return rv;
-- 
1.8.4


[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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