[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] Grant access to more than one dom
On 06/05/2014 12:10 PM, Simon Martin wrote:
Hello Daniel,
I have been using the modified kernel driver for a few weeks now and
it seems to be doing everything I want it to. I attach the patch file,
this patches gntalloc.c and gntalloc.h.
Can you check that you are happy with the changes and then tell me
what to do to push then upstream?
Regards.
Ian has already mentioned the preferred method to submit for upstream. I
have a few comments on this version below.
diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 787d179..60998d1 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 share_list; /* The list of grant reference
numbers associated with this page */
struct notify_info notify; /* Unmap notification */
};
While I think that including the full structure here is correct since it
avoids unneeded allocations for the common case where only one gref_id is
used, this does end up complicating some of the logic slightly. Naming
the variable share_list is a bit misleading, since it contains a gref_id
in addition to the list.
[...]
@@ -179,8 +192,21 @@ undo:
return rc;
}
+static void __release_gref(grant_ref_t *gref_id)
+{
+ if ((*gref_id > 0) &&
+ !gnttab_query_foreign_access(*gref_id) &&
+ gnttab_end_foreign_access_ref(*gref_id, 0))
+ {
+ gnttab_free_grant_reference(*gref_id);
+ *gref_id = 0;
+ }
+}
This function needs to return success/failure.
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 +218,16 @@ 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->share_list.next_share))
+ {
+ pos = list_first_entry(&gref->share_list.next_share,
+ struct gntalloc_share_list,
+ next_share);
+ list_del(&pos->next_share);
+ __release_gref(&pos->gref_id);
+ kfree(pos);
}
+ __release_gref(&gref->share_list.gref_id);
This will leak grant references if __release_gref fails, both inside and
outside the loop. You need to remove the references that can be freed
and return before this line if any of the grant references still have a
foreign mapping.
gref_size--;
list_del(&gref->next_gref);
@@ -337,6 +363,7 @@ static long gntalloc_ioctl_alloc(struct
gntalloc_file_private_data *priv,
out_free:
kfree(gref_ids);
+
Unneeded whitespace change.
out:
return rc;
}
@@ -435,6 +462,97 @@ 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;
+ grant_ref_t gref_id;
+ 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;
+ }
+
+ /* Grant foreign access to the page. */
+ gref_id = gnttab_grant_foreign_access(op.domid,
+ pfn_to_mfn(page_to_pfn(gref->page)), readonly);
+ if ((signed)gref_id < 0) {
+ rc = gref_id;
+ goto share_out;
+ }
+
+ // add this gref to the list for this page
+ share_list = kzalloc(sizeof(*share_list), GFP_KERNEL);
You need to check for kzalloc returning NULL, and for proper error recovery,
the allocation needs to be done before calling gnttab_grant_foreign_access.
Otherwise, it is possible to end up in a situation where you have a grant
that cannot be freed due to a foreign mapping, but with no data structure to
put the ID in so it can be freed later.
+ INIT_LIST_HEAD(&share_list->next_share);
This is not needed since you are immediately adding it to a list.
+ share_list->gref_id = gref_id;
+ list_add_tail(&share_list->next_share, &gref->share_list.next_share);
+
+ // set output data
+ op.gref_id = 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->share_list.next_share, next_share) {
+ // if this is our grant ref then delete it
+ if (pos->gref_id == op.gref_id) {
+ // delete
+ list_del(&pos->next_share);
+ __release_gref(&pos->gref_id);
This needs to return an error if the page is still shared instead of leaking
the reference.
+ kfree(pos);
+
+ // success
+ rc = 0;
+
+ // done
+ break;
+ }
+ }
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.
+unshare_out:
+ mutex_unlock(&gref_mutex);
+
+ return rc;
+}
+
static long gntalloc_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)
{
@@ -450,6 +568,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..20dc973 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 smae page
Typo (smae=>same)
+ * 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 ID of the domain to be given access to the grants. */
+ uint16_t domid;
+ /* Flags for this mapping */
+ uint16_t flags;
+ /* The offset used in call to mmap(). */
+ uint64_t index;
+ /* OUT parameters */
+ /* The grant references of the newly created grant */
+ uint32_t gref_id;
+};
This structure should have uint64_t index as the first member; otherwise,
it will differ in size depending on the alignment of 64-bit types and a
compat_ioctl will be required for 32-bit userspace.
+
+/*
+ * Unhares a page that has already been shared
Typo ^^^^^^^
+ */
+#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__ */
--
Daniel De Graaf
National Security Agency
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
