From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 2/2] xen/gntalloc: safely delete grefs in add_grefs() undo path Date: Tue, 02 Sep 2014 17:54:34 -0400 Message-ID: <54063C9A.9010503@oracle.com> References: <1409667690-23914-1-git-send-email-david.vrabel@citrix.com> <1409667690-23914-3-git-send-email-david.vrabel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XOw1m-0002Gk-3a for xen-devel@lists.xenproject.org; Tue, 02 Sep 2014 21:54:14 +0000 In-Reply-To: <1409667690-23914-3-git-send-email-david.vrabel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: David Vrabel , xen-devel@lists.xenproject.org Cc: Daniel De Graaf , Dave Scott List-Id: xen-devel@lists.xenproject.org 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 > > Also, explicitly delete the gref from the local per-file list, even > though this is not strictly necessary. > > Signed-off-by: David Vrabel > --- > 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); > } >