Re: [Xen-devel] [PATCH v2] xen: fail gnttab_grow_table() in case of missing allocations

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/09/17 11:51, Juergen Gross wrote:
> In case gnttab_grow_table() is being called without
> grant_table_set_limits() having been called for the domain, e.g. in
> case of a toolstack error, fail the function instead of crashing the
> system.
>
> While at it let gnttab_grow_table() return a proper error code instead
> of 1 for success.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> V2:
> - use gprintk() instead of gdprintk() (Roger Pau Monne)
> - let gnttab_grow_table() return an error code (Andrew Cooper)
> ---
>  xen/common/grant_table.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 71706f5cba..144fa7c50f 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1669,7 +1669,11 @@ gnttab_grow_table(struct domain *d, unsigned int 
> req_nr_frames)
>      struct grant_table *gt = d->grant_table;
>      unsigned int i, j;
>  
> -    ASSERT(gt->active);
> +    if ( unlikely(!gt->active) )
> +    {
> +        gprintk(XENLOG_WARNING, "grant_table_set_limits() call missing.\n");

Unnecessary .

> +        return -ENODEV;
> +    }
>  
>      if ( req_nr_frames < INITIAL_NR_GRANT_FRAMES )
>          req_nr_frames = INITIAL_NR_GRANT_FRAMES;
> @@ -1710,7 +1714,7 @@ gnttab_grow_table(struct domain *d, unsigned int 
> req_nr_frames)
>          gnttab_create_shared_page(d, gt, i);
>      gt->nr_grant_frames = req_nr_frames;
>  
> -    return 1;
> +    return 0;
>  
>  shared_alloc_failed:
>      for ( i = nr_grant_frames(gt); i < req_nr_frames; i++ )
> @@ -1726,7 +1730,7 @@ active_alloc_failed:
>          gt->active[i] = NULL;
>      }
>      gdprintk(XENLOG_INFO, "Allocation failure when expanding grant 
> table.\n");
> -    return 0;
> +    return -ENOMEM;
>  }
>  
>  static int
> @@ -1769,8 +1773,10 @@ grant_table_init(struct domain *d, struct grant_table 
> *gt)
>          goto out;
>  
>      /* gnttab_grow_table() allocates a min number of frames, so 0 is okay. */
> -    if ( gnttab_grow_table(d, 0) )
> -        goto unlock;
> +    ret = gnttab_grow_table(d, 0);
> +    if (ret)
> +        goto out;
> +    goto unlock;

Eww.  This isn't your fault, but the goto lables here are horrible.

Here is a fix.  I'm unsure whether to merge it with yours, or do it as a
standalone patch and rebase yours over the top.  Either way, nothing I
can't fix while committing.

~Andrew

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 144fa7c..c58aa5d 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1736,37 +1736,37 @@ gnttab_grow_table(struct domain *d, unsigned int
req_nr_frames)
 static int
 grant_table_init(struct domain *d, struct grant_table *gt)
 {
-    int ret;
+    int ret = -ENOMEM;
 
     grant_write_lock(gt);
 
     if ( gt->active )
     {
         ret = -EBUSY;
-        goto unlock;
+        goto out;
     }
 
     /* Active grant table. */
     gt->active = xzalloc_array(struct active_grant_entry *,
                                max_nr_active_grant_frames);
     if ( gt->active == NULL )
-        goto no_mem;
+        goto out;
 
     /* Tracking of mapped foreign frames table */
     gt->maptrack = vzalloc(max_maptrack_frames * sizeof(*gt->maptrack));
     if ( gt->maptrack == NULL )
-        goto no_mem;
+        goto out;
 
     /* Shared grant table. */
     gt->shared_raw = xzalloc_array(void *, max_grant_frames);
     if ( gt->shared_raw == NULL )
-        goto no_mem;
+        goto out;
 
     /* Status pages for grant table - for version 2 */
     gt->status = xzalloc_array(grant_status_t *,
                                grant_to_status_frames(max_grant_frames));
     if ( gt->status == NULL )
-        goto no_mem;
+        goto out;
 
     ret = gnttab_init_arch(gt);
     if ( ret )
@@ -1774,24 +1774,21 @@ grant_table_init(struct domain *d, struct
grant_table *gt)
 
     /* gnttab_grow_table() allocates a min number of frames, so 0 is
okay. */
     ret = gnttab_grow_table(d, 0);
-    if (ret)
-        goto out;
-    goto unlock;
 
- no_mem:
-    ret = -ENOMEM;
  out:
-    gnttab_destroy_arch(gt);
-    xfree(gt->status);
-    gt->status = NULL;
-    xfree(gt->shared_raw);
-    gt->shared_raw = NULL;
-    vfree(gt->maptrack);
-    gt->maptrack = NULL;
-    xfree(gt->active);
-    gt->active = NULL;
+    if ( ret )
+    {
+        gnttab_destroy_arch(gt);
+        xfree(gt->status);
+        gt->status = NULL;
+        xfree(gt->shared_raw);
+        gt->shared_raw = NULL;
+        vfree(gt->maptrack);
+        gt->maptrack = NULL;
+        xfree(gt->active);
+        gt->active = NULL;
+    }
 
- unlock:
     grant_write_unlock(gt);
 
     return ret;


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