The number of Ion ioctls may continue to grow along with necessary validation. Pull it out into a separate file for easier management and review.

Signed-off-by: Laura Abbott labbott@redhat.com --- v3: Rebase to staging-next --- drivers/staging/android/ion/Makefile | 3 +- drivers/staging/android/ion/ion-ioctl.c | 144 ++++++++++++++++++++++ drivers/staging/android/ion/ion.c | 212 ++------------------------------ drivers/staging/android/ion/ion_priv.h | 91 ++++++++++++++ 4 files changed, 245 insertions(+), 205 deletions(-) create mode 100644 drivers/staging/android/ion/ion-ioctl.c

diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile index 18cc2aa..376c2b2 100644 --- a/drivers/staging/android/ion/Makefile +++ b/drivers/staging/android/ion/Makefile @@ -1,4 +1,5 @@ -obj-$(CONFIG_ION) += ion.o ion_heap.o ion_page_pool.o ion_system_heap.o \ +obj-$(CONFIG_ION) += ion.o ion-ioctl.o ion_heap.o \ + ion_page_pool.o ion_system_heap.o \ ion_carveout_heap.o ion_chunk_heap.o ion_cma_heap.o obj-$(CONFIG_ION_TEST) += ion_test.o ifdef CONFIG_COMPAT diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c new file mode 100644 index 0000000..341ba7d --- /dev/null +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -0,0 +1,144 @@ +/* + * + * Copyright (C) 2011 Google, Inc. + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#include <linux/kernel.h> +#include <linux/file.h> +#include <linux/fs.h> +#include <linux/uaccess.h> + +#include "ion.h" +#include "ion_priv.h" +#include "compat_ion.h" + +/* fix up the cases where the ioctl direction bits are incorrect */ +static unsigned int ion_ioctl_dir(unsigned int cmd) +{ + switch (cmd) { + case ION_IOC_SYNC: + case ION_IOC_FREE: + case ION_IOC_CUSTOM: + return _IOC_WRITE; + default: + return _IOC_DIR(cmd); + } +} + +long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) +{ + struct ion_client *client = filp->private_data; + struct ion_device *dev = client->dev; + struct ion_handle *cleanup_handle = NULL; + int ret = 0; + unsigned int dir; + + union { + struct ion_fd_data fd; + struct ion_allocation_data allocation; + struct ion_handle_data handle; + struct ion_custom_data custom; + } data; + + dir = ion_ioctl_dir(cmd); + + if (_IOC_SIZE(cmd) > sizeof(data)) + return -EINVAL; + + if (dir & _IOC_WRITE) + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) + return -EFAULT; + + switch (cmd) { + case ION_IOC_ALLOC: + { + struct ion_handle *handle; + + handle = ion_alloc(client, data.allocation.len, + data.allocation.align, + data.allocation.heap_id_mask, + data.allocation.flags); + if (IS_ERR(handle)) + return PTR_ERR(handle); + + data.allocation.handle = handle->id; + + cleanup_handle = handle; + break; + } + case ION_IOC_FREE: + { + struct ion_handle *handle; + + mutex_lock(&client->lock); + handle = ion_handle_get_by_id_nolock(client, data.handle.handle); + if (IS_ERR(handle)) { + mutex_unlock(&client->lock); + return PTR_ERR(handle); + } + ion_free_nolock(client, handle); + ion_handle_put_nolock(handle); + mutex_unlock(&client->lock); + break; + } + case ION_IOC_SHARE: + case ION_IOC_MAP: + { + struct ion_handle *handle; + + handle = ion_handle_get_by_id(client, data.handle.handle); + if (IS_ERR(handle)) + return PTR_ERR(handle); + data.fd.fd = ion_share_dma_buf_fd(client, handle); + ion_handle_put(handle); + if (data.fd.fd < 0) + ret = data.fd.fd; + break; + } + case ION_IOC_IMPORT: + { + struct ion_handle *handle; + + handle = ion_import_dma_buf_fd(client, data.fd.fd); + if (IS_ERR(handle)) + ret = PTR_ERR(handle); + else + data.handle.handle = handle->id; + break; + } + case ION_IOC_SYNC: + { + ret = ion_sync_for_device(client, data.fd.fd); + break; + } + case ION_IOC_CUSTOM: + { + if (!dev->custom_ioctl) + return -ENOTTY; + ret = dev->custom_ioctl(client, data.custom.cmd, + data.custom.arg); + break; + } + default: + return -ENOTTY; + } + + if (dir & _IOC_READ) { + if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) { + if (cleanup_handle) + ion_free(client, cleanup_handle); + return -EFAULT; + } + } + return ret; +} diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index 47de11a..a92804f 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -41,80 +41,6 @@ #include "ion_priv.h" #include "compat_ion.h"

-/** - * struct ion_device - the metadata of the ion device node - * @dev: the actual misc device - * @buffers: an rb tree of all the existing buffers - * @buffer_lock: lock protecting the tree of buffers - * @lock: rwsem protecting the tree of heaps and clients - * @heaps: list of all the heaps in the system - * @user_clients: list of all the clients created from userspace - */ -struct ion_device { - struct miscdevice dev; - struct rb_root buffers; - struct mutex buffer_lock; - struct rw_semaphore lock; - struct plist_head heaps; - long (*custom_ioctl)(struct ion_client *client, unsigned int cmd, - unsigned long arg); - struct rb_root clients; - struct dentry *debug_root; - struct dentry *heaps_debug_root; - struct dentry *clients_debug_root; -}; - -/** - * struct ion_client - a process/hw block local address space - * @node: node in the tree of all clients - * @dev: backpointer to ion device - * @handles: an rb tree of all the handles in this client - * @idr: an idr space for allocating handle ids - * @lock: lock protecting the tree of handles - * @name: used for debugging - * @display_name: used for debugging (unique version of @name) - * @display_serial: used for debugging (to make display_name unique) - * @task: used for debugging - * - * A client represents a list of buffers this client may access. - * The mutex stored here is used to protect both handles tree - * as well as the handles themselves, and should be held while modifying either. - */ -struct ion_client { - struct rb_node node; - struct ion_device *dev; - struct rb_root handles; - struct idr idr; - struct mutex lock; - const char *name; - char *display_name; - int display_serial; - struct task_struct *task; - pid_t pid; - struct dentry *debug_root; -}; - -/** - * ion_handle - a client local reference to a buffer - * @ref: reference count - * @client: back pointer to the client the buffer resides in - * @buffer: pointer to the buffer - * @node: node in the client's handle rbtree - * @kmap_cnt: count of times this client has mapped to kernel - * @id: client-unique id allocated by client->idr - * - * Modifications to node, map_cnt or mapping should be protected by the - * lock in the client. Other fields are never changed after initialization. - */ -struct ion_handle { - struct kref ref; - struct ion_client *client; - struct ion_buffer *buffer; - struct rb_node node; - unsigned int kmap_cnt; - int id; -}; - bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer) { return (buffer->flags & ION_FLAG_CACHED) && @@ -376,12 +302,12 @@ static void ion_handle_get(struct ion_handle *handle) kref_get(&handle->ref); }

-static int ion_handle_put_nolock(struct ion_handle *handle) +int ion_handle_put_nolock(struct ion_handle *handle) { return kref_put(&handle->ref, ion_handle_destroy); }

-static int ion_handle_put(struct ion_handle *handle) +int ion_handle_put(struct ion_handle *handle) { struct ion_client *client = handle->client; int ret; @@ -411,8 +337,8 @@ static struct ion_handle *ion_handle_lookup(struct ion_client *client, return ERR_PTR(-EINVAL); }

-static struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client, - int id) +struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client, + int id) { struct ion_handle *handle;

@@ -423,7 +349,7 @@ static struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client, return handle ? handle : ERR_PTR(-EINVAL); }

-static struct ion_handle *ion_handle_get_by_id(struct ion_client *client, +struct ion_handle *ion_handle_get_by_id(struct ion_client *client, int id) { struct ion_handle *handle; @@ -536,8 +462,8 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len, } EXPORT_SYMBOL(ion_alloc);

-static void ion_free_nolock(struct ion_client *client, - struct ion_handle *handle) +void ion_free_nolock(struct ion_client *client, + struct ion_handle *handle) { if (!ion_handle_validate(client, handle)) { WARN(1, "%s: invalid handle passed to free.\n", __func__); @@ -1209,7 +1135,7 @@ struct ion_handle *ion_import_dma_buf_fd(struct ion_client *client, int fd) } EXPORT_SYMBOL(ion_import_dma_buf_fd);

-static int ion_sync_for_device(struct ion_client *client, int fd) +int ion_sync_for_device(struct ion_client *client, int fd) { struct dma_buf *dmabuf; struct ion_buffer *buffer; @@ -1233,128 +1159,6 @@ static int ion_sync_for_device(struct ion_client *client, int fd) return 0; }

-/* fix up the cases where the ioctl direction bits are incorrect */ -static unsigned int ion_ioctl_dir(unsigned int cmd) -{ - switch (cmd) { - case ION_IOC_SYNC: - case ION_IOC_FREE: - case ION_IOC_CUSTOM: - return _IOC_WRITE; - default: - return _IOC_DIR(cmd); - } -} - -static long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) -{ - struct ion_client *client = filp->private_data; - struct ion_device *dev = client->dev; - struct ion_handle *cleanup_handle = NULL; - int ret = 0; - unsigned int dir; - - union { - struct ion_fd_data fd; - struct ion_allocation_data allocation; - struct ion_handle_data handle; - struct ion_custom_data custom; - } data; - - dir = ion_ioctl_dir(cmd); - - if (_IOC_SIZE(cmd) > sizeof(data)) - return -EINVAL; - - if (dir & _IOC_WRITE) - if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) - return -EFAULT; - - switch (cmd) { - case ION_IOC_ALLOC: - { - struct ion_handle *handle; - - handle = ion_alloc(client, data.allocation.len, - data.allocation.align, - data.allocation.heap_id_mask, - data.allocation.flags); - if (IS_ERR(handle)) - return PTR_ERR(handle); - - data.allocation.handle = handle->id; - - cleanup_handle = handle; - break; - } - case ION_IOC_FREE: - { - struct ion_handle *handle; - - mutex_lock(&client->lock); - handle = ion_handle_get_by_id_nolock(client, - data.handle.handle); - if (IS_ERR(handle)) { - mutex_unlock(&client->lock); - return PTR_ERR(handle); - } - ion_free_nolock(client, handle); - ion_handle_put_nolock(handle); - mutex_unlock(&client->lock); - break; - } - case ION_IOC_SHARE: - case ION_IOC_MAP: - { - struct ion_handle *handle; - - handle = ion_handle_get_by_id(client, data.handle.handle); - if (IS_ERR(handle)) - return PTR_ERR(handle); - data.fd.fd = ion_share_dma_buf_fd(client, handle); - ion_handle_put(handle); - if (data.fd.fd < 0) - ret = data.fd.fd; - break; - } - case ION_IOC_IMPORT: - { - struct ion_handle *handle; - - handle = ion_import_dma_buf_fd(client, data.fd.fd); - if (IS_ERR(handle)) - ret = PTR_ERR(handle); - else - data.handle.handle = handle->id; - break; - } - case ION_IOC_SYNC: - { - ret = ion_sync_for_device(client, data.fd.fd); - break; - } - case ION_IOC_CUSTOM: - { - if (!dev->custom_ioctl) - return -ENOTTY; - ret = dev->custom_ioctl(client, data.custom.cmd, - data.custom.arg); - break; - } - default: - return -ENOTTY; - } - - if (dir & _IOC_READ) { - if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) { - if (cleanup_handle) - ion_free(client, cleanup_handle); - return -EFAULT; - } - } - return ret; -} - static int ion_release(struct inode *inode, struct file *file) { struct ion_client *client = file->private_data; diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h index fcbc231..4a78fab 100644 --- a/drivers/staging/android/ion/ion_priv.h +++ b/drivers/staging/android/ion/ion_priv.h @@ -26,6 +26,7 @@ #include <linux/sched.h> #include <linux/shrinker.h> #include <linux/types.h> +#include <linux/miscdevice.h>

#include "ion.h"

@@ -81,6 +82,80 @@ struct ion_buffer { void ion_buffer_destroy(struct ion_buffer *buffer);

/** + * struct ion_device - the metadata of the ion device node + * @dev: the actual misc device + * @buffers: an rb tree of all the existing buffers + * @buffer_lock: lock protecting the tree of buffers + * @lock: rwsem protecting the tree of heaps and clients + * @heaps: list of all the heaps in the system + * @user_clients: list of all the clients created from userspace + */ +struct ion_device { + struct miscdevice dev; + struct rb_root buffers; + struct mutex buffer_lock; + struct rw_semaphore lock; + struct plist_head heaps; + long (*custom_ioctl)(struct ion_client *client, unsigned int cmd, + unsigned long arg); + struct rb_root clients; + struct dentry *debug_root; + struct dentry *heaps_debug_root; + struct dentry *clients_debug_root; +}; + +/** + * struct ion_client - a process/hw block local address space + * @node: node in the tree of all clients + * @dev: backpointer to ion device + * @handles: an rb tree of all the handles in this client + * @idr: an idr space for allocating handle ids + * @lock: lock protecting the tree of handles + * @name: used for debugging + * @display_name: used for debugging (unique version of @name) + * @display_serial: used for debugging (to make display_name unique) + * @task: used for debugging + * + * A client represents a list of buffers this client may access. + * The mutex stored here is used to protect both handles tree + * as well as the handles themselves, and should be held while modifying either. + */ +struct ion_client { + struct rb_node node; + struct ion_device *dev; + struct rb_root handles; + struct idr idr; + struct mutex lock; + const char *name; + char *display_name; + int display_serial; + struct task_struct *task; + pid_t pid; + struct dentry *debug_root; +}; + +/** + * ion_handle - a client local reference to a buffer + * @ref: reference count + * @client: back pointer to the client the buffer resides in + * @buffer: pointer to the buffer + * @node: node in the client's handle rbtree + * @kmap_cnt: count of times this client has mapped to kernel + * @id: client-unique id allocated by client->idr + * + * Modifications to node, map_cnt or mapping should be protected by the + * lock in the client. Other fields are never changed after initialization. + */ +struct ion_handle { + struct kref ref; + struct ion_client *client; + struct ion_buffer *buffer; + struct rb_node node; + unsigned int kmap_cnt; + int id; +}; + +/** * struct ion_heap_ops - ops to operate on a given heap * @allocate: allocate memory * @free: free memory @@ -376,4 +451,20 @@ int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask, void ion_pages_sync_for_device(struct device *dev, struct page *page, size_t size, enum dma_data_direction dir);

+long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg); + +int ion_sync_for_device(struct ion_client *client, int fd); + +struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client, + int id); + +void ion_free_nolock(struct ion_client *client, struct ion_handle *handle); + +int ion_handle_put_nolock(struct ion_handle *handle); + +struct ion_handle *ion_handle_get_by_id(struct ion_client *client, + int id); + +int ion_handle_put(struct ion_handle *handle); + #endif /* _ION_PRIV_H */

Ion clients currently lack a good method to determine what heaps are available and what ids they map to. This leads to tight coupling between user and kernel space and headaches. Add a query ioctl to let userspace know the availability of heaps.

Signed-off-by: Laura Abbott labbott@redhat.com --- v3: Include some refactoring that was previously part of the dropped ABI ioctl. Use u64_to_user_ptr to avoid warnings and ugly double casts. General rebase due to dropped ABI ioctl. --- drivers/staging/android/ion/ion-ioctl.c | 53 ++++++++++++++++++++++++++------- drivers/staging/android/ion/ion.c | 43 ++++++++++++++++++++++++++ drivers/staging/android/ion/ion_priv.h | 3 ++ drivers/staging/android/uapi/ion.h | 39 ++++++++++++++++++++++++ 4 files changed, 128 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c index 341ba7d..7e7431d 100644 --- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -22,6 +22,31 @@ #include "ion_priv.h" #include "compat_ion.h"

+union ion_ioctl_arg { + struct ion_fd_data fd; + struct ion_allocation_data allocation; + struct ion_handle_data handle; + struct ion_custom_data custom; + struct ion_heap_query query; +}; + +static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) +{ + int ret = 0; + + switch (cmd) { + case ION_IOC_HEAP_QUERY: + ret = arg->query.reserved0 != 0; + ret |= arg->query.reserved1 != 0; + ret |= arg->query.reserved2 != 0; + break; + default: + break; + } + + return ret ? -EINVAL : 0; +} + /* fix up the cases where the ioctl direction bits are incorrect */ static unsigned int ion_ioctl_dir(unsigned int cmd) { @@ -42,22 +67,27 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) struct ion_handle *cleanup_handle = NULL; int ret = 0; unsigned int dir; - - union { - struct ion_fd_data fd; - struct ion_allocation_data allocation; - struct ion_handle_data handle; - struct ion_custom_data custom; - } data; + union ion_ioctl_arg data;

dir = ion_ioctl_dir(cmd);

if (_IOC_SIZE(cmd) > sizeof(data)) return -EINVAL;

- if (dir & _IOC_WRITE) - if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) - return -EFAULT; + /* + * The copy_from_user is unconditional here for both read and write + * to do the validate. If there is no write for the ioctl, the + * buffer is cleared + */ + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) + return -EFAULT; + + ret = validate_ioctl_arg(cmd, &data); + if (WARN_ON_ONCE(ret)) + return ret; + + if (!(dir & _IOC_WRITE)) + memset(&data, 0, sizeof(data));

switch (cmd) { case ION_IOC_ALLOC: @@ -129,6 +159,9 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) data.custom.arg); break; } + case ION_IOC_HEAP_QUERY: + ret = ion_query_heaps(client, &data.query); + break; default: return -ENOTTY; } diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index a92804f..396ded5 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -1159,6 +1159,48 @@ int ion_sync_for_device(struct ion_client *client, int fd) return 0; }

+int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query) +{ + struct ion_device *dev = client->dev; + struct ion_heap_data __user *buffer = u64_to_user_ptr(query->heaps); + int ret = -EINVAL, cnt = 0, max_cnt; + struct ion_heap *heap; + struct ion_heap_data hdata; + + memset(&hdata, 0, sizeof(hdata)); + + down_read(&dev->lock); + if (!buffer) { + query->cnt = dev->heap_cnt; + ret = 0; + goto out; + } + + if (query->cnt <= 0) + goto out; + + max_cnt = query->cnt; + + plist_for_each_entry(heap, &dev->heaps, node) { + strncpy(hdata.name, heap->name, MAX_HEAP_NAME); + hdata.name[sizeof(hdata.name) - 1] = '\0'; + hdata.type = heap->type; + hdata.heap_id = heap->id; + + ret = copy_to_user(&buffer[cnt], + &hdata, sizeof(hdata)); + + cnt++; + if (cnt >= max_cnt) + break; + } + + query->cnt = cnt; +out: + up_read(&dev->lock); + return ret; +} + static int ion_release(struct inode *inode, struct file *file) { struct ion_client *client = file->private_data; @@ -1376,6 +1418,7 @@ void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap) } }

+ dev->heap_cnt++; up_write(&dev->lock); } EXPORT_SYMBOL(ion_device_add_heap); diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h index 4a78fab..3c3b324 100644 --- a/drivers/staging/android/ion/ion_priv.h +++ b/drivers/staging/android/ion/ion_priv.h @@ -102,6 +102,7 @@ struct ion_device { struct dentry *debug_root; struct dentry *heaps_debug_root; struct dentry *clients_debug_root; + int heap_cnt; };

/** @@ -467,4 +468,6 @@ struct ion_handle *ion_handle_get_by_id(struct ion_client *client,

int ion_handle_put(struct ion_handle *handle);

+int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query); + #endif /* _ION_PRIV_H */ diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h index a9c4e8b..647f130 100644 --- a/drivers/staging/android/uapi/ion.h +++ b/drivers/staging/android/uapi/ion.h @@ -128,6 +128,36 @@ struct ion_custom_data { unsigned long arg; };

+#define MAX_HEAP_NAME 32 + +/** + * struct ion_heap_data - data about a heap + * @name - first 32 characters of the heap name + * @type - heap type + * @heap_id - heap id for the heap + */ +struct ion_heap_data { + char name[MAX_HEAP_NAME]; + __u32 type; + __u32 heap_id; + __u32 reserved0; + __u32 reserved1; + __u32 reserved2; +}; + +/** + * struct ion_heap_query - collection of data about all heaps + * @cnt - total number of heaps to be copied + * @heaps - buffer to copy heap data + */ +struct ion_heap_query { + __u32 cnt; /* Total number of heaps to be copied */ + __u32 reserved0; /* align to 64bits */ + __u64 heaps; /* buffer to be populated */ + __u32 reserved1; + __u32 reserved2; +}; + #define ION_IOC_MAGIC 'I'

/** @@ -194,4 +224,13 @@ struct ion_custom_data { */ #define ION_IOC_CUSTOM _IOWR(ION_IOC_MAGIC, 6, struct ion_custom_data)

+/** + * DOC: ION_IOC_HEAP_QUERY - information about available heaps + * + * Takes an ion_heap_query structure and populates information about + * available Ion heaps. + */ +#define ION_IOC_HEAP_QUERY _IOWR(ION_IOC_MAGIC, 8, \ + struct ion_heap_query) + #endif /* _UAPI_LINUX_ION_H */