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

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

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