Re: Grant access to more than one dom

[Simon Martin <furryfuttock@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]

Hello Daniel,

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

I have added WARN_ON(gref <= 0)

Removed loop waiting for foreign dom to release page and added
do_cleanup to end of share/unshare IOCTL.

>>> 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".

Implemented this. Not as clean as changing the semantics but avoids
dynamic allocation for the most common case.

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

Sorry. I do this so I can have the original driver loaded at the same
time. I realized this after I'd sent the patch, but I'd already sent
it.

-- 
Best regards,
 Simon                            mailto:furryfuttock@gmail.com

[-- Attachment #2: gntalloc.patch --]
[-- Type: application/octet-stream, Size: 8691 bytes --]

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 787d179..f15d8b6 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -85,6 +85,12 @@ struct notify_info {
 	int event;            /* Port (event channel) to notify */
 };
 
+/* list of foreign grants associated to a given grant reference */
+struct gntalloc_share_list {
+	grant_ref_t gref_id;
+	struct list_head next_share;
+};
+
 /* Metadata on a grant reference. */
 struct gntalloc_gref {
 	struct list_head next_gref;  /* list entry gref_list */
@@ -92,7 +98,7 @@ struct gntalloc_gref {
 	struct page *page;	     /* The shared page */
 	uint64_t file_index;         /* File offset for mmap() */
 	unsigned int users;          /* Use count - when zero, waiting on Xen */
-	grant_ref_t gref_id;         /* The grant reference number */
+	struct gntalloc_share_list shares;  /* The list of grant reference numbers associated with this page */
 	struct notify_info notify;   /* Unmap notification */
 };
 
@@ -125,6 +131,7 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
 	LIST_HEAD(queue_gref);
 	LIST_HEAD(queue_file);
 	struct gntalloc_gref *gref;
+	grant_ref_t gref_id;
 
 	readonly = !(op->flags & GNTALLOC_FLAG_WRITABLE);
 	rc = -ENOMEM;
@@ -141,13 +148,19 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
 			goto undo;
 
 		/* Grant foreign access to the page. */
-		gref->gref_id = gnttab_grant_foreign_access(op->domid,
+		gref_id = gnttab_grant_foreign_access(op->domid,
 			pfn_to_mfn(page_to_pfn(gref->page)), readonly);
-		if ((int)gref->gref_id < 0) {
-			rc = gref->gref_id;
+		if ((int)gref_id < 0) {
+			rc = gref_id;
 			goto undo;
 		}
-		gref_ids[i] = gref->gref_id;
+
+		/* store the gred_id to be returned to the user so they can use it later */
+		gref_ids[i] = gref_id;
+
+		/* add this gref to the list for this page */
+		INIT_LIST_HEAD(&gref->shares.next_share);
+		gref->shares.gref_id = gref_id;
 	}
 
 	/* Add to gref lists. */
@@ -179,8 +192,32 @@ undo:
 	return rc;
 }
 
-static void __del_gref(struct gntalloc_gref *gref)
-{
+static int __release_gref(grant_ref_t *gref_id) {
+	WARN_ON(*gref_id < 0);
+
+	/* if we have an invalid gref_id then we have a real problem, either a
+	   problem in logic or memory corruption, so a leaking a page is the
+	   least of our worries */
+	if (*gref_id <= 0)
+		return 0;
+	/* if this gref is still mapped by the foreign dom then we can't release
+	   so return error and the caller must retry */
+	else if (gnttab_query_foreign_access(*gref_id))
+		return -EAGAIN;
+	/* if we can't release the foreign access to this gref then we can't
+	   release and the caller must retry */
+	else if (!gnttab_end_foreign_access_ref(*gref_id, 0))
+		return -EAGAIN;
+
+	gnttab_free_grant_reference(*gref_id);
+	*gref_id = 0;
+	return 0;
+}
+
+static void __del_gref(struct gntalloc_gref *gref) {
+	struct gntalloc_share_list *pos, *n;
+	int pending = 0;
+
 	if (gref->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
 		uint8_t *tmp = kmap(gref->page);
 		tmp[gref->notify.pgoff] = 0;
@@ -190,18 +227,23 @@ static void __del_gref(struct gntalloc_gref *gref)
 		notify_remote_via_evtchn(gref->notify.event);
 		evtchn_put(gref->notify.event);
 	}
-
 	gref->notify.flags = 0;
 
-	if (gref->gref_id > 0) {
-		if (gnttab_query_foreign_access(gref->gref_id))
-			return;
-
-		if (!gnttab_end_foreign_access_ref(gref->gref_id, 0))
-			return;
-
-		gnttab_free_grant_reference(gref->gref_id);
+	list_for_each_entry_safe(pos, n, &gref->shares.next_share, next_share) {
+		if (!__release_gref(&pos->gref_id))
+		{
+			list_del(&pos->next_share);
+			kfree(pos);
+		}
+		else
+			pending++;
 	}
+	if (gref->shares.gref_id > 0)
+		pending += !!__release_gref(&gref->shares.gref_id);
+
+	/* if there are shares that are still mapped by foreign doms then we postpone page removal to do_cleanup */
+	if (pending)
+		return;
 
 	gref_size--;
 	list_del(&gref->next_gref);
@@ -435,6 +477,126 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
 	return rc;
 }
 
+static long gntalloc_ioctl_share(struct gntalloc_file_private_data *priv, void __user *arg)
+{
+	struct ioctl_gntalloc_share_gref op;
+	struct gntalloc_gref *gref;
+	int rc;
+	int readonly;
+	struct gntalloc_share_list *share_list;
+
+	if (copy_from_user(&op, arg, sizeof(op)))
+		return -EFAULT;
+
+	readonly = !(op.flags & GNTALLOC_FLAG_WRITABLE);
+	rc = -ENOMEM;
+
+	mutex_lock(&gref_mutex);
+
+	/* see if we can free up any pending page releases */
+	do_cleanup();
+
+	gref = find_grefs(priv, op.index, 1);
+
+	if (!gref) {
+		rc = -ENOENT;
+		goto share_out;
+	}
+
+	/* if the initial share is assigned then create a new share list node */
+	if (gref->shares.gref_id) {
+		share_list = kzalloc(sizeof(*share_list), GFP_KERNEL);
+		if (!share_list) {
+			rc = -ENOMEM;
+			goto share_out;
+		}
+
+		share_list->gref_id = gnttab_grant_foreign_access(op.domid,
+			pfn_to_mfn(page_to_pfn(gref->page)), readonly);
+		if ((signed)share_list->gref_id < 0) {
+			kzfree(share_list);
+			rc = share_list->gref_id;
+			goto share_out;
+		}
+
+		list_add_tail(&share_list->next_share, &gref->shares.next_share);
+	}
+	else {
+		share_list = &gref->shares;
+		share_list->gref_id = gnttab_grant_foreign_access(op.domid,
+			pfn_to_mfn(page_to_pfn(gref->page)), readonly);
+		if ((signed)share_list->gref_id < 0) {
+			rc = share_list->gref_id;
+			goto share_out;
+		}
+	}
+
+	op.gref_id = share_list->gref_id;
+	rc = 0;
+
+share_out:
+	mutex_unlock(&gref_mutex);
+
+	if (!rc && copy_to_user(arg, &op, sizeof(op)))
+		rc = -EFAULT;
+
+	return rc;
+}
+
+static long gntalloc_ioctl_unshare(struct gntalloc_file_private_data *priv, void __user *arg)
+{
+	struct ioctl_gntalloc_unshare_gref op;
+	struct gntalloc_gref *gref;
+	int rc = -ENOENT;
+	struct gntalloc_share_list *pos;
+
+	if (copy_from_user(&op, arg, sizeof(op)))
+		return -EFAULT;
+
+	mutex_lock(&gref_mutex);
+	gref = find_grefs(priv, op.index, 1);
+
+	if (!gref)
+		goto unshare_out;
+
+	/* if we are releasing the initial share then replace it with the next available share.
+	   if there are no more shares then just leave the gref_id as 0 */
+	if (gref->shares.gref_id == op.gref_id) {
+		if (__release_gref(&gref->shares.gref_id))
+			rc = -EAGAIN;
+		else {
+			pos = list_first_entry(&gref->shares.next_share,
+							 struct gntalloc_share_list,
+							 next_share);
+			if (pos) {
+				gref->shares.gref_id = pos->gref_id;
+				list_del(&pos->next_share);
+				kfree(pos);
+			}
+		}
+	}
+	else {
+		list_for_each_entry(pos, &gref->shares.next_share, next_share) {
+			if (pos->gref_id == op.gref_id) {
+				if (__release_gref(&pos->gref_id))
+					rc = -EAGAIN;
+				else {
+					list_del(&pos->next_share);
+					kfree(pos);
+					rc = 0;
+				}
+				break;
+			}
+		}
+	}
+
+	do_cleanup();
+
+unshare_out:
+	mutex_unlock(&gref_mutex);
+	return rc;
+}
+
 static long gntalloc_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
@@ -450,6 +612,12 @@ static long gntalloc_ioctl(struct file *filp, unsigned int cmd,
 	case IOCTL_GNTALLOC_SET_UNMAP_NOTIFY:
 		return gntalloc_ioctl_unmap_notify(priv, (void __user *)arg);
 
+	case IOCTL_GNTALLOC_SHARE_GREF:
+		return gntalloc_ioctl_share(priv, (void __user *)arg);
+
+	case IOCTL_GNTALLOC_UNSHARE_GREF:
+		return gntalloc_ioctl_unshare(priv, (void __user *)arg);
+
 	default:
 		return -ENOIOCTLCMD;
 	}
diff --git a/include/uapi/xen/gntalloc.h b/include/uapi/xen/gntalloc.h
index 76bd580..31911da 100644
--- a/include/uapi/xen/gntalloc.h
+++ b/include/uapi/xen/gntalloc.h
@@ -79,4 +79,36 @@ struct ioctl_gntalloc_unmap_notify {
 /* Send an interrupt on the indicated event channel */
 #define UNMAP_NOTIFY_SEND_EVENT 0x2
 
+/*
+ * Shares a page that has already been allocated (so we can share the same page 
+ * between more than one domain)
+ */
+#define IOCTL_GNTALLOC_SHARE_GREF \
+_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntalloc_share_gref))
+struct ioctl_gntalloc_share_gref {
+	/* IN parameters */
+	/* The offset used in call to mmap(). */
+	uint64_t index;
+	/* The ID of the domain to be given access to the grants. */
+	uint16_t domid;
+	/* Flags for this mapping */
+	uint16_t flags;
+	/* OUT parameters */
+	/* The grant references of the newly created grant */
+	uint32_t gref_id;
+};
+
+/*
+ * Unshares a page that has already been shared
+ */
+#define IOCTL_GNTALLOC_UNSHARE_GREF \
+_IOC(_IOC_NONE, 'G', 9, sizeof(struct ioctl_gntalloc_unshare_gref))
+struct ioctl_gntalloc_unshare_gref {
+	/* IN parameters */
+	/* The offset used in call to mmap(). */
+	uint64_t index;
+	/* The grant references of the newly created grant */
+	uint32_t gref_id;
+};
+
 #endif /* __LINUX_PUBLIC_GNTALLOC_H__ */

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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