[PATCH] gnttab_setup_table error case

[PATCH] gnttab_setup_table error case

[Diego Ongaro]

gnttab_setup_table should set an error status code if the gmfn it gets
for a grant table page is invalid.

I ran into this issue when I tried to set up the grant table during hvm
domain creation, and it caused a BUG_ON later down the line. With this
patch, the hypercall will gracefully fail instead.

Signed-off-by: Diego Ongaro <diego.ongaro@citrix.com>
---
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -913,8 +913,15 @@ gnttab_setup_table(
     op.status = GNTST_okay;
     for ( i = 0; i < op.nr_frames; i++ )
     {
         gmfn = gnttab_shared_gmfn(d, d->grant_table, i);
+        if ( gmfn == -1 )
+        {
+            gdprintk(XENLOG_INFO,
+                    "grant table gmfn unavailable\n");
+            op.status = GNTST_general_error;
+            goto out3;
+        }
         (void)copy_to_guest_offset(op.frame_list, i, &gmfn, 1);
     }
 
  out3:

Re: [PATCH] gnttab_setup_table error case

[Qing He]

On Wed, 2008-07-30 at 14:17 +0100, Diego Ongaro wrote:
> gnttab_setup_table should set an error status code if the gmfn it gets
> for a grant table page is invalid.
> 
> I ran into this issue when I tried to set up the grant table during hvm
> domain creation, and it caused a BUG_ON later down the line. With this
> patch, the hypercall will gracefully fail instead.

Do you use 32bit guest on top of 64bit hypervisor? In that case
op.frame_list has different size in the hypervisor and the guest, and
the BUG_ON is probably triggered because of the hypervisor ruining the
stack.

Just curious, but why we want to call setup_table in HVM in the first
place. Since HVM has its isolated address space, it will always fail. It
seems that setup_table is added to HVM because of this:
  http://lists.xensource.com/archives/html/xen-devel/2008-03/msg00927.html

At that time, calling setup_table is a guest bug and I think it's already
fixed by removing that call.

So, can we:
1. let setup_table return -ENOSYS again in HVM? or
2. call something like compat_grant_table_op instead in 32/64? or
2. add a stub in hvm_grant_table_op so that it doesn't bother calling
   {do,compat}_grant_table_op at all?

Thanks,
Qing

> 
> Signed-off-by: Diego Ongaro <diego.ongaro@citrix.com>
> ---
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -913,8 +913,15 @@ gnttab_setup_table(
>      op.status = GNTST_okay;
>      for ( i = 0; i < op.nr_frames; i++ )
>      {
>          gmfn = gnttab_shared_gmfn(d, d->grant_table, i);
> +        if ( gmfn == -1 )
> +        {
> +            gdprintk(XENLOG_INFO,
> +                    "grant table gmfn unavailable\n");
> +            op.status = GNTST_general_error;
> +            goto out3;
> +        }
>          (void)copy_to_guest_offset(op.frame_list, i, &gmfn, 1);
>      }
>  
>   out3:
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] gnttab_setup_table error case

[Samuel Thibault]

Qing He, le Thu 31 Jul 2008 10:08:28 +0800, a écrit :
> On Wed, 2008-07-30 at 14:17 +0100, Diego Ongaro wrote:
> > gnttab_setup_table should set an error status code if the gmfn it gets
> > for a grant table page is invalid.
> > 
> > I ran into this issue when I tried to set up the grant table during hvm
> > domain creation, and it caused a BUG_ON later down the line. With this
> > patch, the hypercall will gracefully fail instead.
> 
> Do you use 32bit guest on top of 64bit hypervisor?

No, just 64/64.

> Just curious, but why we want to call setup_table in HVM in the first
> place. Since HVM has its isolated address space, it will always fail.

That was for his personal project.  The call was made from dom0.  The
problem is that it triggers a BUG_ON, which we do not really want to
happen :)

> 1. let setup_table return -ENOSYS again in HVM?

That could make sense.

Samuel

Re: [PATCH] gnttab_setup_table error case

[Diego Ongaro]

Samuel Thibault wrote:
> Qing He, le Thu 31 Jul 2008 10:08:28 +0800, a écrit :
>> On Wed, 2008-07-30 at 14:17 +0100, Diego Ongaro wrote:
>>> gnttab_setup_table should set an error status code if the gmfn it gets
>>> for a grant table page is invalid.
>>>
>>> I ran into this issue when I tried to set up the grant table during hvm
>>> domain creation, and it caused a BUG_ON later down the line. With this
>>> patch, the hypercall will gracefully fail instead.
>> Do you use 32bit guest on top of 64bit hypervisor?
> 
> No, just 64/64.

Well, no, it was 32 on 64 :)

>> Just curious, but why we want to call setup_table in HVM in the first
>> place. Since HVM has its isolated address space, it will always fail.
> 
> That was for his personal project.  The call was made from dom0.  The
> problem is that it triggers a BUG_ON, which we do not really want to
> happen :)

Keir's xen-unstable.hg cs 18177:9ee2e41a68a1 simply removed the
problematic BUG_ON, so at least userspace can now handle the error.

-Diego