[PATCH v5 0/2] staging: ion: get one device per heap

[PATCH v5 0/2] staging: ion: get one device per heap

[Benjamin Gaignard]

version 5:
- create a configuration flag to keep legacy Ion misc device

version 4:
- add a configuration flag to switch between legacy Ion misc device
  and one device per heap version.
  This change has been suggested after Laura talks at XDC 2017.

version 3:
- change ion_device_add_heap prototype to return a possible error.

version 2:
- simplify ioctl check like propose by Dan
- make sure that we don't register more than ION_DEV_MAX heaps.

Until now all ion heaps are addressing using the same device "/dev/ion".
This way of working doesn't allow to give access rights (for example with
SElinux rules) per heap.
This series propose to have one device "/dev/ionX" per heap.
Query heaps informations will be possible on each device node but
allocation request will only be possible if heap_mask_id match with device minor number.

Using legacy Ion misc device is still by setting ION_LEGACY_DEVICE_API
configuration flag.

Benjamin Gaignard (2):
  staging: ion: simplify ioctl args checking function
  staging: ion: create one device entry per heap

 drivers/staging/android/TODO            |  1 -
 drivers/staging/android/ion/Kconfig     |  7 +++++++
 drivers/staging/android/ion/ion-ioctl.c | 29 +++++++++++++++++++++--------
 drivers/staging/android/ion/ion.c       | 31 ++++++++++++++++++++++++++++++-
 drivers/staging/android/ion/ion.h       | 15 +++++++++++++--
 5 files changed, 71 insertions(+), 12 deletions(-)

-- 
2.7.4

[PATCH v5 0/2] staging: ion: get one device per heap

[Benjamin Gaignard]

version 5:
- create a configuration flag to keep legacy Ion misc device

version 4:
- add a configuration flag to switch between legacy Ion misc device
  and one device per heap version.
  This change has been suggested after Laura talks at XDC 2017.

version 3:
- change ion_device_add_heap prototype to return a possible error.

version 2:
- simplify ioctl check like propose by Dan
- make sure that we don't register more than ION_DEV_MAX heaps.

Until now all ion heaps are addressing using the same device "/dev/ion".
This way of working doesn't allow to give access rights (for example with
SElinux rules) per heap.
This series propose to have one device "/dev/ionX" per heap.
Query heaps informations will be possible on each device node but
allocation request will only be possible if heap_mask_id match with device minor number.

Using legacy Ion misc device is still by setting ION_LEGACY_DEVICE_API
configuration flag.

Benjamin Gaignard (2):
  staging: ion: simplify ioctl args checking function
  staging: ion: create one device entry per heap

 drivers/staging/android/TODO            |  1 -
 drivers/staging/android/ion/Kconfig     |  7 +++++++
 drivers/staging/android/ion/ion-ioctl.c | 29 +++++++++++++++++++++--------
 drivers/staging/android/ion/ion.c       | 31 ++++++++++++++++++++++++++++++-
 drivers/staging/android/ion/ion.h       | 15 +++++++++++++--
 5 files changed, 71 insertions(+), 12 deletions(-)

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 1/2] staging: ion: simplify ioctl args checking function

[Benjamin Gaignard]

Make arguments checking more easy to read.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/staging/android/ion/ion-ioctl.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index d9f8b14..e26b786 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -27,19 +27,18 @@ union ion_ioctl_arg {
 
 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;
+		if (arg->query.reserved0 ||
+		    arg->query.reserved1 ||
+		    arg->query.reserved2)
+			return -EINVAL;
 		break;
 	default:
 		break;
 	}
 
-	return ret ? -EINVAL : 0;
+	return 0;
 }
 
 /* fix up the cases where the ioctl direction bits are incorrect */
-- 
2.7.4

[PATCH v5 1/2] staging: ion: simplify ioctl args checking function

[Benjamin Gaignard]

Make arguments checking more easy to read.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/staging/android/ion/ion-ioctl.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index d9f8b14..e26b786 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -27,19 +27,18 @@ union ion_ioctl_arg {
 
 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;
+		if (arg->query.reserved0 ||
+		    arg->query.reserved1 ||
+		    arg->query.reserved2)
+			return -EINVAL;
 		break;
 	default:
 		break;
 	}
 
-	return ret ? -EINVAL : 0;
+	return 0;
 }
 
 /* fix up the cases where the ioctl direction bits are incorrect */
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

Instead a getting only one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
version 5:
- create a configuration flag to keep legacy Ion misc device

version 4:
- add a configuration flag to switch between legacy Ion misc device
  and one device per heap version.

version 3:
- change ion_device_add_heap prototype to return a possible error.

version 2:
- simplify ioctl check like propose by Dan
- make sure that we don't register more than ION_DEV_MAX heaps.

 drivers/staging/android/TODO            |  1 -
 drivers/staging/android/ion/Kconfig     |  7 +++++++
 drivers/staging/android/ion/ion-ioctl.c | 18 ++++++++++++++++--
 drivers/staging/android/ion/ion.c       | 31 ++++++++++++++++++++++++++++++-
 drivers/staging/android/ion/ion.h       | 15 +++++++++++++--
 5 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 5f14247..d770ffa 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO
@@ -9,7 +9,6 @@ TODO:
 ion/
  - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
    involve putting appropriate bindings in a memory node for Ion to find.
- - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
  - Better test framework (integration with VGEM was suggested)
 
 Please send patches to Greg Kroah-Hartman <greg@kroah.com> and Cc:
diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index a517b2d..cb4666e 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -10,6 +10,13 @@ menuconfig ION
 	  If you're not using Android its probably safe to
 	  say N here.
 
+config ION_LEGACY_DEVICE_API
+	bool "Keep using Ion legacy misc device API"
+	depends on ION
+	help
+	  Choose this option to keep using Ion legacy misc device API
+	  i.e. /dev/ion
+
 config ION_SYSTEM_HEAP
 	bool "Ion system heap"
 	depends on ION
diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index e26b786..bb5c77b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -25,7 +25,8 @@ union ion_ioctl_arg {
 	struct ion_heap_query query;
 };
 
-static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
+static int validate_ioctl_arg(struct file *filp,
+			      unsigned int cmd, union ion_ioctl_arg *arg)
 {
 	switch (cmd) {
 	case ION_IOC_HEAP_QUERY:
@@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
 		    arg->query.reserved2 )
 			return -EINVAL;
 		break;
+
+	case ION_IOC_ALLOC:
+	{
+		int mask = 1 << iminor(filp->f_inode);
+
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+		if (imajor(filp->f_inode) == MISC_MAJOR)
+			return 0;
+#endif
+		if (!(arg->allocation.heap_id_mask & mask))
+			return -EINVAL;
+		break;
+	}
 	default:
 		break;
 	}
@@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
 		return -EFAULT;
 
-	ret = validate_ioctl_arg(cmd, &data);
+	ret = validate_ioctl_arg(filp, cmd, &data);
 	if (WARN_ON_ONCE(ret))
 		return ret;
 
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 93e2c90..092b24c 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -40,6 +40,8 @@
 
 #include "ion.h"
 
+#define ION_DEV_MAX 32
+
 static struct ion_device *internal_dev;
 static int heap_id;
 
@@ -537,15 +539,28 @@ static int debug_shrink_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
 			debug_shrink_set, "%llu\n");
 
-void ion_device_add_heap(struct ion_heap *heap)
+int ion_device_add_heap(struct ion_heap *heap)
 {
 	struct dentry *debug_file;
 	struct ion_device *dev = internal_dev;
+	int ret = 0;
 
 	if (!heap->ops->allocate || !heap->ops->free)
 		pr_err("%s: can not add heap with invalid ops struct.\n",
 		       __func__);
 
+	if (heap_id >= ION_DEV_MAX)
+		return -EBUSY;
+
+	heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
+	dev_set_name(&heap->ddev, "ion%d", heap_id);
+	device_initialize(&heap->ddev);
+	cdev_init(&heap->chrdev, &ion_fops);
+	heap->chrdev.owner = THIS_MODULE;
+	ret = cdev_device_add(&heap->chrdev, &heap->ddev);
+	if (ret < 0)
+		return ret;
+
 	spin_lock_init(&heap->free_lock);
 	heap->free_list_size = 0;
 
@@ -583,6 +598,8 @@ void ion_device_add_heap(struct ion_heap *heap)
 
 	dev->heap_cnt++;
 	up_write(&dev->lock);
+
+	return ret;
 }
 EXPORT_SYMBOL(ion_device_add_heap);
 
@@ -595,6 +612,7 @@ static int ion_device_create(void)
 	if (!idev)
 		return -ENOMEM;
 
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
 	idev->dev.minor = MISC_DYNAMIC_MINOR;
 	idev->dev.name = "ion";
 	idev->dev.fops = &ion_fops;
@@ -605,6 +623,17 @@ static int ion_device_create(void)
 		kfree(idev);
 		return ret;
 	}
+#endif
+
+	ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
+	if (ret) {
+		pr_err("ion: unable to allocate device\n");
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+		misc_deregister(&idev->dev);
+#endif
+		kfree(idev);
+		return ret;
+	}
 
 	idev->debug_root = debugfs_create_dir("ion", NULL);
 	if (!idev->debug_root) {
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index 621e5f7..2b00ccb 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -17,16 +17,19 @@
 #ifndef _ION_H
 #define _ION_H
 
+#include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/dma-direction.h>
 #include <linux/kref.h>
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+#include <linux/miscdevice.h>
+#endif
 #include <linux/mm_types.h>
 #include <linux/mutex.h>
 #include <linux/rbtree.h>
 #include <linux/sched.h>
 #include <linux/shrinker.h>
 #include <linux/types.h>
-#include <linux/miscdevice.h>
 
 #include "../uapi/ion.h"
 
@@ -91,12 +94,16 @@ void ion_buffer_destroy(struct ion_buffer *buffer);
 /**
  * struct ion_device - the metadata of the ion device node
  * @dev:		the actual misc device
+ * @devt:		Ion 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
  */
 struct ion_device {
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
 	struct miscdevice dev;
+#endif
+	dev_t devt;
 	struct rb_root buffers;
 	struct mutex buffer_lock;
 	struct rw_semaphore lock;
@@ -152,6 +159,8 @@ struct ion_heap_ops {
  * struct ion_heap - represents a heap in the system
  * @node:		rb node to put the heap on the device's tree of heaps
  * @dev:		back pointer to the ion_device
+ * @ddev:		device structure
+ * @chrdev:		associated character device
  * @type:		type of heap
  * @ops:		ops struct as above
  * @flags:		flags
@@ -176,6 +185,8 @@ struct ion_heap_ops {
 struct ion_heap {
 	struct plist_node node;
 	struct ion_device *dev;
+	struct device ddev;
+	struct cdev chrdev;
 	enum ion_heap_type type;
 	struct ion_heap_ops *ops;
 	unsigned long flags;
@@ -212,7 +223,7 @@ bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer);
  * ion_device_add_heap - adds a heap to the ion device
  * @heap:		the heap to add
  */
-void ion_device_add_heap(struct ion_heap *heap);
+int ion_device_add_heap(struct ion_heap *heap);
 
 /**
  * some helpers for common operations on buffers using the sg_table
-- 
2.7.4

[PATCH v5 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

Instead a getting only one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
version 5:
- create a configuration flag to keep legacy Ion misc device

version 4:
- add a configuration flag to switch between legacy Ion misc device
  and one device per heap version.

version 3:
- change ion_device_add_heap prototype to return a possible error.

version 2:
- simplify ioctl check like propose by Dan
- make sure that we don't register more than ION_DEV_MAX heaps.

 drivers/staging/android/TODO            |  1 -
 drivers/staging/android/ion/Kconfig     |  7 +++++++
 drivers/staging/android/ion/ion-ioctl.c | 18 ++++++++++++++++--
 drivers/staging/android/ion/ion.c       | 31 ++++++++++++++++++++++++++++++-
 drivers/staging/android/ion/ion.h       | 15 +++++++++++++--
 5 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 5f14247..d770ffa 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO
@@ -9,7 +9,6 @@ TODO:
 ion/
  - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
    involve putting appropriate bindings in a memory node for Ion to find.
- - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
  - Better test framework (integration with VGEM was suggested)
 
 Please send patches to Greg Kroah-Hartman <greg@kroah.com> and Cc:
diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index a517b2d..cb4666e 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -10,6 +10,13 @@ menuconfig ION
 	  If you're not using Android its probably safe to
 	  say N here.
 
+config ION_LEGACY_DEVICE_API
+	bool "Keep using Ion legacy misc device API"
+	depends on ION
+	help
+	  Choose this option to keep using Ion legacy misc device API
+	  i.e. /dev/ion
+
 config ION_SYSTEM_HEAP
 	bool "Ion system heap"
 	depends on ION
diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index e26b786..bb5c77b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -25,7 +25,8 @@ union ion_ioctl_arg {
 	struct ion_heap_query query;
 };
 
-static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
+static int validate_ioctl_arg(struct file *filp,
+			      unsigned int cmd, union ion_ioctl_arg *arg)
 {
 	switch (cmd) {
 	case ION_IOC_HEAP_QUERY:
@@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
 		    arg->query.reserved2 )
 			return -EINVAL;
 		break;
+
+	case ION_IOC_ALLOC:
+	{
+		int mask = 1 << iminor(filp->f_inode);
+
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+		if (imajor(filp->f_inode) == MISC_MAJOR)
+			return 0;
+#endif
+		if (!(arg->allocation.heap_id_mask & mask))
+			return -EINVAL;
+		break;
+	}
 	default:
 		break;
 	}
@@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
 		return -EFAULT;
 
-	ret = validate_ioctl_arg(cmd, &data);
+	ret = validate_ioctl_arg(filp, cmd, &data);
 	if (WARN_ON_ONCE(ret))
 		return ret;
 
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 93e2c90..092b24c 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -40,6 +40,8 @@
 
 #include "ion.h"
 
+#define ION_DEV_MAX 32
+
 static struct ion_device *internal_dev;
 static int heap_id;
 
@@ -537,15 +539,28 @@ static int debug_shrink_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
 			debug_shrink_set, "%llu\n");
 
-void ion_device_add_heap(struct ion_heap *heap)
+int ion_device_add_heap(struct ion_heap *heap)
 {
 	struct dentry *debug_file;
 	struct ion_device *dev = internal_dev;
+	int ret = 0;
 
 	if (!heap->ops->allocate || !heap->ops->free)
 		pr_err("%s: can not add heap with invalid ops struct.\n",
 		       __func__);
 
+	if (heap_id >= ION_DEV_MAX)
+		return -EBUSY;
+
+	heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
+	dev_set_name(&heap->ddev, "ion%d", heap_id);
+	device_initialize(&heap->ddev);
+	cdev_init(&heap->chrdev, &ion_fops);
+	heap->chrdev.owner = THIS_MODULE;
+	ret = cdev_device_add(&heap->chrdev, &heap->ddev);
+	if (ret < 0)
+		return ret;
+
 	spin_lock_init(&heap->free_lock);
 	heap->free_list_size = 0;
 
@@ -583,6 +598,8 @@ void ion_device_add_heap(struct ion_heap *heap)
 
 	dev->heap_cnt++;
 	up_write(&dev->lock);
+
+	return ret;
 }
 EXPORT_SYMBOL(ion_device_add_heap);
 
@@ -595,6 +612,7 @@ static int ion_device_create(void)
 	if (!idev)
 		return -ENOMEM;
 
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
 	idev->dev.minor = MISC_DYNAMIC_MINOR;
 	idev->dev.name = "ion";
 	idev->dev.fops = &ion_fops;
@@ -605,6 +623,17 @@ static int ion_device_create(void)
 		kfree(idev);
 		return ret;
 	}
+#endif
+
+	ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
+	if (ret) {
+		pr_err("ion: unable to allocate device\n");
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+		misc_deregister(&idev->dev);
+#endif
+		kfree(idev);
+		return ret;
+	}
 
 	idev->debug_root = debugfs_create_dir("ion", NULL);
 	if (!idev->debug_root) {
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index 621e5f7..2b00ccb 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -17,16 +17,19 @@
 #ifndef _ION_H
 #define _ION_H
 
+#include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/dma-direction.h>
 #include <linux/kref.h>
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+#include <linux/miscdevice.h>
+#endif
 #include <linux/mm_types.h>
 #include <linux/mutex.h>
 #include <linux/rbtree.h>
 #include <linux/sched.h>
 #include <linux/shrinker.h>
 #include <linux/types.h>
-#include <linux/miscdevice.h>
 
 #include "../uapi/ion.h"
 
@@ -91,12 +94,16 @@ void ion_buffer_destroy(struct ion_buffer *buffer);
 /**
  * struct ion_device - the metadata of the ion device node
  * @dev:		the actual misc device
+ * @devt:		Ion 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
  */
 struct ion_device {
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
 	struct miscdevice dev;
+#endif
+	dev_t devt;
 	struct rb_root buffers;
 	struct mutex buffer_lock;
 	struct rw_semaphore lock;
@@ -152,6 +159,8 @@ struct ion_heap_ops {
  * struct ion_heap - represents a heap in the system
  * @node:		rb node to put the heap on the device's tree of heaps
  * @dev:		back pointer to the ion_device
+ * @ddev:		device structure
+ * @chrdev:		associated character device
  * @type:		type of heap
  * @ops:		ops struct as above
  * @flags:		flags
@@ -176,6 +185,8 @@ struct ion_heap_ops {
 struct ion_heap {
 	struct plist_node node;
 	struct ion_device *dev;
+	struct device ddev;
+	struct cdev chrdev;
 	enum ion_heap_type type;
 	struct ion_heap_ops *ops;
 	unsigned long flags;
@@ -212,7 +223,7 @@ bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer);
  * ion_device_add_heap - adds a heap to the ion device
  * @heap:		the heap to add
  */
-void ion_device_add_heap(struct ion_heap *heap);
+int ion_device_add_heap(struct ion_heap *heap);
 
 /**
  * some helpers for common operations on buffers using the sg_table
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 09/27/2017 06:20 AM, Benjamin Gaignard wrote:
> Instead a getting only one common device "/dev/ion" for
> all the heaps this patch allow to create one device
> entry ("/dev/ionX") per heap.
> Getting an entry per heap could allow to set security rules
> per heap and global ones for all heaps.
> 
> Allocation requests will be only allowed if the mask_id
> match with device minor.
> Query request could be done on any of the devices.
>
Thinking about this a bit more, I'm not 100% sure if this
will allow the security rules we want. Heap ids are assigned
dynamically and therefore so will the /dev/ionX designation.
>From my understanding, security rules like selinux need to
be fully specified at boot time so I'm not sure how you would
be able to write rules to differentiate between /dev/ionX and
/dev/ionY without knowing the values at boottime.

So I think we need a different way to match the heap ids to
get the security we want unless my understanding of security
policies is wrong and we can dynamically specify permissions.

Thanks,
Laura
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> ---
> version 5:
> - create a configuration flag to keep legacy Ion misc device
> 
> version 4:
> - add a configuration flag to switch between legacy Ion misc device
>   and one device per heap version.
> 
> version 3:
> - change ion_device_add_heap prototype to return a possible error.
> 
> version 2:
> - simplify ioctl check like propose by Dan
> - make sure that we don't register more than ION_DEV_MAX heaps.
> 
>  drivers/staging/android/TODO            |  1 -
>  drivers/staging/android/ion/Kconfig     |  7 +++++++
>  drivers/staging/android/ion/ion-ioctl.c | 18 ++++++++++++++++--
>  drivers/staging/android/ion/ion.c       | 31 ++++++++++++++++++++++++++++++-
>  drivers/staging/android/ion/ion.h       | 15 +++++++++++++--
>  5 files changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
> index 5f14247..d770ffa 100644
> --- a/drivers/staging/android/TODO
> +++ b/drivers/staging/android/TODO
> @@ -9,7 +9,6 @@ TODO:
>  ion/
>   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
>     involve putting appropriate bindings in a memory node for Ion to find.
> - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
>   - Better test framework (integration with VGEM was suggested)
>  
>  Please send patches to Greg Kroah-Hartman <greg@kroah.com> and Cc:
> diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
> index a517b2d..cb4666e 100644
> --- a/drivers/staging/android/ion/Kconfig
> +++ b/drivers/staging/android/ion/Kconfig
> @@ -10,6 +10,13 @@ menuconfig ION
>  	  If you're not using Android its probably safe to
>  	  say N here.
>  
> +config ION_LEGACY_DEVICE_API
> +	bool "Keep using Ion legacy misc device API"
> +	depends on ION
> +	help
> +	  Choose this option to keep using Ion legacy misc device API
> +	  i.e. /dev/ion
> +
>  config ION_SYSTEM_HEAP
>  	bool "Ion system heap"
>  	depends on ION
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index e26b786..bb5c77b 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -25,7 +25,8 @@ union ion_ioctl_arg {
>  	struct ion_heap_query query;
>  };
>  
> -static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> +static int validate_ioctl_arg(struct file *filp,
> +			      unsigned int cmd, union ion_ioctl_arg *arg)
>  {
>  	switch (cmd) {
>  	case ION_IOC_HEAP_QUERY:
> @@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>  		    arg->query.reserved2 )
>  			return -EINVAL;
>  		break;
> +
> +	case ION_IOC_ALLOC:
> +	{
> +		int mask = 1 << iminor(filp->f_inode);
> +
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> +		if (imajor(filp->f_inode) == MISC_MAJOR)
> +			return 0;
> +#endif
> +		if (!(arg->allocation.heap_id_mask & mask))
> +			return -EINVAL;
> +		break;
> +	}
>  	default:
>  		break;
>  	}
> @@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
>  		return -EFAULT;
>  
> -	ret = validate_ioctl_arg(cmd, &data);
> +	ret = validate_ioctl_arg(filp, cmd, &data);
>  	if (WARN_ON_ONCE(ret))
>  		return ret;
>  
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 93e2c90..092b24c 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -40,6 +40,8 @@
>  
>  #include "ion.h"
>  
> +#define ION_DEV_MAX 32
> +
>  static struct ion_device *internal_dev;
>  static int heap_id;
>  
> @@ -537,15 +539,28 @@ static int debug_shrink_get(void *data, u64 *val)
>  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>  			debug_shrink_set, "%llu\n");
>  
> -void ion_device_add_heap(struct ion_heap *heap)
> +int ion_device_add_heap(struct ion_heap *heap)
>  {
>  	struct dentry *debug_file;
>  	struct ion_device *dev = internal_dev;
> +	int ret = 0;
>  
>  	if (!heap->ops->allocate || !heap->ops->free)
>  		pr_err("%s: can not add heap with invalid ops struct.\n",
>  		       __func__);
>  
> +	if (heap_id >= ION_DEV_MAX)
> +		return -EBUSY;
> +
> +	heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
> +	dev_set_name(&heap->ddev, "ion%d", heap_id);
> +	device_initialize(&heap->ddev);
> +	cdev_init(&heap->chrdev, &ion_fops);
> +	heap->chrdev.owner = THIS_MODULE;
> +	ret = cdev_device_add(&heap->chrdev, &heap->ddev);
> +	if (ret < 0)
> +		return ret;
> +
>  	spin_lock_init(&heap->free_lock);
>  	heap->free_list_size = 0;
>  
> @@ -583,6 +598,8 @@ void ion_device_add_heap(struct ion_heap *heap)
>  
>  	dev->heap_cnt++;
>  	up_write(&dev->lock);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(ion_device_add_heap);
>  
> @@ -595,6 +612,7 @@ static int ion_device_create(void)
>  	if (!idev)
>  		return -ENOMEM;
>  
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>  	idev->dev.minor = MISC_DYNAMIC_MINOR;
>  	idev->dev.name = "ion";
>  	idev->dev.fops = &ion_fops;
> @@ -605,6 +623,17 @@ static int ion_device_create(void)
>  		kfree(idev);
>  		return ret;
>  	}
> +#endif
> +
> +	ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
> +	if (ret) {
> +		pr_err("ion: unable to allocate device\n");
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> +		misc_deregister(&idev->dev);
> +#endif
> +		kfree(idev);
> +		return ret;
> +	}
>  
>  	idev->debug_root = debugfs_create_dir("ion", NULL);
>  	if (!idev->debug_root) {
> diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
> index 621e5f7..2b00ccb 100644
> --- a/drivers/staging/android/ion/ion.h
> +++ b/drivers/staging/android/ion/ion.h
> @@ -17,16 +17,19 @@
>  #ifndef _ION_H
>  #define _ION_H
>  
> +#include <linux/cdev.h>
>  #include <linux/device.h>
>  #include <linux/dma-direction.h>
>  #include <linux/kref.h>
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> +#include <linux/miscdevice.h>
> +#endif
>  #include <linux/mm_types.h>
>  #include <linux/mutex.h>
>  #include <linux/rbtree.h>
>  #include <linux/sched.h>
>  #include <linux/shrinker.h>
>  #include <linux/types.h>
> -#include <linux/miscdevice.h>
>  
>  #include "../uapi/ion.h"
>  
> @@ -91,12 +94,16 @@ void ion_buffer_destroy(struct ion_buffer *buffer);
>  /**
>   * struct ion_device - the metadata of the ion device node
>   * @dev:		the actual misc device
> + * @devt:		Ion 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
>   */
>  struct ion_device {
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>  	struct miscdevice dev;
> +#endif
> +	dev_t devt;
>  	struct rb_root buffers;
>  	struct mutex buffer_lock;
>  	struct rw_semaphore lock;
> @@ -152,6 +159,8 @@ struct ion_heap_ops {
>   * struct ion_heap - represents a heap in the system
>   * @node:		rb node to put the heap on the device's tree of heaps
>   * @dev:		back pointer to the ion_device
> + * @ddev:		device structure
> + * @chrdev:		associated character device
>   * @type:		type of heap
>   * @ops:		ops struct as above
>   * @flags:		flags
> @@ -176,6 +185,8 @@ struct ion_heap_ops {
>  struct ion_heap {
>  	struct plist_node node;
>  	struct ion_device *dev;
> +	struct device ddev;
> +	struct cdev chrdev;
>  	enum ion_heap_type type;
>  	struct ion_heap_ops *ops;
>  	unsigned long flags;
> @@ -212,7 +223,7 @@ bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer);
>   * ion_device_add_heap - adds a heap to the ion device
>   * @heap:		the heap to add
>   */
> -void ion_device_add_heap(struct ion_heap *heap);
> +int ion_device_add_heap(struct ion_heap *heap);
>  
>  /**
>   * some helpers for common operations on buffers using the sg_table
> 

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 09/27/2017 06:20 AM, Benjamin Gaignard wrote:
> Instead a getting only one common device "/dev/ion" for
> all the heaps this patch allow to create one device
> entry ("/dev/ionX") per heap.
> Getting an entry per heap could allow to set security rules
> per heap and global ones for all heaps.
> 
> Allocation requests will be only allowed if the mask_id
> match with device minor.
> Query request could be done on any of the devices.
>
Thinking about this a bit more, I'm not 100% sure if this
will allow the security rules we want. Heap ids are assigned
dynamically and therefore so will the /dev/ionX designation.
>From my understanding, security rules like selinux need to
be fully specified at boot time so I'm not sure how you would
be able to write rules to differentiate between /dev/ionX and
/dev/ionY without knowing the values at boottime.

So I think we need a different way to match the heap ids to
get the security we want unless my understanding of security
policies is wrong and we can dynamically specify permissions.

Thanks,
Laura
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> version 5:
> - create a configuration flag to keep legacy Ion misc device
> 
> version 4:
> - add a configuration flag to switch between legacy Ion misc device
>   and one device per heap version.
> 
> version 3:
> - change ion_device_add_heap prototype to return a possible error.
> 
> version 2:
> - simplify ioctl check like propose by Dan
> - make sure that we don't register more than ION_DEV_MAX heaps.
> 
>  drivers/staging/android/TODO            |  1 -
>  drivers/staging/android/ion/Kconfig     |  7 +++++++
>  drivers/staging/android/ion/ion-ioctl.c | 18 ++++++++++++++++--
>  drivers/staging/android/ion/ion.c       | 31 ++++++++++++++++++++++++++++++-
>  drivers/staging/android/ion/ion.h       | 15 +++++++++++++--
>  5 files changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
> index 5f14247..d770ffa 100644
> --- a/drivers/staging/android/TODO
> +++ b/drivers/staging/android/TODO
> @@ -9,7 +9,6 @@ TODO:
>  ion/
>   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
>     involve putting appropriate bindings in a memory node for Ion to find.
> - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
>   - Better test framework (integration with VGEM was suggested)
>  
>  Please send patches to Greg Kroah-Hartman <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> and Cc:
> diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
> index a517b2d..cb4666e 100644
> --- a/drivers/staging/android/ion/Kconfig
> +++ b/drivers/staging/android/ion/Kconfig
> @@ -10,6 +10,13 @@ menuconfig ION
>  	  If you're not using Android its probably safe to
>  	  say N here.
>  
> +config ION_LEGACY_DEVICE_API
> +	bool "Keep using Ion legacy misc device API"
> +	depends on ION
> +	help
> +	  Choose this option to keep using Ion legacy misc device API
> +	  i.e. /dev/ion
> +
>  config ION_SYSTEM_HEAP
>  	bool "Ion system heap"
>  	depends on ION
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index e26b786..bb5c77b 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -25,7 +25,8 @@ union ion_ioctl_arg {
>  	struct ion_heap_query query;
>  };
>  
> -static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> +static int validate_ioctl_arg(struct file *filp,
> +			      unsigned int cmd, union ion_ioctl_arg *arg)
>  {
>  	switch (cmd) {
>  	case ION_IOC_HEAP_QUERY:
> @@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>  		    arg->query.reserved2 )
>  			return -EINVAL;
>  		break;
> +
> +	case ION_IOC_ALLOC:
> +	{
> +		int mask = 1 << iminor(filp->f_inode);
> +
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> +		if (imajor(filp->f_inode) == MISC_MAJOR)
> +			return 0;
> +#endif
> +		if (!(arg->allocation.heap_id_mask & mask))
> +			return -EINVAL;
> +		break;
> +	}
>  	default:
>  		break;
>  	}
> @@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
>  		return -EFAULT;
>  
> -	ret = validate_ioctl_arg(cmd, &data);
> +	ret = validate_ioctl_arg(filp, cmd, &data);
>  	if (WARN_ON_ONCE(ret))
>  		return ret;
>  
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 93e2c90..092b24c 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -40,6 +40,8 @@
>  
>  #include "ion.h"
>  
> +#define ION_DEV_MAX 32
> +
>  static struct ion_device *internal_dev;
>  static int heap_id;
>  
> @@ -537,15 +539,28 @@ static int debug_shrink_get(void *data, u64 *val)
>  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>  			debug_shrink_set, "%llu\n");
>  
> -void ion_device_add_heap(struct ion_heap *heap)
> +int ion_device_add_heap(struct ion_heap *heap)
>  {
>  	struct dentry *debug_file;
>  	struct ion_device *dev = internal_dev;
> +	int ret = 0;
>  
>  	if (!heap->ops->allocate || !heap->ops->free)
>  		pr_err("%s: can not add heap with invalid ops struct.\n",
>  		       __func__);
>  
> +	if (heap_id >= ION_DEV_MAX)
> +		return -EBUSY;
> +
> +	heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
> +	dev_set_name(&heap->ddev, "ion%d", heap_id);
> +	device_initialize(&heap->ddev);
> +	cdev_init(&heap->chrdev, &ion_fops);
> +	heap->chrdev.owner = THIS_MODULE;
> +	ret = cdev_device_add(&heap->chrdev, &heap->ddev);
> +	if (ret < 0)
> +		return ret;
> +
>  	spin_lock_init(&heap->free_lock);
>  	heap->free_list_size = 0;
>  
> @@ -583,6 +598,8 @@ void ion_device_add_heap(struct ion_heap *heap)
>  
>  	dev->heap_cnt++;
>  	up_write(&dev->lock);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(ion_device_add_heap);
>  
> @@ -595,6 +612,7 @@ static int ion_device_create(void)
>  	if (!idev)
>  		return -ENOMEM;
>  
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>  	idev->dev.minor = MISC_DYNAMIC_MINOR;
>  	idev->dev.name = "ion";
>  	idev->dev.fops = &ion_fops;
> @@ -605,6 +623,17 @@ static int ion_device_create(void)
>  		kfree(idev);
>  		return ret;
>  	}
> +#endif
> +
> +	ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
> +	if (ret) {
> +		pr_err("ion: unable to allocate device\n");
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> +		misc_deregister(&idev->dev);
> +#endif
> +		kfree(idev);
> +		return ret;
> +	}
>  
>  	idev->debug_root = debugfs_create_dir("ion", NULL);
>  	if (!idev->debug_root) {
> diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
> index 621e5f7..2b00ccb 100644
> --- a/drivers/staging/android/ion/ion.h
> +++ b/drivers/staging/android/ion/ion.h
> @@ -17,16 +17,19 @@
>  #ifndef _ION_H
>  #define _ION_H
>  
> +#include <linux/cdev.h>
>  #include <linux/device.h>
>  #include <linux/dma-direction.h>
>  #include <linux/kref.h>
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> +#include <linux/miscdevice.h>
> +#endif
>  #include <linux/mm_types.h>
>  #include <linux/mutex.h>
>  #include <linux/rbtree.h>
>  #include <linux/sched.h>
>  #include <linux/shrinker.h>
>  #include <linux/types.h>
> -#include <linux/miscdevice.h>
>  
>  #include "../uapi/ion.h"
>  
> @@ -91,12 +94,16 @@ void ion_buffer_destroy(struct ion_buffer *buffer);
>  /**
>   * struct ion_device - the metadata of the ion device node
>   * @dev:		the actual misc device
> + * @devt:		Ion 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
>   */
>  struct ion_device {
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>  	struct miscdevice dev;
> +#endif
> +	dev_t devt;
>  	struct rb_root buffers;
>  	struct mutex buffer_lock;
>  	struct rw_semaphore lock;
> @@ -152,6 +159,8 @@ struct ion_heap_ops {
>   * struct ion_heap - represents a heap in the system
>   * @node:		rb node to put the heap on the device's tree of heaps
>   * @dev:		back pointer to the ion_device
> + * @ddev:		device structure
> + * @chrdev:		associated character device
>   * @type:		type of heap
>   * @ops:		ops struct as above
>   * @flags:		flags
> @@ -176,6 +185,8 @@ struct ion_heap_ops {
>  struct ion_heap {
>  	struct plist_node node;
>  	struct ion_device *dev;
> +	struct device ddev;
> +	struct cdev chrdev;
>  	enum ion_heap_type type;
>  	struct ion_heap_ops *ops;
>  	unsigned long flags;
> @@ -212,7 +223,7 @@ bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer);
>   * ion_device_add_heap - adds a heap to the ion device
>   * @heap:		the heap to add
>   */
> -void ion_device_add_heap(struct ion_heap *heap);
> +int ion_device_add_heap(struct ion_heap *heap);
>  
>  /**
>   * some helpers for common operations on buffers using the sg_table
> 

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Mark Brown]

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

On Mon, Oct 02, 2017 at 11:07:48AM -0700, Laura Abbott wrote:

> Thinking about this a bit more, I'm not 100% sure if this
> will allow the security rules we want. Heap ids are assigned
> dynamically and therefore so will the /dev/ionX designation.
> From my understanding, security rules like selinux need to
> be fully specified at boot time so I'm not sure how you would
> be able to write rules to differentiate between /dev/ionX and
> /dev/ionY without knowing the values at boottime.

Isn't this something that should be managable via udev rules that ensure
stable names in the same way as for things like disks or ethernet
controllers (even if it just ends up doing something like /dev/ion-gpu
or whatever)?  If we're not giving it enough information to assign stable
names where needed we probably need to fix that anyway.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 831 bytes --]

On Mon, Oct 02, 2017 at 11:07:48AM -0700, Laura Abbott wrote:

> Thinking about this a bit more, I'm not 100% sure if this
> will allow the security rules we want. Heap ids are assigned
> dynamically and therefore so will the /dev/ionX designation.
> From my understanding, security rules like selinux need to
> be fully specified at boot time so I'm not sure how you would
> be able to write rules to differentiate between /dev/ionX and
> /dev/ionY without knowing the values at boottime.

Isn't this something that should be managable via udev rules that ensure
stable names in the same way as for things like disks or ethernet
controllers (even if it just ends up doing something like /dev/ion-gpu
or whatever)?  If we're not giving it enough information to assign stable
names where needed we probably need to fix that anyway.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/03/2017 09:48 AM, Mark Brown wrote:
> On Mon, Oct 02, 2017 at 11:07:48AM -0700, Laura Abbott wrote:
> 
>> Thinking about this a bit more, I'm not 100% sure if this
>> will allow the security rules we want. Heap ids are assigned
>> dynamically and therefore so will the /dev/ionX designation.
>> From my understanding, security rules like selinux need to
>> be fully specified at boot time so I'm not sure how you would
>> be able to write rules to differentiate between /dev/ionX and
>> /dev/ionY without knowing the values at boottime.
> 
> Isn't this something that should be managable via udev rules that ensure
> stable names in the same way as for things like disks or ethernet
> controllers (even if it just ends up doing something like /dev/ion-gpu
> or whatever)?  If we're not giving it enough information to assign stable
> names where needed we probably need to fix that anyway.
> 

Android doesn't use a standard udev so we'd need something that
would work there. My understanding was that android needs everything
specified at boot unless that's changed.

There would be enough information to assign stable names
(e.g. /dev/ion-system) if we used the query ioctl to find out
what's actually available. Is just the ioctl useful though?

Thanks,
Laura

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Sandeep Patil]

On Tue, Oct 03, 2017 at 02:42:32PM -0700, Laura Abbott wrote:
> On 10/03/2017 09:48 AM, Mark Brown wrote:
> > On Mon, Oct 02, 2017 at 11:07:48AM -0700, Laura Abbott wrote:
> > 
> >> Thinking about this a bit more, I'm not 100% sure if this
> >> will allow the security rules we want. Heap ids are assigned
> >> dynamically and therefore so will the /dev/ionX designation.
> >> From my understanding, security rules like selinux need to
> >> be fully specified at boot time so I'm not sure how you would
> >> be able to write rules to differentiate between /dev/ionX and
> >> /dev/ionY without knowing the values at boottime.
> > 
> > Isn't this something that should be managable via udev rules that ensure
> > stable names in the same way as for things like disks or ethernet
> > controllers (even if it just ends up doing something like /dev/ion-gpu
> > or whatever)?  If we're not giving it enough information to assign stable
> > names where needed we probably need to fix that anyway.
> > 
> 
> Android doesn't use a standard udev so we'd need something that
> would work there. My understanding was that android needs everything
> specified at boot unless that's changed.
> 
> There would be enough information to assign stable names
> (e.g. /dev/ion-system) if we used the query ioctl to find out
> what's actually available. Is just the ioctl useful though?

Wouldn't this new ioctl() to query the heap name also result in special case
handling of all ion devices in user space?

If the devices are named with their corresponding heap names like ion-system, ion-cma etc.
It is entirely possible and easy in android/ueventd to create those nodes
under "/dev/ion/".  (assuming the heap 'subsystem' for these new devices will
point to 'ion').

Something like the following should work if added in ueventd.rc

  subsystem ion
    devname uevent_devname
    dirname /dev/ion

Also, makes SElinux labelling easier.

(Also FWIW, the SELinux permissions are also possible with the current ion
implementation by adding rules to disallow specific ioctls instead of adding
permissions to access device node as this change would do)


- ssp

> 
> Thanks,
> Laura
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Sandeep Patil]

On Tue, Oct 03, 2017 at 02:42:32PM -0700, Laura Abbott wrote:
> On 10/03/2017 09:48 AM, Mark Brown wrote:
> > On Mon, Oct 02, 2017 at 11:07:48AM -0700, Laura Abbott wrote:
> > 
> >> Thinking about this a bit more, I'm not 100% sure if this
> >> will allow the security rules we want. Heap ids are assigned
> >> dynamically and therefore so will the /dev/ionX designation.
> >> From my understanding, security rules like selinux need to
> >> be fully specified at boot time so I'm not sure how you would
> >> be able to write rules to differentiate between /dev/ionX and
> >> /dev/ionY without knowing the values at boottime.
> > 
> > Isn't this something that should be managable via udev rules that ensure
> > stable names in the same way as for things like disks or ethernet
> > controllers (even if it just ends up doing something like /dev/ion-gpu
> > or whatever)?  If we're not giving it enough information to assign stable
> > names where needed we probably need to fix that anyway.
> > 
> 
> Android doesn't use a standard udev so we'd need something that
> would work there. My understanding was that android needs everything
> specified at boot unless that's changed.
> 
> There would be enough information to assign stable names
> (e.g. /dev/ion-system) if we used the query ioctl to find out
> what's actually available. Is just the ioctl useful though?

Wouldn't this new ioctl() to query the heap name also result in special case
handling of all ion devices in user space?

If the devices are named with their corresponding heap names like ion-system, ion-cma etc.
It is entirely possible and easy in android/ueventd to create those nodes
under "/dev/ion/".  (assuming the heap 'subsystem' for these new devices will
point to 'ion').

Something like the following should work if added in ueventd.rc

  subsystem ion
    devname uevent_devname
    dirname /dev/ion

Also, makes SElinux labelling easier.

(Also FWIW, the SELinux permissions are also possible with the current ion
implementation by adding rules to disallow specific ioctls instead of adding
permissions to access device node as this change would do)


- ssp

> 
> Thanks,
> Laura
> _______________________________________________
> devel mailing list
> devel-tBiZLqfeLfOHmIFyCCdPziST3g8Odh+X@public.gmane.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/03/2017 04:08 PM, Sandeep Patil wrote:
> On Tue, Oct 03, 2017 at 02:42:32PM -0700, Laura Abbott wrote:
>> On 10/03/2017 09:48 AM, Mark Brown wrote:
>>> On Mon, Oct 02, 2017 at 11:07:48AM -0700, Laura Abbott wrote:
>>>
>>>> Thinking about this a bit more, I'm not 100% sure if this
>>>> will allow the security rules we want. Heap ids are assigned
>>>> dynamically and therefore so will the /dev/ionX designation.
>>>> From my understanding, security rules like selinux need to
>>>> be fully specified at boot time so I'm not sure how you would
>>>> be able to write rules to differentiate between /dev/ionX and
>>>> /dev/ionY without knowing the values at boottime.
>>>
>>> Isn't this something that should be managable via udev rules that ensure
>>> stable names in the same way as for things like disks or ethernet
>>> controllers (even if it just ends up doing something like /dev/ion-gpu
>>> or whatever)?  If we're not giving it enough information to assign stable
>>> names where needed we probably need to fix that anyway.
>>>
>>
>> Android doesn't use a standard udev so we'd need something that
>> would work there. My understanding was that android needs everything
>> specified at boot unless that's changed.
>>
>> There would be enough information to assign stable names
>> (e.g. /dev/ion-system) if we used the query ioctl to find out
>> what's actually available. Is just the ioctl useful though?
> 
> Wouldn't this new ioctl() to query the heap name also result in special case
> handling of all ion devices in user space?
> 

I'm not quite sure what you are referring to. If you mean we have
to match on the heap name then yes that's going to happen but we
can't just zero knowledge which heap to allocate from and matching
on heap names seemed like the easiest way to make that happen.

> If the devices are named with their corresponding heap names like ion-system, ion-cma etc.
> It is entirely possible and easy in android/ueventd to create those nodes
> under "/dev/ion/".  (assuming the heap 'subsystem' for these new devices will
> point to 'ion').
> 
> Something like the following should work if added in ueventd.rc
> 
>   subsystem ion
>     devname uevent_devname
>     dirname /dev/ion
> 
> Also, makes SElinux labelling easier.
> 

That's useful to know, thanks.

> (Also FWIW, the SELinux permissions are also possible with the current ion
> implementation by adding rules to disallow specific ioctls instead of adding
> permissions to access device node as this change would do)
>

Can selinux disallow access within the ioctls though? The access control
wanted is at a heap granularity and disallowing certain ioctls won't fix
that.
 
> 
> - ssp
> 

Thanks,
Laura

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/03/2017 04:08 PM, Sandeep Patil wrote:
> On Tue, Oct 03, 2017 at 02:42:32PM -0700, Laura Abbott wrote:
>> On 10/03/2017 09:48 AM, Mark Brown wrote:
>>> On Mon, Oct 02, 2017 at 11:07:48AM -0700, Laura Abbott wrote:
>>>
>>>> Thinking about this a bit more, I'm not 100% sure if this
>>>> will allow the security rules we want. Heap ids are assigned
>>>> dynamically and therefore so will the /dev/ionX designation.
>>>> From my understanding, security rules like selinux need to
>>>> be fully specified at boot time so I'm not sure how you would
>>>> be able to write rules to differentiate between /dev/ionX and
>>>> /dev/ionY without knowing the values at boottime.
>>>
>>> Isn't this something that should be managable via udev rules that ensure
>>> stable names in the same way as for things like disks or ethernet
>>> controllers (even if it just ends up doing something like /dev/ion-gpu
>>> or whatever)?  If we're not giving it enough information to assign stable
>>> names where needed we probably need to fix that anyway.
>>>
>>
>> Android doesn't use a standard udev so we'd need something that
>> would work there. My understanding was that android needs everything
>> specified at boot unless that's changed.
>>
>> There would be enough information to assign stable names
>> (e.g. /dev/ion-system) if we used the query ioctl to find out
>> what's actually available. Is just the ioctl useful though?
> 
> Wouldn't this new ioctl() to query the heap name also result in special case
> handling of all ion devices in user space?
> 

I'm not quite sure what you are referring to. If you mean we have
to match on the heap name then yes that's going to happen but we
can't just zero knowledge which heap to allocate from and matching
on heap names seemed like the easiest way to make that happen.

> If the devices are named with their corresponding heap names like ion-system, ion-cma etc.
> It is entirely possible and easy in android/ueventd to create those nodes
> under "/dev/ion/".  (assuming the heap 'subsystem' for these new devices will
> point to 'ion').
> 
> Something like the following should work if added in ueventd.rc
> 
>   subsystem ion
>     devname uevent_devname
>     dirname /dev/ion
> 
> Also, makes SElinux labelling easier.
> 

That's useful to know, thanks.

> (Also FWIW, the SELinux permissions are also possible with the current ion
> implementation by adding rules to disallow specific ioctls instead of adding
> permissions to access device node as this change would do)
>

Can selinux disallow access within the ioctls though? The access control
wanted is at a heap granularity and disallowing certain ioctls won't fix
that.
 
> 
> - ssp
> 

Thanks,
Laura

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Mark Brown]

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

On Tue, Oct 03, 2017 at 04:08:30PM -0700, Sandeep Patil wrote:

> It is entirely possible and easy in android/ueventd to create those nodes
> under "/dev/ion/".  (assuming the heap 'subsystem' for these new devices will
> point to 'ion').

The reason I didn't say /dev/ion/foo initially is that if people want to
keep the existing /dev/ion around for compatibility reasons then the
/dev/ion name isn't available which might cause issues.  Otherwise just
dumping everything under a directory (perhaps with a different name) was
my first thought as well.

> (Also FWIW, the SELinux permissions are also possible with the current ion
> implementation by adding rules to disallow specific ioctls instead of adding
> permissions to access device node as this change would do)

AIUI the request is to limit access to specific heaps, and obviously not
everyone wants to deal with SELinux at all.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Mark Brown]

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

On Tue, Oct 03, 2017 at 04:08:30PM -0700, Sandeep Patil wrote:

> It is entirely possible and easy in android/ueventd to create those nodes
> under "/dev/ion/".  (assuming the heap 'subsystem' for these new devices will
> point to 'ion').

The reason I didn't say /dev/ion/foo initially is that if people want to
keep the existing /dev/ion around for compatibility reasons then the
/dev/ion name isn't available which might cause issues.  Otherwise just
dumping everything under a directory (perhaps with a different name) was
my first thought as well.

> (Also FWIW, the SELinux permissions are also possible with the current ion
> implementation by adding rules to disallow specific ioctls instead of adding
> permissions to access device node as this change would do)

AIUI the request is to limit access to specific heaps, and obviously not
everyone wants to deal with SELinux at all.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-10-04 12:17 GMT+02:00 Mark Brown <broonie@kernel.org>:
> On Tue, Oct 03, 2017 at 04:08:30PM -0700, Sandeep Patil wrote:
>
>> It is entirely possible and easy in android/ueventd to create those nodes
>> under "/dev/ion/".  (assuming the heap 'subsystem' for these new devices will
>> point to 'ion').

I think it is the same problem than for webcam under v4l framework.
Each time you plug a webcam you got a v4l node but android/uevent rules
the plug order doesn't have impact.
The same think will happen for ion nodes it may be even easier because
the heap will always being created in the smae order for a given product
configuration.

>
> The reason I didn't say /dev/ion/foo initially is that if people want to
> keep the existing /dev/ion around for compatibility reasons then the
> /dev/ion name isn't available which might cause issues.  Otherwise just
> dumping everything under a directory (perhaps with a different name) was
> my first thought as well.
>
>> (Also FWIW, the SELinux permissions are also possible with the current ion
>> implementation by adding rules to disallow specific ioctls instead of adding
>> permissions to access device node as this change would do)
>
> AIUI the request is to limit access to specific heaps, and obviously not
> everyone wants to deal with SELinux at all.

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-10-04 12:17 GMT+02:00 Mark Brown <broonie@kernel.org>:
> On Tue, Oct 03, 2017 at 04:08:30PM -0700, Sandeep Patil wrote:
>
>> It is entirely possible and easy in android/ueventd to create those nodes
>> under "/dev/ion/".  (assuming the heap 'subsystem' for these new devices will
>> point to 'ion').

I think it is the same problem than for webcam under v4l framework.
Each time you plug a webcam you got a v4l node but android/uevent rules
the plug order doesn't have impact.
The same think will happen for ion nodes it may be even easier because
the heap will always being created in the smae order for a given product
configuration.

>
> The reason I didn't say /dev/ion/foo initially is that if people want to
> keep the existing /dev/ion around for compatibility reasons then the
> /dev/ion name isn't available which might cause issues.  Otherwise just
> dumping everything under a directory (perhaps with a different name) was
> my first thought as well.
>
>> (Also FWIW, the SELinux permissions are also possible with the current ion
>> implementation by adding rules to disallow specific ioctls instead of adding
>> permissions to access device node as this change would do)
>
> AIUI the request is to limit access to specific heaps, and obviously not
> everyone wants to deal with SELinux at all.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 1/2] staging: ion: simplify ioctl args checking function

[Benjamin Gaignard]

2017-09-27 15:20 GMT+02:00 Benjamin Gaignard <benjamin.gaignard@linaro.org>:
> Make arguments checking more easy to read.
>

Hi Laura,

Even if we don't have found a solution for the second patch I believe
this one could be useful.
May I ask you your point of view on those few lines ?

Benjamin

> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> ---
>  drivers/staging/android/ion/ion-ioctl.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index d9f8b14..e26b786 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -27,19 +27,18 @@ union ion_ioctl_arg {
>
>  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;
> +               if (arg->query.reserved0 ||
> +                   arg->query.reserved1 ||
> +                   arg->query.reserved2)
> +                       return -EINVAL;
>                 break;
>         default:
>                 break;
>         }
>
> -       return ret ? -EINVAL : 0;
> +       return 0;
>  }
>
>  /* fix up the cases where the ioctl direction bits are incorrect */
> --
> 2.7.4
>



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5 1/2] staging: ion: simplify ioctl args checking function

[Benjamin Gaignard]

2017-09-27 15:20 GMT+02:00 Benjamin Gaignard <benjamin.gaignard@linaro.org>:
> Make arguments checking more easy to read.
>

Hi Laura,

Even if we don't have found a solution for the second patch I believe
this one could be useful.
May I ask you your point of view on those few lines ?

Benjamin

> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> ---
>  drivers/staging/android/ion/ion-ioctl.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index d9f8b14..e26b786 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -27,19 +27,18 @@ union ion_ioctl_arg {
>
>  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;
> +               if (arg->query.reserved0 ||
> +                   arg->query.reserved1 ||
> +                   arg->query.reserved2)
> +                       return -EINVAL;
>                 break;
>         default:
>                 break;
>         }
>
> -       return ret ? -EINVAL : 0;
> +       return 0;
>  }
>
>  /* fix up the cases where the ioctl direction bits are incorrect */
> --
> 2.7.4
>



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 1/2] staging: ion: simplify ioctl args checking function

[Laura Abbott]

On 10/09/2017 02:21 AM, Benjamin Gaignard wrote:
> 2017-09-27 15:20 GMT+02:00 Benjamin Gaignard <benjamin.gaignard@linaro.org>:
>> Make arguments checking more easy to read.
>>
> 
> Hi Laura,
> 
> Even if we don't have found a solution for the second patch I believe
> this one could be useful.
> May I ask you your point of view on those few lines ?
> 
> Benjamin
> 

Yes, this looks better.

Acked-by: Laura Abbott <labbott@redhat.com>

>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> ---
>>  drivers/staging/android/ion/ion-ioctl.c | 11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
>> index d9f8b14..e26b786 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -27,19 +27,18 @@ union ion_ioctl_arg {
>>
>>  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;
>> +               if (arg->query.reserved0 ||
>> +                   arg->query.reserved1 ||
>> +                   arg->query.reserved2)
>> +                       return -EINVAL;
>>                 break;
>>         default:
>>                 break;
>>         }
>>
>> -       return ret ? -EINVAL : 0;
>> +       return 0;
>>  }
>>
>>  /* fix up the cases where the ioctl direction bits are incorrect */
>> --
>> 2.7.4
>>
> 
> 
> 

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/05/2017 06:06 AM, Benjamin Gaignard wrote:
> 2017-10-04 12:17 GMT+02:00 Mark Brown <broonie@kernel.org>:
>> On Tue, Oct 03, 2017 at 04:08:30PM -0700, Sandeep Patil wrote:
>>
>>> It is entirely possible and easy in android/ueventd to create those nodes
>>> under "/dev/ion/".  (assuming the heap 'subsystem' for these new devices will
>>> point to 'ion').
> 
> I think it is the same problem than for webcam under v4l framework.
> Each time you plug a webcam you got a v4l node but android/uevent rules
> the plug order doesn't have impact.
> The same think will happen for ion nodes it may be even easier because
> the heap will always being created in the smae order for a given product
> configuration.
> 

Relying on the heap being created in the same order seems troublesome.
If for some reason it changes in the kernel we might break something
in userspace.

Anyway, to move this forward I think we need to see a proof of concept
of using selinux to protect access to specific heaps.

Thanks,
Laura

>>
>> The reason I didn't say /dev/ion/foo initially is that if people want to
>> keep the existing /dev/ion around for compatibility reasons then the
>> /dev/ion name isn't available which might cause issues.  Otherwise just
>> dumping everything under a directory (perhaps with a different name) was
>> my first thought as well.
>>
>>> (Also FWIW, the SELinux permissions are also possible with the current ion
>>> implementation by adding rules to disallow specific ioctls instead of adding
>>> permissions to access device node as this change would do)
>>
>> AIUI the request is to limit access to specific heaps, and obviously not
>> everyone wants to deal with SELinux at all.

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Mark Brown]

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

On Mon, Oct 09, 2017 at 02:25:47PM -0700, Laura Abbott wrote:

> Anyway, to move this forward I think we need to see a proof of concept
> of using selinux to protect access to specific heaps.

Aren't Unix permissions enough with separate files or am I
misunderstanding what you're looking to see a proof of concept for?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/09/2017 03:08 PM, Mark Brown wrote:
> On Mon, Oct 09, 2017 at 02:25:47PM -0700, Laura Abbott wrote:
> 
>> Anyway, to move this forward I think we need to see a proof of concept
>> of using selinux to protect access to specific heaps.
> 
> Aren't Unix permissions enough with separate files or am I
> misunderstanding what you're looking to see a proof of concept for?
> 

The goal is to be able to restrict heap access to certain services
and selinux groups on Android so straight unix permissions aren't
sufficient.

Thanks,
Laura

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/09/2017 03:08 PM, Mark Brown wrote:
> On Mon, Oct 09, 2017 at 02:25:47PM -0700, Laura Abbott wrote:
> 
>> Anyway, to move this forward I think we need to see a proof of concept
>> of using selinux to protect access to specific heaps.
> 
> Aren't Unix permissions enough with separate files or am I
> misunderstanding what you're looking to see a proof of concept for?
> 

The goal is to be able to restrict heap access to certain services
and selinux groups on Android so straight unix permissions aren't
sufficient.

Thanks,
Laura
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Mark Brown]

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

On Mon, Oct 09, 2017 at 05:10:37PM -0700, Laura Abbott wrote:
> On 10/09/2017 03:08 PM, Mark Brown wrote:
> > On Mon, Oct 09, 2017 at 02:25:47PM -0700, Laura Abbott wrote:

> >> Anyway, to move this forward I think we need to see a proof of concept
> >> of using selinux to protect access to specific heaps.

> > Aren't Unix permissions enough with separate files or am I
> > misunderstanding what you're looking to see a proof of concept for?

> The goal is to be able to restrict heap access to certain services
> and selinux groups on Android so straight unix permissions aren't
> sufficient.

Oh, there's Android users for this?  The users I was aware of were
non-Android.  Though even so I'd have thought that given that SELinux is
a superset of Unix file permissions it ought to be sufficient to be able
to use them.  I'd been thinking people were suggesting SELinux as a
replacement for file permissions, using the single file and the greater
capabilities of SELinux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/10/2017 02:11 AM, Mark Brown wrote:
> On Mon, Oct 09, 2017 at 05:10:37PM -0700, Laura Abbott wrote:
>> On 10/09/2017 03:08 PM, Mark Brown wrote:
>>> On Mon, Oct 09, 2017 at 02:25:47PM -0700, Laura Abbott wrote:
> 
>>>> Anyway, to move this forward I think we need to see a proof of concept
>>>> of using selinux to protect access to specific heaps.
> 
>>> Aren't Unix permissions enough with separate files or am I
>>> misunderstanding what you're looking to see a proof of concept for?
> 
>> The goal is to be able to restrict heap access to certain services
>> and selinux groups on Android so straight unix permissions aren't
>> sufficient.
> 
> Oh, there's Android users for this?  The users I was aware of were
> non-Android.  Though even so I'd have thought that given that SELinux is
> a superset of Unix file permissions it ought to be sufficient to be able
> to use them.  I'd been thinking people were suggesting SELinux as a
> replacement for file permissions, using the single file and the greater
> capabilities of SELinux.
> 
Unix file permissions are necessary but not sufficient, they
can be used separately. Mostly what I want to see before
merging this is an example that splitting the Ion heaps provides
more protection than just keeping /dev/ion.

Thanks,
Laura

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/10/2017 02:11 AM, Mark Brown wrote:
> On Mon, Oct 09, 2017 at 05:10:37PM -0700, Laura Abbott wrote:
>> On 10/09/2017 03:08 PM, Mark Brown wrote:
>>> On Mon, Oct 09, 2017 at 02:25:47PM -0700, Laura Abbott wrote:
> 
>>>> Anyway, to move this forward I think we need to see a proof of concept
>>>> of using selinux to protect access to specific heaps.
> 
>>> Aren't Unix permissions enough with separate files or am I
>>> misunderstanding what you're looking to see a proof of concept for?
> 
>> The goal is to be able to restrict heap access to certain services
>> and selinux groups on Android so straight unix permissions aren't
>> sufficient.
> 
> Oh, there's Android users for this?  The users I was aware of were
> non-Android.  Though even so I'd have thought that given that SELinux is
> a superset of Unix file permissions it ought to be sufficient to be able
> to use them.  I'd been thinking people were suggesting SELinux as a
> replacement for file permissions, using the single file and the greater
> capabilities of SELinux.
> 
Unix file permissions are necessary but not sufficient, they
can be used separately. Mostly what I want to see before
merging this is an example that splitting the Ion heaps provides
more protection than just keeping /dev/ion.

Thanks,
Laura
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-10-17 0:09 GMT+02:00 Laura Abbott <labbott@redhat.com>:
> On 10/10/2017 02:11 AM, Mark Brown wrote:
>> On Mon, Oct 09, 2017 at 05:10:37PM -0700, Laura Abbott wrote:
>>> On 10/09/2017 03:08 PM, Mark Brown wrote:
>>>> On Mon, Oct 09, 2017 at 02:25:47PM -0700, Laura Abbott wrote:
>>
>>>>> Anyway, to move this forward I think we need to see a proof of concept
>>>>> of using selinux to protect access to specific heaps.
>>
>>>> Aren't Unix permissions enough with separate files or am I
>>>> misunderstanding what you're looking to see a proof of concept for?
>>
>>> The goal is to be able to restrict heap access to certain services
>>> and selinux groups on Android so straight unix permissions aren't
>>> sufficient.
>>
>> Oh, there's Android users for this?  The users I was aware of were
>> non-Android.  Though even so I'd have thought that given that SELinux is
>> a superset of Unix file permissions it ought to be sufficient to be able
>> to use them.  I'd been thinking people were suggesting SELinux as a
>> replacement for file permissions, using the single file and the greater
>> capabilities of SELinux.
>>
> Unix file permissions are necessary but not sufficient, they
> can be used separately. Mostly what I want to see before
> merging this is an example that splitting the Ion heaps provides
> more protection than just keeping /dev/ion.
>

To give you an example on my system I have cma regions and so
2 heaps. One is for video decoding/encoding usage and one is dedicated
to display. The goal is to be sure to have enough memory for each devices

With only one /dev/ion nothing (except heap id mask) prohibed one video
apllication to use the cma region dedicated to display.
With one device per heaps I could change the permissions to be sure that
only display have access to the correct heap.
In android init.rc file I will have to change
  chmod 0666 /dev/ion
  chown system graphics /dev/ion

to something like
  chmod 0666 /dev/ion1
  chown system graphics /dev/ion1
  chmod 0666 /dev/ion2
  chown system media /dev/ion2

Android SEpolicy is defined like that
   allow { appdomain -isolated_app } ion_device:chr_file rw_file_perms;
which means that apps could have access to /dev/ion
with multiple devices we can imagine to protect some heap of being used by
the apps, for example like this
   allow { appdomain -isolated_app } ion_device0:chr_file {open ioctl};
   allow { system } ion_device1:chr_file {open ioctl};
  allow { media } ion_device2:chr_file {open ioctl};

Benjamin

> Thanks,
> Laura

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-10-17 0:09 GMT+02:00 Laura Abbott <labbott-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On 10/10/2017 02:11 AM, Mark Brown wrote:
>> On Mon, Oct 09, 2017 at 05:10:37PM -0700, Laura Abbott wrote:
>>> On 10/09/2017 03:08 PM, Mark Brown wrote:
>>>> On Mon, Oct 09, 2017 at 02:25:47PM -0700, Laura Abbott wrote:
>>
>>>>> Anyway, to move this forward I think we need to see a proof of concept
>>>>> of using selinux to protect access to specific heaps.
>>
>>>> Aren't Unix permissions enough with separate files or am I
>>>> misunderstanding what you're looking to see a proof of concept for?
>>
>>> The goal is to be able to restrict heap access to certain services
>>> and selinux groups on Android so straight unix permissions aren't
>>> sufficient.
>>
>> Oh, there's Android users for this?  The users I was aware of were
>> non-Android.  Though even so I'd have thought that given that SELinux is
>> a superset of Unix file permissions it ought to be sufficient to be able
>> to use them.  I'd been thinking people were suggesting SELinux as a
>> replacement for file permissions, using the single file and the greater
>> capabilities of SELinux.
>>
> Unix file permissions are necessary but not sufficient, they
> can be used separately. Mostly what I want to see before
> merging this is an example that splitting the Ion heaps provides
> more protection than just keeping /dev/ion.
>

To give you an example on my system I have cma regions and so
2 heaps. One is for video decoding/encoding usage and one is dedicated
to display. The goal is to be sure to have enough memory for each devices

With only one /dev/ion nothing (except heap id mask) prohibed one video
apllication to use the cma region dedicated to display.
With one device per heaps I could change the permissions to be sure that
only display have access to the correct heap.
In android init.rc file I will have to change
  chmod 0666 /dev/ion
  chown system graphics /dev/ion

to something like
  chmod 0666 /dev/ion1
  chown system graphics /dev/ion1
  chmod 0666 /dev/ion2
  chown system media /dev/ion2

Android SEpolicy is defined like that
   allow { appdomain -isolated_app } ion_device:chr_file rw_file_perms;
which means that apps could have access to /dev/ion
with multiple devices we can imagine to protect some heap of being used by
the apps, for example like this
   allow { appdomain -isolated_app } ion_device0:chr_file {open ioctl};
   allow { system } ion_device1:chr_file {open ioctl};
  allow { media } ion_device2:chr_file {open ioctl};

Benjamin

> Thanks,
> Laura

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 09/27/2017 06:20 AM, Benjamin Gaignard wrote:
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 93e2c90..092b24c 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -40,6 +40,8 @@
>  
>  #include "ion.h"
>  
> +#define ION_DEV_MAX 32
> +
>  static struct ion_device *internal_dev;
>  static int heap_id;
>  
> @@ -537,15 +539,28 @@ static int debug_shrink_get(void *data, u64 *val)
>  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>  			debug_shrink_set, "%llu\n");
>  
> -void ion_device_add_heap(struct ion_heap *heap)
> +int ion_device_add_heap(struct ion_heap *heap)
>  {
>  	struct dentry *debug_file;
>  	struct ion_device *dev = internal_dev;
> +	int ret = 0;
>  
>  	if (!heap->ops->allocate || !heap->ops->free)
>  		pr_err("%s: can not add heap with invalid ops struct.\n",
>  		       __func__);
>  
> +	if (heap_id >= ION_DEV_MAX)
> +		return -EBUSY;
> +
> +	heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
> +	dev_set_name(&heap->ddev, "ion%d", heap_id);
> +	device_initialize(&heap->ddev);
> +	cdev_init(&heap->chrdev, &ion_fops);
> +	heap->chrdev.owner = THIS_MODULE;
> +	ret = cdev_device_add(&heap->chrdev, &heap->ddev);
> +	if (ret < 0)
> +		return ret;
> +
>  	spin_lock_init(&heap->free_lock);
>  	heap->free_list_size = 0;
>  
> @@ -583,6 +598,8 @@ void ion_device_add_heap(struct ion_heap *heap)
>  
>  	dev->heap_cnt++;
>  	up_write(&dev->lock);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(ion_device_add_heap);
>  
> @@ -595,6 +612,7 @@ static int ion_device_create(void)
>  	if (!idev)
>  		return -ENOMEM;
>  
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>  	idev->dev.minor = MISC_DYNAMIC_MINOR;
>  	idev->dev.name = "ion";
>  	idev->dev.fops = &ion_fops;
> @@ -605,6 +623,17 @@ static int ion_device_create(void)
>  		kfree(idev);
>  		return ret;
>  	}
> +#endif
> +
> +	ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
> +	if (ret) {
> +		pr_err("ion: unable to allocate device\n");
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> +		misc_deregister(&idev->dev);
> +#endif
> +		kfree(idev);
> +		return ret;
> +	}
>  
>  	idev->debug_root = debugfs_create_dir("ion", NULL);
>  	if (!idev->debug_root) {

I'm not 100% sure about the device hierarchy here. We're
ending up with devices at the root of /sys/devices

/sys/devices # ls
breakpoint ion0 ion1 ion2 platform software system virtual 

and the Android init system doesn't pick this up. I'll
admit to being out of my area here but I don't think
this looks quite right.

Thanks,
Laura

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 09/27/2017 06:20 AM, Benjamin Gaignard wrote:
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 93e2c90..092b24c 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -40,6 +40,8 @@
>  
>  #include "ion.h"
>  
> +#define ION_DEV_MAX 32
> +
>  static struct ion_device *internal_dev;
>  static int heap_id;
>  
> @@ -537,15 +539,28 @@ static int debug_shrink_get(void *data, u64 *val)
>  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>  			debug_shrink_set, "%llu\n");
>  
> -void ion_device_add_heap(struct ion_heap *heap)
> +int ion_device_add_heap(struct ion_heap *heap)
>  {
>  	struct dentry *debug_file;
>  	struct ion_device *dev = internal_dev;
> +	int ret = 0;
>  
>  	if (!heap->ops->allocate || !heap->ops->free)
>  		pr_err("%s: can not add heap with invalid ops struct.\n",
>  		       __func__);
>  
> +	if (heap_id >= ION_DEV_MAX)
> +		return -EBUSY;
> +
> +	heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
> +	dev_set_name(&heap->ddev, "ion%d", heap_id);
> +	device_initialize(&heap->ddev);
> +	cdev_init(&heap->chrdev, &ion_fops);
> +	heap->chrdev.owner = THIS_MODULE;
> +	ret = cdev_device_add(&heap->chrdev, &heap->ddev);
> +	if (ret < 0)
> +		return ret;
> +
>  	spin_lock_init(&heap->free_lock);
>  	heap->free_list_size = 0;
>  
> @@ -583,6 +598,8 @@ void ion_device_add_heap(struct ion_heap *heap)
>  
>  	dev->heap_cnt++;
>  	up_write(&dev->lock);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(ion_device_add_heap);
>  
> @@ -595,6 +612,7 @@ static int ion_device_create(void)
>  	if (!idev)
>  		return -ENOMEM;
>  
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>  	idev->dev.minor = MISC_DYNAMIC_MINOR;
>  	idev->dev.name = "ion";
>  	idev->dev.fops = &ion_fops;
> @@ -605,6 +623,17 @@ static int ion_device_create(void)
>  		kfree(idev);
>  		return ret;
>  	}
> +#endif
> +
> +	ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
> +	if (ret) {
> +		pr_err("ion: unable to allocate device\n");
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> +		misc_deregister(&idev->dev);
> +#endif
> +		kfree(idev);
> +		return ret;
> +	}
>  
>  	idev->debug_root = debugfs_create_dir("ion", NULL);
>  	if (!idev->debug_root) {

I'm not 100% sure about the device hierarchy here. We're
ending up with devices at the root of /sys/devices

/sys/devices # ls
breakpoint ion0 ion1 ion2 platform software system virtual 

and the Android init system doesn't pick this up. I'll
admit to being out of my area here but I don't think
this looks quite right.

Thanks,
Laura



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-10-18 22:07 GMT+02:00 Laura Abbott <labbott@redhat.com>:
> On 09/27/2017 06:20 AM, Benjamin Gaignard wrote:
>> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>> index 93e2c90..092b24c 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -40,6 +40,8 @@
>>
>>  #include "ion.h"
>>
>> +#define ION_DEV_MAX 32
>> +
>>  static struct ion_device *internal_dev;
>>  static int heap_id;
>>
>> @@ -537,15 +539,28 @@ static int debug_shrink_get(void *data, u64 *val)
>>  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>>                       debug_shrink_set, "%llu\n");
>>
>> -void ion_device_add_heap(struct ion_heap *heap)
>> +int ion_device_add_heap(struct ion_heap *heap)
>>  {
>>       struct dentry *debug_file;
>>       struct ion_device *dev = internal_dev;
>> +     int ret = 0;
>>
>>       if (!heap->ops->allocate || !heap->ops->free)
>>               pr_err("%s: can not add heap with invalid ops struct.\n",
>>                      __func__);
>>
>> +     if (heap_id >= ION_DEV_MAX)
>> +             return -EBUSY;
>> +
>> +     heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
>> +     dev_set_name(&heap->ddev, "ion%d", heap_id);
>> +     device_initialize(&heap->ddev);
>> +     cdev_init(&heap->chrdev, &ion_fops);
>> +     heap->chrdev.owner = THIS_MODULE;
>> +     ret = cdev_device_add(&heap->chrdev, &heap->ddev);
>> +     if (ret < 0)
>> +             return ret;
>> +
>>       spin_lock_init(&heap->free_lock);
>>       heap->free_list_size = 0;
>>
>> @@ -583,6 +598,8 @@ void ion_device_add_heap(struct ion_heap *heap)
>>
>>       dev->heap_cnt++;
>>       up_write(&dev->lock);
>> +
>> +     return ret;
>>  }
>>  EXPORT_SYMBOL(ion_device_add_heap);
>>
>> @@ -595,6 +612,7 @@ static int ion_device_create(void)
>>       if (!idev)
>>               return -ENOMEM;
>>
>> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>>       idev->dev.minor = MISC_DYNAMIC_MINOR;
>>       idev->dev.name = "ion";
>>       idev->dev.fops = &ion_fops;
>> @@ -605,6 +623,17 @@ static int ion_device_create(void)
>>               kfree(idev);
>>               return ret;
>>       }
>> +#endif
>> +
>> +     ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
>> +     if (ret) {
>> +             pr_err("ion: unable to allocate device\n");
>> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>> +             misc_deregister(&idev->dev);
>> +#endif
>> +             kfree(idev);
>> +             return ret;
>> +     }
>>
>>       idev->debug_root = debugfs_create_dir("ion", NULL);
>>       if (!idev->debug_root) {
>
> I'm not 100% sure about the device hierarchy here. We're
> ending up with devices at the root of /sys/devices
>
> /sys/devices # ls
> breakpoint ion0 ion1 ion2 platform software system virtual
>
> and the Android init system doesn't pick this up. I'll
> admit to being out of my area here but I don't think
> this looks quite right.
>

You are right it is because ion devices are parentless so they
directly put under /sys/devices directory. I will give them platform_bus
as parent to solve that problem.

Benjamin

> Thanks,
> Laura
>
>

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-10-18 22:07 GMT+02:00 Laura Abbott <labbott-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On 09/27/2017 06:20 AM, Benjamin Gaignard wrote:
>> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>> index 93e2c90..092b24c 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -40,6 +40,8 @@
>>
>>  #include "ion.h"
>>
>> +#define ION_DEV_MAX 32
>> +
>>  static struct ion_device *internal_dev;
>>  static int heap_id;
>>
>> @@ -537,15 +539,28 @@ static int debug_shrink_get(void *data, u64 *val)
>>  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>>                       debug_shrink_set, "%llu\n");
>>
>> -void ion_device_add_heap(struct ion_heap *heap)
>> +int ion_device_add_heap(struct ion_heap *heap)
>>  {
>>       struct dentry *debug_file;
>>       struct ion_device *dev = internal_dev;
>> +     int ret = 0;
>>
>>       if (!heap->ops->allocate || !heap->ops->free)
>>               pr_err("%s: can not add heap with invalid ops struct.\n",
>>                      __func__);
>>
>> +     if (heap_id >= ION_DEV_MAX)
>> +             return -EBUSY;
>> +
>> +     heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
>> +     dev_set_name(&heap->ddev, "ion%d", heap_id);
>> +     device_initialize(&heap->ddev);
>> +     cdev_init(&heap->chrdev, &ion_fops);
>> +     heap->chrdev.owner = THIS_MODULE;
>> +     ret = cdev_device_add(&heap->chrdev, &heap->ddev);
>> +     if (ret < 0)
>> +             return ret;
>> +
>>       spin_lock_init(&heap->free_lock);
>>       heap->free_list_size = 0;
>>
>> @@ -583,6 +598,8 @@ void ion_device_add_heap(struct ion_heap *heap)
>>
>>       dev->heap_cnt++;
>>       up_write(&dev->lock);
>> +
>> +     return ret;
>>  }
>>  EXPORT_SYMBOL(ion_device_add_heap);
>>
>> @@ -595,6 +612,7 @@ static int ion_device_create(void)
>>       if (!idev)
>>               return -ENOMEM;
>>
>> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>>       idev->dev.minor = MISC_DYNAMIC_MINOR;
>>       idev->dev.name = "ion";
>>       idev->dev.fops = &ion_fops;
>> @@ -605,6 +623,17 @@ static int ion_device_create(void)
>>               kfree(idev);
>>               return ret;
>>       }
>> +#endif
>> +
>> +     ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
>> +     if (ret) {
>> +             pr_err("ion: unable to allocate device\n");
>> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>> +             misc_deregister(&idev->dev);
>> +#endif
>> +             kfree(idev);
>> +             return ret;
>> +     }
>>
>>       idev->debug_root = debugfs_create_dir("ion", NULL);
>>       if (!idev->debug_root) {
>
> I'm not 100% sure about the device hierarchy here. We're
> ending up with devices at the root of /sys/devices
>
> /sys/devices # ls
> breakpoint ion0 ion1 ion2 platform software system virtual
>
> and the Android init system doesn't pick this up. I'll
> admit to being out of my area here but I don't think
> this looks quite right.
>

You are right it is because ion devices are parentless so they
directly put under /sys/devices directory. I will give them platform_bus
as parent to solve that problem.

Benjamin

> Thanks,
> Laura
>
>