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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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);
>   	}
>