[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] Grant access to more than one dom



On 06/09/2014 11:53 AM, Simon Martin wrote:
Hi Daniel,

Thanks for the revision. I attach an updated patch. With regards your
comments:

1.- Renamed share_list field of gntalloc_gref. Now called shares.

2.- As you said, I think the increased logic is worth the effort of
not requiring dynamic memory for the usual case of only one share.

3.- Analyzing the return value of __release_gref I found there is a
possible (unlikely) infinite loop in the original __del_gref
implementation. If gref_id <= 0 then the list element will never be
released. If we have this situation then a memory leak is the the
least of our worries as there is either a problem in the logic or
memory corruption. I propose that under this situation we should just
return OK and carry on. What do you think?

The infinite loop you added is incorrect and is actually quite likely to
be triggered if the other domain is not responding to whatever unmap
request has been set up.  This does not have to be an error; it could be
triggered because the other domain has not yet been scheduled after the
notify was queued.  If __release_gref fails, then you need to return from
__del_gref without actually deleting the gref object.  This postpones the
actual deletion until it is retried by do_cleanup.

Encountering gref <= 0 in this loop should not happen; that indicates a
significantproblem and deserves a WARN_ON if you want to check for it.

4.- Fixed possible leak in __del_gref

5.- Fixed whitespace changes.

6.- Checked kzalloc return value in gntalloc_ioctl_share.

7.- Removed superfluous INIT_LIST_HEAD in gntalloc_ioctl_share.

8.- Fixed possible gref leak in gntalloc_ioctl_unshare

9.- Re your comment about gntalloc_ioctl_unshare:

It would be nice if this function also supported freeing the initial grant
reference, so that it can be used to change the domain ID a page is being
shared with.

The idea of gntalloc_gref.shares is that there is 1 or more grefs
associated to any allocation. This would mean changing that to 0 or
more, i.e. gntalloc_gref.shares should be changed directly to a list
head and we will require a memory allocation for the original
allocation. The change is simple, shall I do it?

I would instead change the logic to something like:
If removing the primary reference and there are secondary references, promote
the first secondary reference (and remove/free its gntalloc_share_list). If
removing the last reference, either error, act as DEALLOC_GREF, or change all
other uses of gref_id to handle gref_id==0 meaning "not shared".

10.- gntalloc.h typos fixed.

11.- Fixed struct ioctl_gntalloc_share_gref member order.

The name change to xen/gntalloc_trio should not be in the submitted patch;
I assume this was for testing.


--
Daniel De Graaf
National Security Agency

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.