From 8ca19a8937ad91703cfefccf13bd8017b39510cd Mon Sep 17 00:00:00 2001 From: Daniel De Graaf Date: Thu, 27 Oct 2011 17:58:48 -0400 Subject: xen/gntalloc: Change gref_lock to a mutex The event channel release function cannot be called under a spinlock because it can attempt to acquire a mutex due to the event channel reference acquired when setting up unmap notifications. Signed-off-by: Daniel De Graaf Signed-off-by: Konrad Rzeszutek Wilk --- drivers/xen/gntalloc.c | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) (limited to 'drivers/xen/gntalloc.c') diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c index f6832f46aea..439352d094d 100644 --- a/drivers/xen/gntalloc.c +++ b/drivers/xen/gntalloc.c @@ -74,7 +74,7 @@ MODULE_PARM_DESC(limit, "Maximum number of grants that may be allocated by " "the gntalloc device"); static LIST_HEAD(gref_list); -static DEFINE_SPINLOCK(gref_lock); +static DEFINE_MUTEX(gref_mutex); static int gref_size; struct notify_info { @@ -143,15 +143,15 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op, } /* Add to gref lists. */ - spin_lock(&gref_lock); + mutex_lock(&gref_mutex); list_splice_tail(&queue_gref, &gref_list); list_splice_tail(&queue_file, &priv->list); - spin_unlock(&gref_lock); + mutex_unlock(&gref_mutex); return 0; undo: - spin_lock(&gref_lock); + mutex_lock(&gref_mutex); gref_size -= (op->count - i); list_for_each_entry(gref, &queue_file, next_file) { @@ -167,7 +167,7 @@ undo: */ if (unlikely(!list_empty(&queue_gref))) list_splice_tail(&queue_gref, &gref_list); - spin_unlock(&gref_lock); + mutex_unlock(&gref_mutex); return rc; } @@ -251,7 +251,7 @@ static int gntalloc_release(struct inode *inode, struct file *filp) pr_debug("%s: priv %p\n", __func__, priv); - spin_lock(&gref_lock); + mutex_lock(&gref_mutex); while (!list_empty(&priv->list)) { gref = list_entry(priv->list.next, struct gntalloc_gref, next_file); @@ -261,7 +261,7 @@ static int gntalloc_release(struct inode *inode, struct file *filp) __del_gref(gref); } kfree(priv); - spin_unlock(&gref_lock); + mutex_unlock(&gref_mutex); return 0; } @@ -286,21 +286,21 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv, goto out; } - spin_lock(&gref_lock); + mutex_lock(&gref_mutex); /* Clean up pages that were at zero (local) users but were still mapped * by remote domains. Since those pages count towards the limit that we * are about to enforce, removing them here is a good idea. */ do_cleanup(); if (gref_size + op.count > limit) { - spin_unlock(&gref_lock); + mutex_unlock(&gref_mutex); rc = -ENOSPC; goto out_free; } gref_size += op.count; op.index = priv->index; priv->index += op.count * PAGE_SIZE; - spin_unlock(&gref_lock); + mutex_unlock(&gref_mutex); rc = add_grefs(&op, gref_ids, priv); if (rc < 0) @@ -343,7 +343,7 @@ static long gntalloc_ioctl_dealloc(struct gntalloc_file_private_data *priv, goto dealloc_grant_out; } - spin_lock(&gref_lock); + mutex_lock(&gref_mutex); gref = find_grefs(priv, op.index, op.count); if (gref) { /* Remove from the file list only, and decrease reference count. @@ -363,7 +363,7 @@ static long gntalloc_ioctl_dealloc(struct gntalloc_file_private_data *priv, do_cleanup(); - spin_unlock(&gref_lock); + mutex_unlock(&gref_mutex); dealloc_grant_out: return rc; } @@ -383,7 +383,7 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv, index = op.index & ~(PAGE_SIZE - 1); pgoff = op.index & (PAGE_SIZE - 1); - spin_lock(&gref_lock); + mutex_lock(&gref_mutex); gref = find_grefs(priv, index, 1); if (!gref) { @@ -400,8 +400,9 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv, gref->notify.pgoff = pgoff; gref->notify.event = op.event_channel_port; rc = 0; + unlock_out: - spin_unlock(&gref_lock); + mutex_unlock(&gref_mutex); return rc; } @@ -433,9 +434,9 @@ static void gntalloc_vma_open(struct vm_area_struct *vma) if (!gref) return; - spin_lock(&gref_lock); + mutex_lock(&gref_mutex); gref->users++; - spin_unlock(&gref_lock); + mutex_unlock(&gref_mutex); } static void gntalloc_vma_close(struct vm_area_struct *vma) @@ -444,11 +445,11 @@ static void gntalloc_vma_close(struct vm_area_struct *vma) if (!gref) return; - spin_lock(&gref_lock); + mutex_lock(&gref_mutex); gref->users--; if (gref->users == 0) __del_gref(gref); - spin_unlock(&gref_lock); + mutex_unlock(&gref_mutex); } static struct vm_operations_struct gntalloc_vmops = { @@ -471,7 +472,7 @@ static int gntalloc_mmap(struct file *filp, struct vm_area_struct *vma) return -EINVAL; } - spin_lock(&gref_lock); + mutex_lock(&gref_mutex); gref = find_grefs(priv, vma->vm_pgoff << PAGE_SHIFT, count); if (gref == NULL) { rv = -ENOENT; @@ -499,7 +500,7 @@ static int gntalloc_mmap(struct file *filp, struct vm_area_struct *vma) rv = 0; out_unlock: - spin_unlock(&gref_lock); + mutex_unlock(&gref_mutex); return rv; } -- cgit v1.2.3-18-g5258 From 0cc678f850f2cba0cedbd133fcbbf175554cd6c6 Mon Sep 17 00:00:00 2001 From: Daniel De Graaf Date: Thu, 27 Oct 2011 17:58:49 -0400 Subject: xen/gnt{dev,alloc}: reserve event channels for notify When using the unmap notify ioctl, the event channel used for notification needs to be reserved to avoid it being deallocated prior to sending the notification. Signed-off-by: Daniel De Graaf Signed-off-by: Konrad Rzeszutek Wilk --- drivers/xen/gntalloc.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) (limited to 'drivers/xen/gntalloc.c') diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c index 439352d094d..c95181f43a6 100644 --- a/drivers/xen/gntalloc.c +++ b/drivers/xen/gntalloc.c @@ -178,8 +178,10 @@ static void __del_gref(struct gntalloc_gref *gref) tmp[gref->notify.pgoff] = 0; kunmap(gref->page); } - if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) + if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) { notify_remote_via_evtchn(gref->notify.event); + evtchn_put(gref->notify.event); + } gref->notify.flags = 0; @@ -396,6 +398,23 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv, goto unlock_out; } + /* We need to grab a reference to the event channel we are going to use + * to send the notify before releasing the reference we may already have + * (if someone has called this ioctl twice). This is required so that + * it is possible to change the clear_byte part of the notification + * without disturbing the event channel part, which may now be the last + * reference to that event channel. + */ + if (op.action & UNMAP_NOTIFY_SEND_EVENT) { + if (evtchn_get(op.event_channel_port)) { + rc = -EINVAL; + goto unlock_out; + } + } + + if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) + evtchn_put(gref->notify.event); + gref->notify.flags = op.action; gref->notify.pgoff = pgoff; gref->notify.event = op.event_channel_port; -- cgit v1.2.3-18-g5258 From 0105d2b4fbc24c2fb6ca9bae650784dd7ddf0b12 Mon Sep 17 00:00:00 2001 From: Daniel De Graaf Date: Mon, 28 Nov 2011 11:49:10 -0500 Subject: xen/gntalloc: release grant references on page free gnttab_end_foreign_access_ref does not return the grant reference it is passed to the free list; gnttab_free_grant_reference needs to be explicitly called. While gnttab_end_foreign_access provides a wrapper for this, it is unsuitable because it does not return errors. Reported-by: Anil Madhavapeddy Signed-off-by: Daniel De Graaf Signed-off-by: Konrad Rzeszutek Wilk --- drivers/xen/gntalloc.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'drivers/xen/gntalloc.c') diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c index c95181f43a6..f330a4b8b68 100644 --- a/drivers/xen/gntalloc.c +++ b/drivers/xen/gntalloc.c @@ -191,6 +191,8 @@ static void __del_gref(struct gntalloc_gref *gref) if (!gnttab_end_foreign_access_ref(gref->gref_id, 0)) return; + + gnttab_free_grant_reference(gref->gref_id); } gref_size--; -- cgit v1.2.3-18-g5258 From 243082e0d59f169a1fa502f51ee5a820889fae93 Mon Sep 17 00:00:00 2001 From: Daniel De Graaf Date: Mon, 28 Nov 2011 11:49:11 -0500 Subject: xen/gntalloc: fix reference counts on multi-page mappings When a multi-page mapping of gntalloc is created, the reference counts of all pages in the vma are incremented. However, the vma open/close operations only adjusted the reference count of the first page in the mapping, leaking the other pages. Store a struct in the vm_private_data to track the original page count to properly free the pages when the last reference to the vma is closed. Reported-by: Anil Madhavapeddy Signed-off-by: Daniel De Graaf Signed-off-by: Konrad Rzeszutek Wilk --- drivers/xen/gntalloc.c | 56 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 13 deletions(-) (limited to 'drivers/xen/gntalloc.c') diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c index f330a4b8b68..e8ea56583b4 100644 --- a/drivers/xen/gntalloc.c +++ b/drivers/xen/gntalloc.c @@ -99,6 +99,12 @@ struct gntalloc_file_private_data { uint64_t index; }; +struct gntalloc_vma_private_data { + struct gntalloc_gref *gref; + int users; + int count; +}; + static void __del_gref(struct gntalloc_gref *gref); static void do_cleanup(void) @@ -451,25 +457,39 @@ static long gntalloc_ioctl(struct file *filp, unsigned int cmd, static void gntalloc_vma_open(struct vm_area_struct *vma) { - struct gntalloc_gref *gref = vma->vm_private_data; - if (!gref) + struct gntalloc_vma_private_data *priv = vma->vm_private_data; + + if (!priv) return; mutex_lock(&gref_mutex); - gref->users++; + priv->users++; mutex_unlock(&gref_mutex); } static void gntalloc_vma_close(struct vm_area_struct *vma) { - struct gntalloc_gref *gref = vma->vm_private_data; - if (!gref) + struct gntalloc_vma_private_data *priv = vma->vm_private_data; + struct gntalloc_gref *gref, *next; + int i; + + if (!priv) return; mutex_lock(&gref_mutex); - gref->users--; - if (gref->users == 0) - __del_gref(gref); + priv->users--; + if (priv->users == 0) { + gref = priv->gref; + for (i = 0; i < priv->count; i++) { + gref->users--; + next = list_entry(gref->next_gref.next, + struct gntalloc_gref, next_gref); + if (gref->users == 0) + __del_gref(gref); + gref = next; + } + kfree(priv); + } mutex_unlock(&gref_mutex); } @@ -481,19 +501,25 @@ static struct vm_operations_struct gntalloc_vmops = { static int gntalloc_mmap(struct file *filp, struct vm_area_struct *vma) { struct gntalloc_file_private_data *priv = filp->private_data; + struct gntalloc_vma_private_data *vm_priv; struct gntalloc_gref *gref; int count = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; int rv, i; - pr_debug("%s: priv %p, page %lu+%d\n", __func__, - priv, vma->vm_pgoff, count); - if (!(vma->vm_flags & VM_SHARED)) { printk(KERN_ERR "%s: Mapping must be shared.\n", __func__); return -EINVAL; } + vm_priv = kmalloc(sizeof(*vm_priv), GFP_KERNEL); + if (!vm_priv) + return -ENOMEM; + mutex_lock(&gref_mutex); + + pr_debug("%s: priv %p,%p, page %lu+%d\n", __func__, + priv, vm_priv, vma->vm_pgoff, count); + gref = find_grefs(priv, vma->vm_pgoff << PAGE_SHIFT, count); if (gref == NULL) { rv = -ENOENT; @@ -502,9 +528,13 @@ static int gntalloc_mmap(struct file *filp, struct vm_area_struct *vma) goto out_unlock; } - vma->vm_private_data = gref; + vm_priv->gref = gref; + vm_priv->users = 1; + vm_priv->count = count; + + vma->vm_private_data = vm_priv; - vma->vm_flags |= VM_RESERVED; + vma->vm_flags |= VM_RESERVED | VM_DONTEXPAND; vma->vm_ops = &gntalloc_vmops; -- cgit v1.2.3-18-g5258 From 2e16341438c9eca15a2e0bb2ad8555bbdf24b86d Mon Sep 17 00:00:00 2001 From: Julia Lawall Date: Fri, 23 Dec 2011 18:39:29 +0100 Subject: xen-gntalloc: introduce missing kfree Error handling code following a kmalloc should free the allocated data. Out_unlock is used on both success and failure, so free vm_priv before jumping to that label. A simplified version of the semantic match that finds the problem is as follows: (http://coccinelle.lip6.fr) // @r exists@ local idexpression x; statement S; identifier f1; position p1,p2; expression *ptr != NULL; @@ x@p1 = \(kmalloc\|kzalloc\|kcalloc\)(...); ... if (x == NULL) S <... when != x when != if (...) { <+...x...+> } x->f1 ...> ( return \(0\|<+...x...+>\|ptr\); | return@p2 ...; ) @script:python@ p1 << r.p1; p2 << r.p2; @@ print "* file: %s kmalloc %s return %s" % (p1[0].file,p1[0].line,p2[0].line) // Signed-off-by: Julia Lawall [v1: Altered the description a bit] Signed-off-by: Konrad Rzeszutek Wilk --- drivers/xen/gntalloc.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/xen/gntalloc.c') diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c index e2400c8963f..934985d14c2 100644 --- a/drivers/xen/gntalloc.c +++ b/drivers/xen/gntalloc.c @@ -525,6 +525,7 @@ static int gntalloc_mmap(struct file *filp, struct vm_area_struct *vma) rv = -ENOENT; pr_debug("%s: Could not find grant reference", __func__); + kfree(vm_priv); goto out_unlock; } -- cgit v1.2.3-18-g5258