Re: Grant access to more than one dom

[Simon Martin <furryfuttock@gmail.com>]

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

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?

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?

10.- gntalloc.h typos fixed.

11.- Fixed struct ioctl_gntalloc_share_gref member order.

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

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

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 787d179..db33a89 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,31 @@ undo:
 	return rc;
 }
 
+static int __release_gref(grant_ref_t *gref_id)
+{
+	/* 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 = NULL;
+
 	if (gref->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
 		uint8_t *tmp = kmap(gref->page);
 		tmp[gref->notify.pgoff] = 0;
@@ -192,16 +228,18 @@ static void __del_gref(struct gntalloc_gref *gref)
 	}
 
 	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);
+	while (!list_empty(&gref->shares.next_share))
+	{
+		pos = list_first_entry(&gref->shares.next_share,
+						 struct gntalloc_share_list,
+						 next_share);
+		if (!__release_gref(&pos->gref_id))
+		{
+			list_del(&pos->next_share);
+			kfree(pos);
+		}
 	}
+	while (__release_gref(&gref->shares.gref_id));
 
 	gref_size--;
 	list_del(&gref->next_gref);
@@ -435,6 +473,104 @@ 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;
+
+	/* get the grant structure */
+	mutex_lock(&gref_mutex);
+	gref = find_grefs(priv, op.index, 1);
+
+	if (!gref) {
+		rc = -ENOENT;
+		goto share_out;
+	}
+
+	// create share list node
+	share_list = kzalloc(sizeof(*share_list), GFP_KERNEL);
+	if (!share_list) {
+		rc = -ENOMEM;
+		goto share_out;
+	}
+
+	/* Grant foreign access to the page. */
+	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;
+	}
+
+	// add this gref to the list for this page
+	list_add_tail(&share_list->next_share, &gref->shares.next_share);
+
+	// set output data
+	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;
+
+	/* get the grant structure */
+	mutex_lock(&gref_mutex);
+	gref = find_grefs(priv, op.index, 1);
+
+	if (!gref)
+		goto unshare_out;
+
+	// search the share list for this grant ref
+	list_for_each_entry(pos, &gref->shares.next_share, next_share) {
+		// if this is our grant ref then delete it
+		if (pos->gref_id == op.gref_id) {
+			// if we cannot release then request again
+			if (__release_gref(&pos->gref_id))
+				rc = -EAGAIN;
+			else {
+				// delete
+				list_del(&pos->next_share);
+				kfree(pos);
+
+				// success
+				rc = 0;
+			}
+
+			// done
+			break;
+		}
+	}
+
+unshare_out:
+	mutex_unlock(&gref_mutex);
+	return rc;
+}
+
 static long gntalloc_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
@@ -450,6 +586,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;
 	}
@@ -573,7 +715,7 @@ static const struct file_operations gntalloc_fops = {
  */
 static struct miscdevice gntalloc_miscdev = {
 	.minor	= MISC_DYNAMIC_MINOR,
-	.name	= "xen/gntalloc",
+	.name	= "xen/gntalloc_trio",
 	.fops	= &gntalloc_fops,
 };
 
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