diff mbox

[7/7] drm/mode: Add user blob-creation ioctl

Message ID 1429554176-9865-8-git-send-email-daniels@collabora.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Stone April 20, 2015, 6:22 p.m. UTC
Add an ioctl which allows users to create blob properties from supplied
data. Currently this only supports modes, creating a drm_display_mode from
the userspace drm_mode_modeinfo.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/drm_crtc.c  | 160 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_fops.c  |   5 +-
 drivers/gpu/drm/drm_ioctl.c |   2 +
 include/drm/drmP.h          |   4 ++
 include/drm/drm_crtc.h      |   9 ++-
 include/uapi/drm/drm.h      |   2 +
 include/uapi/drm/drm_mode.h |  22 ++++++
 7 files changed, 199 insertions(+), 5 deletions(-)

Comments

Maarten Lankhorst May 7, 2015, 7:53 a.m. UTC | #1
Op 20-04-15 om 20:22 schreef Daniel Stone:
> Add an ioctl which allows users to create blob properties from supplied
> data. Currently this only supports modes, creating a drm_display_mode from
> the userspace drm_mode_modeinfo.
>
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> ---
> <snip>
> +int drm_mode_createblob_ioctl(struct drm_device *dev,
> +			      void *data, struct drm_file *file_priv)
> +{
> +	struct drm_mode_create_blob *out_resp = data;
> +	struct drm_property_blob *blob;
> +	void *blob_data;
> +	int ret = 0;
> +	void __user *blob_ptr;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> +		return -EINVAL;
> +
> +	switch (out_resp->type) {
> +	case DRM_MODE_BLOB_TYPE_MODE: {
> +		if (out_resp->length != sizeof(struct drm_mode_modeinfo)) {
> +			ret = -E2BIG;to length
You want -EINVAL here, -E2BIG means "argument list too long".
> +			goto out_unlocked;
> +		}
> +		break;
> +	}
> +	default:
> +		ret = -EINVAL;
> +		goto out_unlocked;
> +	}
> +
> +	blob_data = kzalloc(out_resp->length, GFP_USER);
> +	if (!blob_data) {
> +		ret = -ENOMEM;
> +		goto out_unlocked;
> +	}
> +
> +	blob_ptr = (void __user *)(unsigned long)out_resp->data;
> +	if (copy_from_user(blob_data, blob_ptr, out_resp->length)) {
> +		ret = -EFAULT;
> +		goto out_data;
> +	}
> +
> +	blob = drm_property_create_blob(dev, out_resp->length, blob_data);
> +	if (!blob) {
> +		ret = -EINVAL;
drm_property_create_blob can fail from -ENOMEM or -EINVAL here, so correctly return the error from drm_property_create_blob?

You're also doing a double allocation, one for blob_ptr and the other for blob. It might be better to do a single allocation of the blob and copy
the data to blob->data directly.

For the whole series if this patch is fixed up:
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 03245fb..4737794 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4216,6 +4216,9 @@  drm_property_create_blob(struct drm_device *dev, size_t length,
 	if (!blob)
 		return NULL;
 
+	/* This must be explicitly initialised, so we can safely call list_del
+	 * on it in the removal handler, even if it isn't in a file list. */
+	INIT_LIST_HEAD(&blob->head_file);
 	blob->length = length;
 	blob->dev = dev;
 
@@ -4232,7 +4235,8 @@  drm_property_create_blob(struct drm_device *dev, size_t length,
 
 	kref_init(&blob->refcount);
 
-	list_add_tail(&blob->head, &dev->mode_config.property_blob_list);
+	list_add_tail(&blob->head_global,
+	              &dev->mode_config.property_blob_list);
 
 	mutex_unlock(&dev->mode_config.blob_lock);
 
@@ -4254,7 +4258,8 @@  static void drm_property_free_blob(struct kref *kref)
 
 	WARN_ON(!mutex_is_locked(&blob->dev->mode_config.blob_lock));
 
-	list_del(&blob->head);
+	list_del(&blob->head_global);
+	list_del(&blob->head_file);
 	drm_mode_object_put(blob->dev, &blob->base);
 
 	kfree(blob);
@@ -4308,6 +4313,26 @@  static void drm_property_unreference_blob_locked(struct drm_property_blob *blob)
 }
 
 /**
+ * drm_property_destroy_user_blobs - destroy all blobs created by this client
+ * @dev:       DRM device
+ * @file_priv: destroy all blobs owned by this file handle
+ */
+void drm_property_destroy_user_blobs(struct drm_device *dev,
+				     struct drm_file *file_priv)
+{
+	struct drm_property_blob *blob, *bt;
+
+	mutex_lock(&dev->mode_config.blob_lock);
+
+	list_for_each_entry_safe(blob, bt, &file_priv->blobs, head_file) {
+		list_del_init(&blob->head_file);
+		drm_property_unreference_blob_locked(blob);
+	}
+
+	mutex_unlock(&dev->mode_config.blob_lock);
+}
+
+/**
  * drm_property_reference_blob - Take a reference on an existing property
  *
  * Take a new reference on an existing blob property.
@@ -4497,6 +4522,135 @@  done:
 }
 
 /**
+ * drm_mode_createblob_ioctl - create a new blob property
+ * @dev: DRM device
+ * @data: ioctl data
+ * @file_priv: DRM file info
+ *
+ * This function creates a new blob property with user-defined values. In order
+ * to give us sensible validation and checking when creating, rather than at
+ * every potential use, we also require a type to be provided upfront.
+ *
+ * Called by the user via ioctl.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_mode_createblob_ioctl(struct drm_device *dev,
+			      void *data, struct drm_file *file_priv)
+{
+	struct drm_mode_create_blob *out_resp = data;
+	struct drm_property_blob *blob;
+	void *blob_data;
+	int ret = 0;
+	void __user *blob_ptr;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return -EINVAL;
+
+	switch (out_resp->type) {
+	case DRM_MODE_BLOB_TYPE_MODE: {
+		if (out_resp->length != sizeof(struct drm_mode_modeinfo)) {
+			ret = -E2BIG;
+			goto out_unlocked;
+		}
+		break;
+	}
+	default:
+		ret = -EINVAL;
+		goto out_unlocked;
+	}
+
+	blob_data = kzalloc(out_resp->length, GFP_USER);
+	if (!blob_data) {
+		ret = -ENOMEM;
+		goto out_unlocked;
+	}
+
+	blob_ptr = (void __user *)(unsigned long)out_resp->data;
+	if (copy_from_user(blob_data, blob_ptr, out_resp->length)) {
+		ret = -EFAULT;
+		goto out_data;
+	}
+
+	blob = drm_property_create_blob(dev, out_resp->length, blob_data);
+	if (!blob) {
+		ret = -EINVAL;
+		goto out_data;
+	}
+
+	/* Dropping the lock between create_blob and our access here is safe
+	 * as only the same file_priv can remove the blob; at this point, it is
+	 * not associated with any file_priv. */
+	mutex_lock(&dev->mode_config.blob_lock);
+	out_resp->blob_id = blob->base.id;
+	list_add_tail(&file_priv->blobs, &blob->head_file);
+	mutex_unlock(&dev->mode_config.blob_lock);
+
+out_data:
+	kfree(blob_data);
+out_unlocked:
+	return ret;
+}
+
+/**
+ * drm_mode_destroyblob_ioctl - destroy a user blob property
+ * @dev: DRM device
+ * @data: ioctl data
+ * @file_priv: DRM file info
+ *
+ * Destroy an existing user-defined blob property.
+ *
+ * Called by the user via ioctl.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_mode_destroyblob_ioctl(struct drm_device *dev,
+			       void *data, struct drm_file *file_priv)
+{
+	struct drm_mode_destroy_blob *out_resp = data;
+	struct drm_property_blob *blob = NULL, *bt;
+	bool found = false;
+	int ret = 0;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return -EINVAL;
+
+	mutex_lock(&dev->mode_config.blob_lock);
+	blob = __drm_property_lookup_blob(dev, out_resp->blob_id);
+	if (!blob) {
+		ret = -ENOENT;
+		goto err;
+	}
+
+	/* Ensure the property was actually created by this user. */
+	list_for_each_entry(bt, &file_priv->blobs, head_file) {
+		if (bt == blob) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		ret = -EPERM;
+		goto err;
+	}
+
+	/* We must drop head_file here, because we may not be the last
+	 * reference on the blob. */
+	list_del_init(&blob->head_file);
+	drm_property_unreference_blob_locked(blob);
+	mutex_unlock(&dev->mode_config.blob_lock);
+
+	return 0;
+
+err:
+	mutex_unlock(&dev->mode_config.blob_lock);
+	return ret;
+}
+
+/**
  * drm_mode_connector_set_path_property - set tile property on connector
  * @connector: connector to set property on.
  * @path: path to use for property; must not be NULL.
@@ -5699,7 +5853,7 @@  void drm_mode_config_cleanup(struct drm_device *dev)
 	}
 
 	list_for_each_entry_safe(blob, bt, &dev->mode_config.property_blob_list,
-				 head) {
+				 head_global) {
 		drm_property_unreference_blob(blob);
 	}
 
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 076dd60..09846ed 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -167,6 +167,7 @@  static int drm_open_helper(struct file *filp, struct drm_minor *minor)
 	INIT_LIST_HEAD(&priv->lhead);
 	INIT_LIST_HEAD(&priv->fbs);
 	mutex_init(&priv->fbs_lock);
+	INIT_LIST_HEAD(&priv->blobs);
 	INIT_LIST_HEAD(&priv->event_list);
 	init_waitqueue_head(&priv->event_wait);
 	priv->event_space = 4096; /* set aside 4k for event buffer */
@@ -408,8 +409,10 @@  int drm_release(struct inode *inode, struct file *filp)
 
 	drm_events_release(file_priv);
 
-	if (drm_core_check_feature(dev, DRIVER_MODESET))
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		drm_fb_release(file_priv);
+		drm_property_destroy_user_blobs(dev, file_priv);
+	}
 
 	if (drm_core_check_feature(dev, DRIVER_GEM))
 		drm_gem_release(dev, file_priv);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 266dcd6..9bac1b7 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -641,6 +641,8 @@  static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_SETPROPERTY, drm_mode_obj_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CURSOR2, drm_mode_cursor2_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATEPROPBLOB, drm_mode_createblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROYPROPBLOB, drm_mode_destroyblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 62c40777..c5c717e 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -326,6 +326,10 @@  struct drm_file {
 	struct list_head fbs;
 	struct mutex fbs_lock;
 
+	/** User-created blob properties; this retains a reference on the
+	 *  property. */
+	struct list_head blobs;
+
 	wait_queue_head_t event_wait;
 	struct list_head event_list;
 	int event_space;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 07c99f6..a228614 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -219,7 +219,8 @@  struct drm_property_blob {
 	struct drm_mode_object base;
 	struct drm_device *dev;
 	struct kref refcount;
-	struct list_head head;
+	struct list_head head_global;
+	struct list_head head_file;
 	size_t length;
 	unsigned char data[];
 };
@@ -1289,6 +1290,8 @@  extern const char *drm_get_dvi_i_select_name(int val);
 extern const char *drm_get_tv_subconnector_name(int val);
 extern const char *drm_get_tv_select_name(int val);
 extern void drm_fb_release(struct drm_file *file_priv);
+extern void drm_property_destroy_user_blobs(struct drm_device *dev,
+                                            struct drm_file *file_priv);
 extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group);
 extern void drm_mode_group_destroy(struct drm_mode_group *group);
 extern void drm_reinit_primary_mode_group(struct drm_device *dev);
@@ -1434,6 +1437,10 @@  extern int drm_mode_getproperty_ioctl(struct drm_device *dev,
 				      void *data, struct drm_file *file_priv);
 extern int drm_mode_getblob_ioctl(struct drm_device *dev,
 				  void *data, struct drm_file *file_priv);
+extern int drm_mode_createblob_ioctl(struct drm_device *dev,
+				     void *data, struct drm_file *file_priv);
+extern int drm_mode_destroyblob_ioctl(struct drm_device *dev,
+				      void *data, struct drm_file *file_priv);
 extern int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
 					      void *data, struct drm_file *file_priv);
 extern int drm_mode_getencoder(struct drm_device *dev,
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index ff6ef62..3801584 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -786,6 +786,8 @@  struct drm_prime_handle {
 #define DRM_IOCTL_MODE_OBJ_SETPROPERTY	DRM_IOWR(0xBA, struct drm_mode_obj_set_property)
 #define DRM_IOCTL_MODE_CURSOR2		DRM_IOWR(0xBB, struct drm_mode_cursor2)
 #define DRM_IOCTL_MODE_ATOMIC		DRM_IOWR(0xBC, struct drm_mode_atomic)
+#define DRM_IOCTL_MODE_CREATEPROPBLOB	DRM_IOWR(0xBD, struct drm_mode_create_blob)
+#define DRM_IOCTL_MODE_DESTROYPROPBLOB	DRM_IOWR(0xBE, struct drm_mode_destroy_blob)
 
 /**
  * Device specific ioctls should only be in their respective headers
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index dbeba94..73ac925 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -558,4 +558,26 @@  struct drm_mode_atomic {
 	__u64 user_data;
 };
 
+#define DRM_MODE_BLOB_TYPE_MODE		1 /**< drm_mode_modeinfo */
+
+/**
+ * Create a new 'blob' data property, copying length bytes from data pointer,
+ * and returning new blob ID.
+ */
+struct drm_mode_create_blob {
+	__u32 type;
+	__u32 length;
+	/** Pointer to data to create blob. */
+	__u64 data;
+	/** Return: new property ID. */
+	__u32 blob_id;
+};
+
+/**
+ * Destroy a user-created blob property.
+ */
+struct drm_mode_destroy_blob {
+	__u32 blob_id;
+};
+
 #endif