[RFC PATCH 47/60] hyper_dmabuf: fix issues with event-polling

From: Dongwon Kim
Date: Tue Dec 19 2017 - 14:41:10 EST

This patch fixes several defects on event handling
including:

1. Imported sgt info and exported sgt info now have
buffer for private data (priv) with variable size

2. Now user input to export_remote_ioctl contain sz_priv,
which specifies size of private data (e.g. meta data)

3. Increased max size of operands to 64 * sizeof(int)
to accomodate maximum size of private data

4. Initialize mutexes and spinlock

5. Change max event queue depth to 32 to prevent user app
to display too much outdated frames.

6. Frees oldest event if event queue is full to prevent
overflow.

Signed-off-by: Dongwon Kim <dongwon.kim@xxxxxxxxx>
---
drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c | 23 ++++++---
drivers/xen/hyper_dmabuf/hyper_dmabuf_event.c | 8 +--
drivers/xen/hyper_dmabuf/hyper_dmabuf_event.h | 2 +-
drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c | 64 ++++++++++++++++++++++--
drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c | 42 +++++++++++++---
drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h | 2 +-
drivers/xen/hyper_dmabuf/hyper_dmabuf_sgl_proc.c | 1 +
drivers/xen/hyper_dmabuf/hyper_dmabuf_struct.h | 9 +++-
include/uapi/xen/hyper_dmabuf.h | 7 ++-
9 files changed, 131 insertions(+), 27 deletions(-)

diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c
index 005677d..87ea6ca 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c
@@ -74,9 +74,6 @@ int hyper_dmabuf_release(struct inode *inode, struct file *filp)
{
hyper_dmabuf_foreach_exported(hyper_dmabuf_emergency_release, filp);

- /* clean up event queue */
- hyper_dmabuf_events_release();
-
return 0;
}

@@ -98,12 +95,18 @@ ssize_t hyper_dmabuf_event_read(struct file *filp, char __user *buffer,
int ret;

/* only root can read events */
- if (!capable(CAP_DAC_OVERRIDE))
+ if (!capable(CAP_DAC_OVERRIDE)) {
+ dev_err(hyper_dmabuf_private.device,
+ "Only root can read events\n");
return -EFAULT;
+ }

/* make sure user buffer can be written */
- if (!access_ok(VERIFY_WRITE, buffer, count))
+ if (!access_ok(VERIFY_WRITE, buffer, count)) {
+ dev_err(hyper_dmabuf_private.device,
+ "User buffer can't be written.\n");
return -EFAULT;
+ }

ret = mutex_lock_interruptible(&hyper_dmabuf_private.event_read_lock);
if (ret)
@@ -132,7 +135,7 @@ ssize_t hyper_dmabuf_event_read(struct file *filp, char __user *buffer,
ret = wait_event_interruptible(hyper_dmabuf_private.event_wait,
!list_empty(&hyper_dmabuf_private.event_list));

- if (ret >= 0)
+ if (ret == 0)
ret = mutex_lock_interruptible(&hyper_dmabuf_private.event_read_lock);

if (ret)
@@ -174,13 +177,14 @@ ssize_t hyper_dmabuf_event_read(struct file *filp, char __user *buffer,
}

ret += e->event_data.hdr.size;
+ hyper_dmabuf_private.curr_num_event--;
kfree(e);
}
}

mutex_unlock(&hyper_dmabuf_private.event_read_lock);

- return 0;
+ return ret;
}

static struct file_operations hyper_dmabuf_driver_fops =
@@ -233,6 +237,8 @@ static int __init hyper_dmabuf_drv_init(void)
printk( KERN_NOTICE "hyper_dmabuf_starting: Initialization started\n");

mutex_init(&hyper_dmabuf_private.lock);
+ mutex_init(&hyper_dmabuf_private.event_read_lock);
+ spin_lock_init(&hyper_dmabuf_private.event_lock);

ret = register_device();
if (ret < 0) {
@@ -329,6 +335,9 @@ static void hyper_dmabuf_drv_exit(void)

hyper_dmabuf_private.exited = true;

+ /* clean up event queue */
+ hyper_dmabuf_events_release();
+
mutex_unlock(&hyper_dmabuf_private.lock);

dev_info(hyper_dmabuf_private.device,
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_event.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_event.c
index be70e54..8998a7d 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_event.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_event.c
@@ -49,11 +49,12 @@ static void hyper_dmabuf_send_event_locked(struct hyper_dmabuf_event *e)

/* check current number of event then if it hits the max num allowed
* then remove the oldest event in the list */
- if (hyper_dmabuf_private.curr_num_event > MAX_NUMBER_OF_EVENT - 1) {
+ if (hyper_dmabuf_private.curr_num_event > MAX_DEPTH_EVENT_QUEUE - 1) {
oldest = list_first_entry(&hyper_dmabuf_private.event_list,
struct hyper_dmabuf_event, link);
list_del(&oldest->link);
hyper_dmabuf_private.curr_num_event--;
+ kfree(oldest);
}

list_add_tail(&e->link,
@@ -74,6 +75,7 @@ void hyper_dmabuf_events_release()
list_for_each_entry_safe(e, et, &hyper_dmabuf_private.event_list,
link) {
list_del(&e->link);
+ kfree(e);
hyper_dmabuf_private.curr_num_event--;
}

@@ -104,8 +106,8 @@ int hyper_dmabuf_import_event(hyper_dmabuf_id_t hid)

e->event_data.hdr.event_type = HYPER_DMABUF_NEW_IMPORT;
e->event_data.hdr.hid = hid;
- e->event_data.data = (void*)&imported_sgt_info->priv[0];
- e->event_data.hdr.size = 128;
+ e->event_data.data = (void*)imported_sgt_info->priv;
+ e->event_data.hdr.size = imported_sgt_info->sz_priv;

spin_lock_irqsave(&hyper_dmabuf_private.event_lock, irqflags);

diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_event.h b/drivers/xen/hyper_dmabuf/hyper_dmabuf_event.h
index 44c4856..50db04f 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_event.h
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_event.h
@@ -25,7 +25,7 @@
#ifndef __HYPER_DMABUF_EVENT_H__
#define __HYPER_DMABUF_EVENT_H__

-#define MAX_NUMBER_OF_EVENT 1024
+#define MAX_DEPTH_EVENT_QUEUE 32

enum hyper_dmabuf_event_type {
HYPER_DMABUF_NEW_IMPORT = 0x10000,
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
index 85b70db..06f95ca 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
@@ -87,7 +87,7 @@ static int hyper_dmabuf_send_export_msg(struct hyper_dmabuf_sgt_info *sgt_info,
{
struct hyper_dmabuf_backend_ops *ops = hyper_dmabuf_private.backend_ops;
struct hyper_dmabuf_req *req;
- int operands[40] = {0};
+ int operands[MAX_NUMBER_OF_OPERANDS] = {0};
int ret, i;

/* now create request for importer via ring */
@@ -108,8 +108,10 @@ static int hyper_dmabuf_send_export_msg(struct hyper_dmabuf_sgt_info *sgt_info,
}
}

- /* driver/application specific private info, max 4x4 bytes */
- memcpy(&operands[8], &sgt_info->priv[0], sizeof(unsigned int) * 32);
+ operands[8] = sgt_info->sz_priv;
+
+ /* driver/application specific private info */
+ memcpy(&operands[9], sgt_info->priv, operands[8]);

req = kcalloc(1, sizeof(*req), GFP_KERNEL);

@@ -181,8 +183,32 @@ static int hyper_dmabuf_export_remote_ioctl(struct file *filp, void *data)
sgt_info->unexport_scheduled = 0;
}

+ /* if there's any change in size of private data.
+ * we reallocate space for private data with new size */
+ if (export_remote_attr->sz_priv != sgt_info->sz_priv) {
+ kfree(sgt_info->priv);
+
+ /* truncating size */
+ if (export_remote_attr->sz_priv > MAX_SIZE_PRIV_DATA) {
+ sgt_info->sz_priv = MAX_SIZE_PRIV_DATA;
+ } else {
+ sgt_info->sz_priv = export_remote_attr->sz_priv;
+ }
+
+ sgt_info->priv = kcalloc(1, sgt_info->sz_priv, GFP_KERNEL);
+
+ if(!sgt_info->priv) {
+ dev_err(hyper_dmabuf_private.device,
+ "Can't reallocate priv because there's no more space left\n");
+ hyper_dmabuf_remove_exported(sgt_info->hid);
+ hyper_dmabuf_cleanup_sgt_info(sgt_info, true);
+ kfree(sgt_info);
+ return -ENOMEM;
+ }
+ }
+
/* update private data in sgt_info with new ones */
- memcpy(&sgt_info->priv[0], &export_remote_attr->priv[0], sizeof(unsigned int) * 32);
+ copy_from_user(sgt_info->priv, export_remote_attr->priv, sgt_info->sz_priv);

/* send an export msg for updating priv in importer */
ret = hyper_dmabuf_send_export_msg(sgt_info, NULL);
@@ -222,6 +248,26 @@ static int hyper_dmabuf_export_remote_ioctl(struct file *filp, void *data)
goto fail_sgt_info_creation;
}

+ /* possible truncation */
+ if (export_remote_attr->sz_priv > MAX_SIZE_PRIV_DATA) {
+ sgt_info->sz_priv = MAX_SIZE_PRIV_DATA;
+ } else {
+ sgt_info->sz_priv = export_remote_attr->sz_priv;
+ }
+
+ /* creating buffer for private data of buffer */
+ if(sgt_info->sz_priv != 0) {
+ sgt_info->priv = kcalloc(1, sgt_info->sz_priv, GFP_KERNEL);
+
+ if(!sgt_info->priv) {
+ dev_err(hyper_dmabuf_private.device, "no more space left\n");
+ ret = -ENOMEM;
+ goto fail_priv_creation;
+ }
+ } else {
+ dev_err(hyper_dmabuf_private.device, "size is 0\n");
+ }
+
sgt_info->hid = hyper_dmabuf_get_hid();

/* no more exported dmabuf allowed */
@@ -279,7 +325,7 @@ static int hyper_dmabuf_export_remote_ioctl(struct file *filp, void *data)
INIT_LIST_HEAD(&sgt_info->va_vmapped->list);

/* copy private data to sgt_info */
- memcpy(&sgt_info->priv[0], &export_remote_attr->priv[0], sizeof(unsigned int) * 32);
+ copy_from_user(sgt_info->priv, export_remote_attr->priv, sgt_info->sz_priv);

page_info = hyper_dmabuf_ext_pgs(sgt);
if (!page_info) {
@@ -329,6 +375,10 @@ static int hyper_dmabuf_export_remote_ioctl(struct file *filp, void *data)

fail_map_active_attached:
kfree(sgt_info->active_sgts);
+ kfree(sgt_info->priv);
+
+fail_priv_creation:
+ kfree(sgt_info);

fail_map_active_sgts:
fail_sgt_info_creation:
@@ -553,6 +603,10 @@ static void hyper_dmabuf_delayed_unexport(struct work_struct *work)
hyper_dmabuf_remove_exported(sgt_info->hid);
/* register hyper_dmabuf_id to the list for reuse */
store_reusable_hid(sgt_info->hid);
+
+ if (sgt_info->sz_priv > 0 && !sgt_info->priv)
+ kfree(sgt_info->priv);
+
kfree(sgt_info);
}
}
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c
index 152f9e3..ec37c3b 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c
@@ -65,10 +65,11 @@ void hyper_dmabuf_create_request(struct hyper_dmabuf_req *req,
* operands5 : offset of data in the first page
* operands6 : length of data in the last page
* operands7 : top-level reference number for shared pages
- * operands8~39 : Driver-specific private data (e.g. graphic buffer's meta info)
+ * operands8 : size of private data (from operands9)
+ * operands9 ~ : Driver-specific private data (e.g. graphic buffer's meta info)
*/

- memcpy(&req->operands[0], &operands[0], 40 * sizeof(int));
+ memcpy(&req->operands[0], &operands[0], 9 * sizeof(int) + operands[8]);
break;

case HYPER_DMABUF_NOTIFY_UNEXPORT:
@@ -135,7 +136,8 @@ void cmd_process_work(struct work_struct *work)
* operands5 : offset of data in the first page
* operands6 : length of data in the last page
* operands7 : top-level reference number for shared pages
- * operands8~11 : Driver-specific private data (e.g. graphic buffer's meta info)
+ * operands8 : size of private data (from operands9)
+ * operands9 ~ : Driver-specific private data (e.g. graphic buffer's meta info)
*/

/* if nents == 0, it means it is a message only for priv synchronization
@@ -152,8 +154,25 @@ void cmd_process_work(struct work_struct *work)
"Can't find imported sgt_info from IMPORT_LIST\n");
break;
}
- /* updating pri data */
- memcpy(&imported_sgt_info->priv[0], &req->operands[8], 32 * sizeof(int));
+
+ /* if size of new private data is different,
+ * we reallocate it. */
+ if (imported_sgt_info->sz_priv != req->operands[8]) {
+ kfree(imported_sgt_info->priv);
+ imported_sgt_info->sz_priv = req->operands[8];
+ imported_sgt_info->priv = kcalloc(1, req->operands[8], GFP_KERNEL);
+ if (!imported_sgt_info->priv) {
+ dev_err(hyper_dmabuf_private.device,
+ "Fail to allocate priv\n");
+
+ /* set it invalid */
+ imported_sgt_info->valid = 0;
+ break;
+ }
+ }
+
+ /* updating priv data */
+ memcpy(imported_sgt_info->priv, &req->operands[9], req->operands[8]);

#ifdef CONFIG_HYPER_DMABUF_EVENT_GEN
/* generating import event */
@@ -171,6 +190,17 @@ void cmd_process_work(struct work_struct *work)
break;
}

+ imported_sgt_info->sz_priv = req->operands[8];
+ imported_sgt_info->priv = kcalloc(1, req->operands[8], GFP_KERNEL);
+
+ if (!imported_sgt_info->priv) {
+ dev_err(hyper_dmabuf_private.device,
+ "Fail to allocate priv\n");
+
+ kfree(imported_sgt_info);
+ break;
+ }
+
imported_sgt_info->hid.id = req->operands[0];

for (i=0; i<3; i++)
@@ -190,7 +220,7 @@ void cmd_process_work(struct work_struct *work)
dev_dbg(hyper_dmabuf_private.device, "\tlast len %d\n", req->operands[6]);
dev_dbg(hyper_dmabuf_private.device, "\tgrefid %d\n", req->operands[7]);

- memcpy(&imported_sgt_info->priv[0], &req->operands[8], 32 * sizeof(int));
+ memcpy(imported_sgt_info->priv, &req->operands[9], req->operands[8]);

imported_sgt_info->valid = 1;
hyper_dmabuf_register_imported(imported_sgt_info);
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h
index 7464273..0f6e795 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h
@@ -25,7 +25,7 @@
#ifndef __HYPER_DMABUF_MSG_H__
#define __HYPER_DMABUF_MSG_H__

-#define MAX_NUMBER_OF_OPERANDS 40
+#define MAX_NUMBER_OF_OPERANDS 64

struct hyper_dmabuf_req {
unsigned int request_id;
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_sgl_proc.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_sgl_proc.c
index dd17d26..691a714 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_sgl_proc.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_sgl_proc.c
@@ -255,6 +255,7 @@ int hyper_dmabuf_cleanup_sgt_info(struct hyper_dmabuf_sgt_info *sgt_info, int fo
kfree(sgt_info->active_attached);
kfree(sgt_info->va_kmapped);
kfree(sgt_info->va_vmapped);
+ kfree(sgt_info->priv);

return 0;
}
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_struct.h b/drivers/xen/hyper_dmabuf/hyper_dmabuf_struct.h
index f01f535..6f929f2 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_struct.h
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_struct.h
@@ -57,6 +57,7 @@ struct hyper_dmabuf_pages_info {
struct page **pages; /* pages that contains reference numbers of shared pages*/
};

+
/* Both importer and exporter use this structure to point to sg lists
*
* Exporter stores references to sgt in a hash table
@@ -90,7 +91,9 @@ struct hyper_dmabuf_sgt_info {
* uses releases hyper_dmabuf device
*/
struct file *filp;
- int priv[32]; /* device specific info (e.g. image's meta info?) */
+
+ size_t sz_priv;
+ char *priv; /* device specific info (e.g. image's meta info?) */
};

/* Importer store references (before mapping) on shared pages
@@ -110,7 +113,9 @@ struct hyper_dmabuf_imported_sgt_info {
void *refs_info;
bool valid;
int num_importers;
- int priv[32]; /* device specific info (e.g. image's meta info?) */
+
+ size_t sz_priv;
+ char *priv; /* device specific info (e.g. image's meta info?) */
};

#endif /* __HYPER_DMABUF_STRUCT_H__ */
diff --git a/include/uapi/xen/hyper_dmabuf.h b/include/uapi/xen/hyper_dmabuf.h
index 3a6172e..df01b17 100644
--- a/include/uapi/xen/hyper_dmabuf.h
+++ b/include/uapi/xen/hyper_dmabuf.h
@@ -25,6 +25,8 @@
#ifndef __LINUX_PUBLIC_HYPER_DMABUF_H__
#define __LINUX_PUBLIC_HYPER_DMABUF_H__

+#define MAX_SIZE_PRIV_DATA 192
+
typedef struct {
int id;
int rng_key[3]; /* 12bytes long random number */
@@ -33,7 +35,7 @@ typedef struct {
struct hyper_dmabuf_event_hdr {
int event_type; /* one type only for now - new import */
hyper_dmabuf_id_t hid; /* hyper_dmabuf_id of specific hyper_dmabuf */
- size_t size; /* size of data */
+ int size; /* size of data */
};

struct hyper_dmabuf_event_data {
@@ -67,7 +69,8 @@ struct ioctl_hyper_dmabuf_export_remote {
int remote_domain;
/* exported dma buf id */
hyper_dmabuf_id_t hid;
- int priv[32];
+ int sz_priv;
+ char *priv;
};

#define IOCTL_HYPER_DMABUF_EXPORT_FD \
--
2.7.4