From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH 1/6] xen-gntdev: Fix circular locking dependency Date: Tue, 14 Dec 2010 16:40:20 -0500 Message-ID: <4D07E444.3020309@tycho.nsa.gov> References: <1292338553-20575-1-git-send-email-dgdegra@tycho.nsa.gov> <1292338553-20575-2-git-send-email-dgdegra@tycho.nsa.gov> <4D07DD76.2010900@goop.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4D07DD76.2010900@goop.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jeremy Fitzhardinge Cc: xen-devel , Ian Campbell List-Id: xen-devel@lists.xenproject.org On 12/14/2010 04:11 PM, Jeremy Fitzhardinge wrote: > On 12/14/2010 06:55 AM, Daniel De Graaf wrote: >> apply_to_page_range will acquire PTE lock while priv->lock is held, and >> mn_invl_range_start tries to acquire priv->lock with PTE already held. >> Fix by not holding priv->lock during the entire map operation. > > Is priv->lock needed to protect the contents of map? > > J No, since the map can only be mapped once (checked by map->vma assignment while the lock is held). The unmap ioctl will return -EBUSY until an munmap() is called on the area, so it also can't race, and the other users are only active once the mmap operation completes. > >> Signed-off-by: Daniel De Graaf >> --- >> drivers/xen/gntdev.c | 19 +++++++++---------- >> 1 files changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c >> index a33e443..c582804 100644 >> --- a/drivers/xen/gntdev.c >> +++ b/drivers/xen/gntdev.c >> @@ -581,23 +581,22 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) >> if (!(vma->vm_flags & VM_WRITE)) >> map->flags |= GNTMAP_readonly; >> >> + spin_unlock(&priv->lock); >> + >> err = apply_to_page_range(vma->vm_mm, vma->vm_start, >> vma->vm_end - vma->vm_start, >> find_grant_ptes, map); >> - if (err) { >> - goto unlock_out; >> - if (debug) >> - printk("%s: find_grant_ptes() failure.\n", __FUNCTION__); >> - } >> + if (err) >> + return err; >> >> err = map_grant_pages(map); >> - if (err) { >> - goto unlock_out; >> - if (debug) >> - printk("%s: map_grant_pages() failure.\n", __FUNCTION__); >> - } >> + if (err) >> + return err; >> + >> map->is_mapped = 1; >> >> + return 0; >> + >> unlock_out: >> spin_unlock(&priv->lock); >> return err; >