WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH 5/6] xen-gntalloc: Userspace grant allocation dri

To: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH 5/6] xen-gntalloc: Userspace grant allocation driver
From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date: Thu, 27 Jan 2011 16:29:55 -0500
Cc: jeremy@xxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxx, Ian.Campbell@xxxxxxxxxx
Delivery-date: Thu, 27 Jan 2011 13:30:59 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4D41DBC2.6050804@xxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1295625548-22069-1-git-send-email-dgdegra@xxxxxxxxxxxxx> <1295625548-22069-6-git-send-email-dgdegra@xxxxxxxxxxxxx> <20110127185210.GA27853@xxxxxxxxxxxx> <4D41DBC2.6050804@xxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.20 (2009-06-14)
> >> +  try_module_get(THIS_MODULE);
> > 
> > No checking if it fails?
> 
> Actually, looking at it again, this seems redundant: won't open() itself
> prevent rmmod of the module until everything is closed?

I hope so :-)
> 
> Regardless, while the failure seems unlikely it should be checked for.

OK.

.. big snip ..
> >> +  spin_lock(&gref_lock);
> >> +  do_cleanup();
> > 
> > Hmm, why the cleanup here? I see it  gntalloc_ioctl_dealloc
> > which makes sense since the users-- might have been decremented to zero.
> > But here?
> 
> This is to 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, it is a good idea to remove any pages that have
> been unmapped by remote domains since last time we checked.

Ok, can you put that as comment right before calling do_cleanup, please?

> 
> This would be much cleaner if Xen allowed a domain to force others to
> unmap its pages, but that's a significant change to the semantics of
> shared memory in the hypervisor.
>  
> >> +  if (gref_size + op.count > limit) {
> >> +          spin_unlock(&gref_lock);
> >> +          rc = -ENOSPC;
> >> +          goto out_free;
> >> +  }
> >> +  gref_size += op.count;
> >> +  op.index = priv->index;
> >> +  priv->index += op.count * PAGE_SIZE;
> >> +  spin_unlock(&gref_lock);
> >> +
> >> +  rc = add_grefs(&op, gref_ids, priv);
> >> +  if (rc < 0)
> >> +          goto out_free;
> >> +
> >> +  if (copy_to_user(arg, &op, sizeof(op))) {
> >> +          rc = -EFAULT;
> >> +          goto out_free;
> > 
> > Not something that would clean the newly added grant? Say
> > the code I suggested below the 'out' label.
> > 
> 
> That races with a concurrent removal operation that has guessed
> the offset we just added, and removed the gref. As soon as we unlock
> gref_lock at the end of add_grefs, gref is unsafe to dereference.

Aha! Can you put a comment about this so in the future we won't
try to correct this "mistake" ?

> 
> This could be solved by a per-file lock, or by holding gref_lock
> for longer, but the copy_to_user producing -EFAULT seemed unlikely
> enough that forcing a close() seemed the better choice - especially
> since the userspace application will be segfaulting soon if it is
> trying to read the offsets.

True enought.

.. big snip..
> >> +  spin_lock(&gref_lock);
> >> +  gref->users--;
> >> +  if (gref->users == 0)
> >> +          __del_gref(gref);
> > 
> > I just want to be convienced here that I am wrong.
> > 
> > If the 'ioctl_deallo' has not been done, what will if unmap this VMA?
> > Will it be OK to  yank the gref from gref_list while (and kfree it) while
> > it is still referenced in the filp->private_data? Or would end up trying
> > to derefence the *priv and go BOOM?
> 
> The VMA itself is unmapped regardless. The gref structure (and the pages
> pointed to by the vma) is deallocated when the last reference goes away.
> In your example, it would be on _release() of the file or a later dealloc
> ioctl.

OK, you convienced me.
> 
> The only time __del_gref is called here is when the file has been closed
> or the segment has already had ioctl_dealloc run on it.

<nods>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel