[PATCH 0/2] xen/gntalloc: fix oopses after running out of grant refs

[PATCH 0/2] xen/gntalloc: fix oopses after running out of grant refs

[David Vrabel]

These two patches fix oopses that happen if a grant failed because
there are no free grant references.

This would not happen on a typical system with the default options as
the gntalloc device limits the number of pages that may be granted and
this check fails first, but could be triggered on VM with many PV
devices or if the limit is increase.

David

[PATCH 1/2] xen/gntalloc: fix oops after runnning out of grant refs

[David Vrabel]

Only set gref->gref_id if foreign access was successfully granted and
the grant ref is valid.

If gref->gref_id == -ENOSPC the test in __del_gref() would incorrectly
attempt to end foreign access (because grant_ref_t is unsigned).

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Reported-by: Dave Scott <dave.scott@citrix.com>
---
 drivers/xen/gntalloc.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 787d179..8ed2bb4f 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -141,13 +141,11 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
 			goto undo;
 
 		/* Grant foreign access to the page. */
-		gref->gref_id = gnttab_grant_foreign_access(op->domid,
+		rc = gnttab_grant_foreign_access(op->domid,
 			pfn_to_mfn(page_to_pfn(gref->page)), readonly);
-		if ((int)gref->gref_id < 0) {
-			rc = gref->gref_id;
+		if (rc < 0)
 			goto undo;
-		}
-		gref_ids[i] = gref->gref_id;
+		gref_ids[i] = gref->gref_id = rc;
 	}
 
 	/* Add to gref lists. */
@@ -193,7 +191,7 @@ static void __del_gref(struct gntalloc_gref *gref)
 
 	gref->notify.flags = 0;
 
-	if (gref->gref_id > 0) {
+	if (gref->gref_id) {
 		if (gnttab_query_foreign_access(gref->gref_id))
 			return;
 
-- 
1.7.10.4

[PATCH 2/2] xen/gntalloc: safely delete grefs in add_grefs() undo path

[David Vrabel]

If a gref could not be added (perhaps because the limit has been
reached or there are no more grant references available).  The undo
path may crash because __del_gref() frees the gref while it is being
used for a list iteration.

A comment suggests that using list_for_each_entry() is safe since the
gref isn't removed from the list being iterated over, but it is freed
and thus list_for_each_entry_safe() must be used.

Also, explicitly delete the gref from the local per-file list, even
though this is not strictly necessary.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/xen/gntalloc.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 8ed2bb4f..e53fe19 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -124,7 +124,7 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
 	int i, rc, readonly;
 	LIST_HEAD(queue_gref);
 	LIST_HEAD(queue_file);
-	struct gntalloc_gref *gref;
+	struct gntalloc_gref *gref, *next;
 
 	readonly = !(op->flags & GNTALLOC_FLAG_WRITABLE);
 	rc = -ENOMEM;
@@ -160,8 +160,8 @@ undo:
 	mutex_lock(&gref_mutex);
 	gref_size -= (op->count - i);
 
-	list_for_each_entry(gref, &queue_file, next_file) {
-		/* __del_gref does not remove from queue_file */
+	list_for_each_entry_safe(gref, next, &queue_file, next_file) {
+		list_del(&gref->next_file);
 		__del_gref(gref);
 	}
 
-- 
1.7.10.4

Re: [PATCH 2/2] xen/gntalloc: safely delete grefs in add_grefs() undo path

[Boris Ostrovsky]

On 09/02/2014 10:21 AM, David Vrabel wrote:
> If a gref could not be added (perhaps because the limit has been
> reached or there are no more grant references available).  The undo
> path may crash because __del_gref() frees the gref while it is being
> used for a list iteration.

Need to fix commit message above.

>
> A comment suggests that using list_for_each_entry() is safe since the
> gref isn't removed from the list being iterated over, but it is freed
> and thus list_for_each_entry_safe() must be used.

I don't read the comment in the code as if it implied anything about safety.

Other than that, for both patches

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

>
> Also, explicitly delete the gref from the local per-file list, even
> though this is not strictly necessary.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   drivers/xen/gntalloc.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
> index 8ed2bb4f..e53fe19 100644
> --- a/drivers/xen/gntalloc.c
> +++ b/drivers/xen/gntalloc.c
> @@ -124,7 +124,7 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
>   	int i, rc, readonly;
>   	LIST_HEAD(queue_gref);
>   	LIST_HEAD(queue_file);
> -	struct gntalloc_gref *gref;
> +	struct gntalloc_gref *gref, *next;
>   
>   	readonly = !(op->flags & GNTALLOC_FLAG_WRITABLE);
>   	rc = -ENOMEM;
> @@ -160,8 +160,8 @@ undo:
>   	mutex_lock(&gref_mutex);
>   	gref_size -= (op->count - i);
>   
> -	list_for_each_entry(gref, &queue_file, next_file) {
> -		/* __del_gref does not remove from queue_file */
> +	list_for_each_entry_safe(gref, next, &queue_file, next_file) {
> +		list_del(&gref->next_file);
>   		__del_gref(gref);
>   	}
>   

Re: [PATCH 2/2] xen/gntalloc: safely delete grefs in add_grefs() undo path

[David Vrabel]

On 02/09/14 22:54, Boris Ostrovsky wrote:
> On 09/02/2014 10:21 AM, David Vrabel wrote:
>> If a gref could not be added (perhaps because the limit has been
>> reached or there are no more grant references available).  The undo
>> path may crash because __del_gref() frees the gref while it is being
>> used for a list iteration.
> 
> Need to fix commit message above.
> 
>>
>> A comment suggests that using list_for_each_entry() is safe since the
>> gref isn't removed from the list being iterated over, but it is freed
>> and thus list_for_each_entry_safe() must be used.
> 
> I don't read the comment in the code as if it implied anything about
> safety.
> 
> Other than that, for both patches
> 
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Applied to stable/for-linus-3.17-b.

Thanks

David