diff mbox

[Xen-devel,V2,1/2] x86/hvm: Add check when register io handler

Message ID 1463168217-16080-2-git-send-email-suravee.suthikulpanit@amd.com
State New
Headers show

Commit Message

Suthikulpanit, Suravee May 13, 2016, 7:36 p.m. UTC 
From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

At the time of registering HVM I/O handler, the HVM domain might
not have been initialized, which means the hvm_domain.io_handler
would be NULL. In the hvm_next_io_handler(), this should be checked
before returning and referencing the array. Also, the io_handler_count
should only be incremented on success.

So, this patch adds error handling in hvm_next_io_handler.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/arch/x86/hvm/intercept.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Suthikulpanit, Suravee May 16, 2016, 4:03 p.m. UTC | #1 
Hi Paul,

On 05/16/2016 03:01 AM, Paul Durrant wrote:
>> -----Original Message-----
>> >From:suravee.suthikulpanit@amd.com
>> >[mailto:suravee.suthikulpanit@amd.com]
>> >Sent: 13 May 2016 20:37
>> >To:xen-devel@lists.xen.org; George Dunlap;jbeulich@suse.com
>> >Cc: Paul Durrant; Suravee Suthikulpanit; Suravee Suthikulpanit
>> >Subject: [PATCH V2 1/2] x86/hvm: Add check when register io handler
>> >
>> >From: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com>
>> >
>> >At the time of registering HVM I/O handler, the HVM domain might
>> >not have been initialized,
> I/O handler registration is internal to Xen so any caller that attempt to register before domain initialization should be removed and replaced with one that does it at the right time.

Ok. I'll just remove that call for now.

>> >which means the hvm_domain.io_handler
>> >would be NULL. In the hvm_next_io_handler(), this should be checked
>> >before returning and referencing the array. Also, the io_handler_count
>> >should only be incremented on success.
>> >
>> >So, this patch adds error handling in hvm_next_io_handler.
>> >
> This isn't necessary. An ASSERT would be preferable so that buggy callers can be easily caught.

Ok, I'll update the patch to ASSERT() and send it out. Although, just 
want to make sure that you think it should really be doing assert and 
not warning + handling error? It seems quite aggressive to crash the 
hypervisor simply because some io handler are not properly call.

Thanks,
Suravee
diff mbox

Patch

diff --git a/xen/arch/x86/hvm/intercept.c b/xen/arch/x86/hvm/intercept.c
index 7096d74..b837f5f 100644
--- a/xen/arch/x86/hvm/intercept.c
+++ b/xen/arch/x86/hvm/intercept.c
@@ -248,7 +248,10 @@  int hvm_io_intercept(ioreq_t *p)
 
 struct hvm_io_handler *hvm_next_io_handler(struct domain *d)
 {
-    unsigned int i = d->arch.hvm_domain.io_handler_count++;
+    unsigned int i = d->arch.hvm_domain.io_handler_count;
+
+    if ( !d->arch.hvm_domain.io_handler )
+        return NULL;
 
     if ( i == NR_IO_HANDLERS )
     {
@@ -256,6 +259,7 @@  struct hvm_io_handler *hvm_next_io_handler(struct domain *d)
         return NULL;
     }
 
+    d->arch.hvm_domain.io_handler_count++;
     return &d->arch.hvm_domain.io_handler[i];
 }