[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [BUGFIX][PATCH 4/4] hvm_save_one: allow the 2nd instance to be fetched for PIC.



On 12/13/13 09:38, Jan Beulich wrote:
On 12.12.13 at 01:56, Don Slutz <dslutz@xxxxxxxxxxx> wrote:
@@ -114,20 +111,20 @@ int hvm_save_one(struct domain *d, uint16_t typecode, 
uint16_t instance,
      }
      else
      {
-        uint32_t off;
+        uint32_t off, add;
"add" is a pretty odd name for what this is being used for. Why
don't you use desc->length directly?
I picked the name when the 3rd part of the "for" was "off += add". During unit testing that did not work and so did not try and pick a new one. I could have added add2:

    const uint32_t add2 = sizeof (struct hvm_save_descriptor);

And then the last part of the for becomes "off += add + add2".

I can not use desc->length because:

save.c: In function 'hvm_save_one':
save.c:120:47: error: 'desc' undeclared (first use in this function)
save.c:120:47: note: each undeclared identifier is reported only once for each function it appears in

(It is only defined in the body of the for).

rv = -EBADSLT;
-        for (off = 0; off < ctxt.cur; off += hvm_sr_handlers[typecode].size) {
+        for (off = 0; off < ctxt.cur; off += add + sizeof (struct 
hvm_save_descriptor)) {
              struct hvm_save_descriptor *desc
                     = (struct hvm_save_descriptor *)&ctxt.data[off];
+            add = desc->length;
              if (instance == desc->instance) {
                  rv = 0;
                  if ( copy_to_guest(handle,
                                     ctxt.data
                                     + off
                                     + sizeof (struct hvm_save_descriptor),
-                                   hvm_sr_handlers[typecode].size
-                                   - sizeof (struct hvm_save_descriptor)) )
+                                   add) )
Further, the way this was done before means that for multi-
instance records all records would get copied out, but the
caller would have no way of knowing that (except by implying
that behavior, e.g. "known to be the case for PIC").
Not exactly.  Using the following adjustment to check-hvmctx (patch #1):

cb4d0e75a497f169c8419b30c1954f92bb8e29a8 Mon Sep 17 00:00:00 2001
From: Don Slutz <dslutz@xxxxxxxxxxx>
Date: Sat, 14 Dec 2013 19:31:16 -0500
Subject: [PATCH] check-hvmctx: add check for extra data under debug 8

Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
---
 tools/tests/check-hvmctx/check-hvmctx.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/tools/tests/check-hvmctx/check-hvmctx.c b/tools/tests/check-hvmctx/check-hvmctx.c
index 9b5c3aa..668b77a 100644
--- a/tools/tests/check-hvmctx/check-hvmctx.c
+++ b/tools/tests/check-hvmctx/check-hvmctx.c
@@ -574,7 +574,7 @@ int main(int argc, char **argv)
         default:
             if (xc_domain_hvm_getcontext_partial(
                     xch, domid, desc.typecode, desc.instance,
-                    tbuf, desc.length) != 0) {
+                    tbuf, len) != 0) {
fprintf(stderr, "Error: xc_domain_hvm_getcontext_partial: entry %i: type %u instance %u, length %u: ",
                         entry, (unsigned) desc.typecode,
                         (unsigned) desc.instance, (unsigned) desc.length);
@@ -582,6 +582,23 @@ int main(int argc, char **argv)
                 retval = 42;
                 memset(tbuf, 0xee, desc.length);
             }
+            if (debug & 0x08) {
+                int i;
+                int header = 1;
+
+                for (i = desc.length; i < len; i++) {
+                    if (tbuf[i]) {
+                        if (header) {
+                            header = 0;
+ printf("Error: entry %i: type %u instance %u, length %u extra data!\n",
+                                   entry, (unsigned) desc.typecode,
+ (unsigned) desc.instance, (unsigned) desc.length);
+                        }
+                        printf("[%03x] unexpected data=%02x\n",
+                               i, tbuf[i]);
+                    }
+                }
+            }
             ret = desc.length;
 #ifndef NOTCLEAN
             if (desc.typecode == HVM_SAVE_CODE(CPU))
--
1.7.11.7

and before this patch:

dcs-xen-51:~/xen/tools/tests/check-hvmctx>make clean;make CHECK_HVMCTX="-DDOPIC"
rm -f *.o check-hvmctx *~ .*.d
gcc -O1 -fno-omit-frame-pointer -m64 -g -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -D__XEN_TOOLS__ -MMD -MF .check-hvmctx.o.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -fno-optimize-sibling-calls -Werror -I/home/don/xen/tools/tests/check-hvmctx/../../../tools/libxc -I/home/don/xen/tools/tests/check-hvmctx/../../../tools/include -I/home/don/xen/tools/tests/check-hvmctx/../../../tools/include -I/home/don/xen/tools/tests/check-hvmctx/../../../tools/xenstore -I/home/don/xen/tools/tests/check-hvmctx/../../../tools/include -I/home/don/xen/tools/tests/check-hvmctx/../../../tools -DDOPIC -c -o check-hvmctx.o check-hvmctx.c gcc -o check-hvmctx check-hvmctx.o /home/don/xen/tools/tests/check-hvmctx/../../../tools/libxc/libxenctrl.so
dcs-xen-51:~/xen/tools/tests/check-hvmctx>sudo ./check-hvmctx 1 8
Check HVM save record vs partial for domain 1
Error: entry 8: type 3 instance 0, length 8 extra data!
[008] unexpected data=03
[00a] unexpected data=01
[00c] unexpected data=08
[010] unexpected data=01
[011] unexpected data=ff
[013] unexpected data=28
[016] unexpected data=0c
Error: xc_domain_hvm_getcontext_partial: entry 9: type 3 instance 1, length 8: Invalid argument
Error: entry 9: type 3 instance 1, length 8 mismatch!
    PIC: IRQ base 0x28, irr 0x1, imr 0xff, isr 0


I see that after the expected length (8), there is a struct hvm_save_descriptor (type 3 instance 1, length 8) followed by another HVM_SAVE_TYPE(PIC).
Which shows another shortcoming of the interface: There's no
buffer size being passed in from the caller, yet we have variable
size records. Which means latent data corruption in the caller.
This is not 100% correct. The libxl code for xc_domain_hvm_getcontext_partial does take a length:

/* Get just one element of the HVM guest context.
 * size must be >= HVM_SAVE_LENGTH(type) */
int xc_domain_hvm_getcontext_partial(xc_interface *xch,
                                     uint32_t domid,
                                     uint16_t typecode,
                                     uint16_t instance,
                                     void *ctxt_buf,
                                     uint32_t size)
{

and it gets embeded in


DECLARE_HYPERCALL_BOUNCE(ctxt_buf, size, XC_HYPERCALL_BUFFER_BOUNCE_OUT);

which is handle. I do not know that much about "XEN_GUEST_HANDLE_64(uint8) handle" and DECLARE_HYPERCALL_BOUNCE, but what my unit testing has shown me is that copy_to_guest(handle, <ptr>, <nr>) does only copy up to the size stored in handle. It looks to zero an unknown amount more (looks to be page sized). So there is more needed here.

Hence I think rather than complicating the logic here, we should
change the interface to pass a size in and back out, which will
not only avoid corrupting memory, but also allow the guest to
recognize multi-instance data being returned.
The size is passed in, just not passed out.  And the fact that the data is:

HVM_SAVE_TYPE(PIC)
struct hvm_save_descriptor
HVM_SAVE_TYPE(PIC)

Is strange to me. One descriptor for two entries. This was the primary reason I went the way I did. I am not saying that the interface should not change; I just do not see that as a bug fix type change.

     -Don Slutz
Jan


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


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.