Re: [PATCH 5/6] xen-gntalloc: Userspace grant allocation driver

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

> >> +	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>