Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1478558 > unrolled thread

[PATCHv3 2/2] staging: android: ion: Add ioctl to query available heaps

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCHv3 2/2] staging: android: ion: Add ioctl to query available heaps Laura Abbott <labbott@redhat.com> - 2016-09-07 21:00 +0200
    Re: [Linaro-mm-sig] [PATCHv3 2/2] staging: android: ion: Add ioctl to query available heaps Arnd Bergmann <arnd@arndb.de> - 2016-09-07 21:40 +0200
      Re: [Linaro-mm-sig] [PATCHv3 2/2] staging: android: ion: Add ioctl to  query available heaps Laura Abbott <labbott@redhat.com> - 2016-09-08 02:20 +0200

#1478558 — [PATCHv3 2/2] staging: android: ion: Add ioctl to query available heaps

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 */
-- 
2.7.4

[toc] | [next] | [standalone]

#1478576 — Re: [Linaro-mm-sig] [PATCHv3 2/2] staging: android: ion: Add ioctl to query available heaps

On Wednesday, September 7, 2016 11:49:59 AM CEST Laura Abbott wrote:

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

I noticed that the WARN_ON_ONCE warns about invalid user input,
but I think we tend to normally just use WARN_ON for things that
go wrong inside of the kernel or in hardware. 

Maybe better use printk_once() or printk_ratelimited.

Is there any noticeable overhead in always copying the structure?
copy_from_user() can be a bit slow depending on debugging or
security features, and it seems unnecessary if the validation
is only done for one of the commands.

Otherwise the patch looks good to me.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1478687 — Re: [Linaro-mm-sig] [PATCHv3 2/2] staging: android: ion: Add ioctl to query available heaps

On 09/07/2016 12:37 PM, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 11:49:59 AM CEST Laura Abbott wrote:
>
>> -	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;
>
> I noticed that the WARN_ON_ONCE warns about invalid user input,
> but I think we tend to normally just use WARN_ON for things that
> go wrong inside of the kernel or in hardware.
>
> Maybe better use printk_once() or printk_ratelimited.
>

Sure, the error code should hopefully be enough of a hint to
userspace to maybe check the log.

> Is there any noticeable overhead in always copying the structure?
> copy_from_user() can be a bit slow depending on debugging or
> security features, and it seems unnecessary if the validation
> is only done for one of the commands.
>

Good point. It made sense with some of the other ioctls (specifically
the ABI) but isn't necessary now. We can evaluate later when other
ioctls get added.

> Otherwise the patch looks good to me.
>
> 	Arnd
>

Thanks!

Laura

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web