[PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl

[PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl

[Benjamin Tissoires]

This adds two new ioctls, UINPUT_DEV_SETUP and UI_ABS_SETUP, that
replaces the old device setup method (by write()'ing "struct
uinput_user_dev" to the node). The old method is not easily extendable
and requires huge payloads. Furthermore, overloading write() without
properly versioned objects is error-prone.

Therefore, we introduce two new ioctls to replace the old method.
These ioctls support all features of the old method, plus a "resolution"
field for absinfo. Furthermore, it's properly forward-compatible to new
ABS codes and a growing "struct input_absinfo" structure.

UI_ABS_SETUP also allows user-space to skip unknown axes if not set.
There is no need to copy the whole array temporarily into the kernel,
but instead the caller issues several ioctl where we copy each value
manually.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
---

Changes from v2:
- returned E2BIG when the userspace has been compiled against a newer kernel
  than the current one
- formatting changes
- remove extra memset


 drivers/input/misc/uinput.c | 80 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/uinput.h      |  5 +++
 include/uapi/linux/uinput.h | 83 +++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 162 insertions(+), 6 deletions(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 345df9b..7af1e54 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -370,8 +370,73 @@ static int uinput_allocate_device(struct uinput_device *udev)
 	return 0;
 }
 
-static int uinput_setup_device(struct uinput_device *udev,
-			       const char __user *buffer, size_t count)
+static int uinput_dev_setup(struct uinput_device *udev,
+			    struct uinput_setup __user *arg)
+{
+	struct uinput_setup setup;
+	struct input_dev *dev;
+	int retval;
+
+	if (udev->state == UIST_CREATED)
+		return -EINVAL;
+	if (copy_from_user(&setup, arg, sizeof(setup)))
+		return -EFAULT;
+	if (!setup.name[0])
+		return -EINVAL;
+
+	dev = udev->dev;
+	dev->id = setup.id;
+	udev->ff_effects_max = setup.ff_effects_max;
+
+	kfree(dev->name);
+	dev->name = kstrndup(setup.name, UINPUT_MAX_NAME_SIZE, GFP_KERNEL);
+	if (!dev->name)
+		return -ENOMEM;
+
+	retval = uinput_validate_absbits(dev);
+	if (retval < 0)
+		return retval;
+
+	udev->state = UIST_SETUP_COMPLETE;
+	return 0;
+}
+
+static int uinput_abs_setup(struct uinput_device *udev,
+			    struct uinput_setup __user *arg, size_t size)
+{
+	struct uinput_abs_setup setup = {};
+	struct input_dev *dev;
+
+	if (size > sizeof(setup))
+		return -E2BIG;
+	if (udev->state == UIST_CREATED)
+		return -EINVAL;
+	if (copy_from_user(&setup, arg, size))
+		return -EFAULT;
+	if (setup.code > ABS_MAX)
+		return -ERANGE;
+
+	dev = udev->dev;
+
+	input_alloc_absinfo(dev);
+	if (!dev->absinfo)
+		return -ENOMEM;
+
+	set_bit(setup.code, dev->absbit);
+	dev->absinfo[setup.code] = setup.absinfo;
+
+	/*
+	 * We restore the state to UIST_NEW_DEVICE because the user has to call
+	 * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the
+	 * validity of the absbits.
+	 */
+	udev->state = UIST_NEW_DEVICE;
+	return 0;
+}
+
+/* legacy setup via write() */
+static int uinput_setup_device_legacy(struct uinput_device *udev,
+				      const char __user *buffer, size_t count)
 {
 	struct uinput_user_dev	*user_dev;
 	struct input_dev	*dev;
@@ -474,7 +539,7 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer,
 
 	retval = udev->state == UIST_CREATED ?
 			uinput_inject_events(udev, buffer, count) :
-			uinput_setup_device(udev, buffer, count);
+			uinput_setup_device_legacy(udev, buffer, count);
 
 	mutex_unlock(&udev->mutex);
 
@@ -735,6 +800,12 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 			uinput_destroy_device(udev);
 			goto out;
 
+		case UI_DEV_SETUP:
+			retval = uinput_dev_setup(udev, p);
+			goto out;
+
+		/* UI_ABS_SETUP is handled in the variable size ioctls */
+
 		case UI_SET_EVBIT:
 			retval = uinput_set_bit(arg, evbit, EV_MAX);
 			goto out;
@@ -879,6 +950,9 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 		name = dev_name(&udev->dev->dev);
 		retval = uinput_str_to_user(p, name, size);
 		goto out;
+	case UI_ABS_SETUP & ~IOCSIZE_MASK:
+		retval = uinput_abs_setup(udev, p, size);
+		goto out;
 	}
 
 	retval = -EINVAL;
diff --git a/include/linux/uinput.h b/include/linux/uinput.h
index 0994c0d..75de43d 100644
--- a/include/linux/uinput.h
+++ b/include/linux/uinput.h
@@ -20,6 +20,11 @@
  * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
  *
  * Changes/Revisions:
+ *	0.5	08/13/2015 (David Herrmann <dh.herrmann@gmail.com> &
+ *			    Benjamin Tissoires <benjamin.tissoires@redhat.com>)
+ *		- add UI_DEV_SETUP ioctl
+ *		- add UI_ABS_SETUP ioctl
+ *		- add UI_GET_VERSION ioctl
  *	0.4	01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
  *		- add UI_GET_SYSNAME ioctl
  *	0.3	24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
index 013c9d8..ef6c9f5 100644
--- a/include/uapi/linux/uinput.h
+++ b/include/uapi/linux/uinput.h
@@ -20,6 +20,11 @@
  * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
  *
  * Changes/Revisions:
+ *	0.5	08/13/2015 (David Herrmann <dh.herrmann@gmail.com> &
+ *			    Benjamin Tissoires <benjamin.tissoires@redhat.com>)
+ *		- add UI_DEV_SETUP ioctl
+ *		- add UI_ABS_SETUP ioctl
+ *		- add UI_GET_VERSION ioctl
  *	0.4	01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
  *		- add UI_GET_SYSNAME ioctl
  *	0.3	24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
@@ -37,8 +42,8 @@
 #include <linux/types.h>
 #include <linux/input.h>
 
-#define UINPUT_VERSION		4
-
+#define UINPUT_VERSION		5
+#define UINPUT_MAX_NAME_SIZE	80
 
 struct uinput_ff_upload {
 	__u32			request_id;
@@ -58,6 +63,79 @@ struct uinput_ff_erase {
 #define UI_DEV_CREATE		_IO(UINPUT_IOCTL_BASE, 1)
 #define UI_DEV_DESTROY		_IO(UINPUT_IOCTL_BASE, 2)
 
+struct uinput_setup {
+	struct input_id id;
+	char name[UINPUT_MAX_NAME_SIZE];
+	__u32 ff_effects_max;
+};
+
+/**
+ * UI_DEV_SETUP - Set device parameters for setup
+ *
+ * This ioctl sets parameters for the input-device to be created. It must be
+ * issued *before* calling UI_DEV_CREATE or it will fail. This ioctl supersedes
+ * the old "struct uinput_user_dev" method, which wrote this data via write().
+ * To actually set the absolute axes, you also need to call the ioctl
+ * UI_ABS_SETUP *before* calling this ioctl.
+ *
+ * This ioctl takes a "struct uinput_setup" object as argument. The fields of
+ * this object are as follows:
+ *              id: See the description of "struct input_id". This field is
+ *                  copied unchanged into the new device.
+ *            name: This is used unchanged as name for the new device.
+ *  ff_effects_max: This limits the maximum numbers of force-feedback effects.
+ *                  See below for a description of FF with uinput.
+ *
+ * This ioctl can be called multiple times and will overwrite previous values.
+ * If this ioctl fails with -EINVAL, you're recommended to use the old
+ * "uinput_user_dev" method via write() as fallback, in case you run on an old
+ * kernel that does not support this ioctl.
+ *
+ * This ioctl may fail with -EINVAL if it is not supported or if you passed
+ * incorrect values, -ENOMEM if the kernel runs out of memory or -EFAULT if the
+ * passed uinput_setup object cannot be read/written.
+ * If this call fails, partial data may have already been applied to the
+ * internal device.
+ */
+#define UI_DEV_SETUP _IOW(UINPUT_IOCTL_BASE, 3, struct uinput_setup)
+
+struct uinput_abs_setup {
+	__u16  code; /* axis code */
+	/* __u16 filler; */
+	struct input_absinfo absinfo;
+};
+
+/**
+ * UI_ABS_SETUP - Set absolute axis information for the device to setup
+ *
+ * This ioctl sets one absolute axis information for the input-device to be
+ * created. It must be issued *before* calling UI_DEV_SETUP and UI_DEV_CREATE
+ * for every absolute axis the device exports.
+ * This ioctl supersedes the old "struct uinput_user_dev" method, which wrote
+ * part of this data and the content of UI_DEV_SETUP via write().
+ *
+ * This ioctl takes a "struct uinput_abs_setup" object as argument. The fields
+ * of this object are as follows:
+ *            code: The corresponding input code associated with this axis
+ *                  (ABS_X, ABS_Y, etc...)
+ *         absinfo: See "struct input_absinfo" for a description of this field.
+ *                  This field is copied unchanged into the kernel for the
+ *                  specified axis. If the axis is not enabled via
+ *                  UI_SET_ABSBIT, this ioctl will enable it.
+ *
+ * This ioctl can be called multiple times and will overwrite previous values.
+ * If this ioctl fails with -EINVAL, you're recommended to use the old
+ * "uinput_user_dev" method via write() as fallback, in case you run on an old
+ * kernel that does not support this ioctl.
+ *
+ * This ioctl may fail with -EINVAL if it is not supported or if you passed
+ * incorrect values, -ENOMEM if the kernel runs out of memory or -EFAULT if the
+ * passed uinput_setup object cannot be read/written.
+ * If this call fails, partial data may have already been applied to the
+ * internal device.
+ */
+#define UI_ABS_SETUP _IOW(UINPUT_IOCTL_BASE, 4, struct uinput_abs_setup)
+
 #define UI_SET_EVBIT		_IOW(UINPUT_IOCTL_BASE, 100, int)
 #define UI_SET_KEYBIT		_IOW(UINPUT_IOCTL_BASE, 101, int)
 #define UI_SET_RELBIT		_IOW(UINPUT_IOCTL_BASE, 102, int)
@@ -144,7 +222,6 @@ struct uinput_ff_erase {
 #define UI_FF_UPLOAD		1
 #define UI_FF_ERASE		2
 
-#define UINPUT_MAX_NAME_SIZE	80
 struct uinput_user_dev {
 	char name[UINPUT_MAX_NAME_SIZE];
 	struct input_id id;
-- 
2.4.3

Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl

[David Herrmann]

Hi Dmitry

Any comment on this?

Thanks
David

On Tue, Aug 25, 2015 at 5:12 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> This adds two new ioctls, UINPUT_DEV_SETUP and UI_ABS_SETUP, that
> replaces the old device setup method (by write()'ing "struct
> uinput_user_dev" to the node). The old method is not easily extendable
> and requires huge payloads. Furthermore, overloading write() without
> properly versioned objects is error-prone.
>
> Therefore, we introduce two new ioctls to replace the old method.
> These ioctls support all features of the old method, plus a "resolution"
> field for absinfo. Furthermore, it's properly forward-compatible to new
> ABS codes and a growing "struct input_absinfo" structure.
>
> UI_ABS_SETUP also allows user-space to skip unknown axes if not set.
> There is no need to copy the whole array temporarily into the kernel,
> but instead the caller issues several ioctl where we copy each value
> manually.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>
> Changes from v2:
> - returned E2BIG when the userspace has been compiled against a newer kernel
>   than the current one
> - formatting changes
> - remove extra memset
>
>
>  drivers/input/misc/uinput.c | 80 +++++++++++++++++++++++++++++++++++++++++--
>  include/linux/uinput.h      |  5 +++
>  include/uapi/linux/uinput.h | 83 +++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 162 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 345df9b..7af1e54 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -370,8 +370,73 @@ static int uinput_allocate_device(struct uinput_device *udev)
>         return 0;
>  }
>
> -static int uinput_setup_device(struct uinput_device *udev,
> -                              const char __user *buffer, size_t count)
> +static int uinput_dev_setup(struct uinput_device *udev,
> +                           struct uinput_setup __user *arg)
> +{
> +       struct uinput_setup setup;
> +       struct input_dev *dev;
> +       int retval;
> +
> +       if (udev->state == UIST_CREATED)
> +               return -EINVAL;
> +       if (copy_from_user(&setup, arg, sizeof(setup)))
> +               return -EFAULT;
> +       if (!setup.name[0])
> +               return -EINVAL;
> +
> +       dev = udev->dev;
> +       dev->id = setup.id;
> +       udev->ff_effects_max = setup.ff_effects_max;
> +
> +       kfree(dev->name);
> +       dev->name = kstrndup(setup.name, UINPUT_MAX_NAME_SIZE, GFP_KERNEL);
> +       if (!dev->name)
> +               return -ENOMEM;
> +
> +       retval = uinput_validate_absbits(dev);
> +       if (retval < 0)
> +               return retval;
> +
> +       udev->state = UIST_SETUP_COMPLETE;
> +       return 0;
> +}
> +
> +static int uinput_abs_setup(struct uinput_device *udev,
> +                           struct uinput_setup __user *arg, size_t size)
> +{
> +       struct uinput_abs_setup setup = {};
> +       struct input_dev *dev;
> +
> +       if (size > sizeof(setup))
> +               return -E2BIG;
> +       if (udev->state == UIST_CREATED)
> +               return -EINVAL;
> +       if (copy_from_user(&setup, arg, size))
> +               return -EFAULT;
> +       if (setup.code > ABS_MAX)
> +               return -ERANGE;
> +
> +       dev = udev->dev;
> +
> +       input_alloc_absinfo(dev);
> +       if (!dev->absinfo)
> +               return -ENOMEM;
> +
> +       set_bit(setup.code, dev->absbit);
> +       dev->absinfo[setup.code] = setup.absinfo;
> +
> +       /*
> +        * We restore the state to UIST_NEW_DEVICE because the user has to call
> +        * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the
> +        * validity of the absbits.
> +        */
> +       udev->state = UIST_NEW_DEVICE;
> +       return 0;
> +}
> +
> +/* legacy setup via write() */
> +static int uinput_setup_device_legacy(struct uinput_device *udev,
> +                                     const char __user *buffer, size_t count)
>  {
>         struct uinput_user_dev  *user_dev;
>         struct input_dev        *dev;
> @@ -474,7 +539,7 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer,
>
>         retval = udev->state == UIST_CREATED ?
>                         uinput_inject_events(udev, buffer, count) :
> -                       uinput_setup_device(udev, buffer, count);
> +                       uinput_setup_device_legacy(udev, buffer, count);
>
>         mutex_unlock(&udev->mutex);
>
> @@ -735,6 +800,12 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>                         uinput_destroy_device(udev);
>                         goto out;
>
> +               case UI_DEV_SETUP:
> +                       retval = uinput_dev_setup(udev, p);
> +                       goto out;
> +
> +               /* UI_ABS_SETUP is handled in the variable size ioctls */
> +
>                 case UI_SET_EVBIT:
>                         retval = uinput_set_bit(arg, evbit, EV_MAX);
>                         goto out;
> @@ -879,6 +950,9 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>                 name = dev_name(&udev->dev->dev);
>                 retval = uinput_str_to_user(p, name, size);
>                 goto out;
> +       case UI_ABS_SETUP & ~IOCSIZE_MASK:
> +               retval = uinput_abs_setup(udev, p, size);
> +               goto out;
>         }
>
>         retval = -EINVAL;
> diff --git a/include/linux/uinput.h b/include/linux/uinput.h
> index 0994c0d..75de43d 100644
> --- a/include/linux/uinput.h
> +++ b/include/linux/uinput.h
> @@ -20,6 +20,11 @@
>   * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
>   *
>   * Changes/Revisions:
> + *     0.5     08/13/2015 (David Herrmann <dh.herrmann@gmail.com> &
> + *                         Benjamin Tissoires <benjamin.tissoires@redhat.com>)
> + *             - add UI_DEV_SETUP ioctl
> + *             - add UI_ABS_SETUP ioctl
> + *             - add UI_GET_VERSION ioctl
>   *     0.4     01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
>   *             - add UI_GET_SYSNAME ioctl
>   *     0.3     24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index 013c9d8..ef6c9f5 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -20,6 +20,11 @@
>   * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
>   *
>   * Changes/Revisions:
> + *     0.5     08/13/2015 (David Herrmann <dh.herrmann@gmail.com> &
> + *                         Benjamin Tissoires <benjamin.tissoires@redhat.com>)
> + *             - add UI_DEV_SETUP ioctl
> + *             - add UI_ABS_SETUP ioctl
> + *             - add UI_GET_VERSION ioctl
>   *     0.4     01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
>   *             - add UI_GET_SYSNAME ioctl
>   *     0.3     24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
> @@ -37,8 +42,8 @@
>  #include <linux/types.h>
>  #include <linux/input.h>
>
> -#define UINPUT_VERSION         4
> -
> +#define UINPUT_VERSION         5
> +#define UINPUT_MAX_NAME_SIZE   80
>
>  struct uinput_ff_upload {
>         __u32                   request_id;
> @@ -58,6 +63,79 @@ struct uinput_ff_erase {
>  #define UI_DEV_CREATE          _IO(UINPUT_IOCTL_BASE, 1)
>  #define UI_DEV_DESTROY         _IO(UINPUT_IOCTL_BASE, 2)
>
> +struct uinput_setup {
> +       struct input_id id;
> +       char name[UINPUT_MAX_NAME_SIZE];
> +       __u32 ff_effects_max;
> +};
> +
> +/**
> + * UI_DEV_SETUP - Set device parameters for setup
> + *
> + * This ioctl sets parameters for the input-device to be created. It must be
> + * issued *before* calling UI_DEV_CREATE or it will fail. This ioctl supersedes
> + * the old "struct uinput_user_dev" method, which wrote this data via write().
> + * To actually set the absolute axes, you also need to call the ioctl
> + * UI_ABS_SETUP *before* calling this ioctl.
> + *
> + * This ioctl takes a "struct uinput_setup" object as argument. The fields of
> + * this object are as follows:
> + *              id: See the description of "struct input_id". This field is
> + *                  copied unchanged into the new device.
> + *            name: This is used unchanged as name for the new device.
> + *  ff_effects_max: This limits the maximum numbers of force-feedback effects.
> + *                  See below for a description of FF with uinput.
> + *
> + * This ioctl can be called multiple times and will overwrite previous values.
> + * If this ioctl fails with -EINVAL, you're recommended to use the old
> + * "uinput_user_dev" method via write() as fallback, in case you run on an old
> + * kernel that does not support this ioctl.
> + *
> + * This ioctl may fail with -EINVAL if it is not supported or if you passed
> + * incorrect values, -ENOMEM if the kernel runs out of memory or -EFAULT if the
> + * passed uinput_setup object cannot be read/written.
> + * If this call fails, partial data may have already been applied to the
> + * internal device.
> + */
> +#define UI_DEV_SETUP _IOW(UINPUT_IOCTL_BASE, 3, struct uinput_setup)
> +
> +struct uinput_abs_setup {
> +       __u16  code; /* axis code */
> +       /* __u16 filler; */
> +       struct input_absinfo absinfo;
> +};
> +
> +/**
> + * UI_ABS_SETUP - Set absolute axis information for the device to setup
> + *
> + * This ioctl sets one absolute axis information for the input-device to be
> + * created. It must be issued *before* calling UI_DEV_SETUP and UI_DEV_CREATE
> + * for every absolute axis the device exports.
> + * This ioctl supersedes the old "struct uinput_user_dev" method, which wrote
> + * part of this data and the content of UI_DEV_SETUP via write().
> + *
> + * This ioctl takes a "struct uinput_abs_setup" object as argument. The fields
> + * of this object are as follows:
> + *            code: The corresponding input code associated with this axis
> + *                  (ABS_X, ABS_Y, etc...)
> + *         absinfo: See "struct input_absinfo" for a description of this field.
> + *                  This field is copied unchanged into the kernel for the
> + *                  specified axis. If the axis is not enabled via
> + *                  UI_SET_ABSBIT, this ioctl will enable it.
> + *
> + * This ioctl can be called multiple times and will overwrite previous values.
> + * If this ioctl fails with -EINVAL, you're recommended to use the old
> + * "uinput_user_dev" method via write() as fallback, in case you run on an old
> + * kernel that does not support this ioctl.
> + *
> + * This ioctl may fail with -EINVAL if it is not supported or if you passed
> + * incorrect values, -ENOMEM if the kernel runs out of memory or -EFAULT if the
> + * passed uinput_setup object cannot be read/written.
> + * If this call fails, partial data may have already been applied to the
> + * internal device.
> + */
> +#define UI_ABS_SETUP _IOW(UINPUT_IOCTL_BASE, 4, struct uinput_abs_setup)
> +
>  #define UI_SET_EVBIT           _IOW(UINPUT_IOCTL_BASE, 100, int)
>  #define UI_SET_KEYBIT          _IOW(UINPUT_IOCTL_BASE, 101, int)
>  #define UI_SET_RELBIT          _IOW(UINPUT_IOCTL_BASE, 102, int)
> @@ -144,7 +222,6 @@ struct uinput_ff_erase {
>  #define UI_FF_UPLOAD           1
>  #define UI_FF_ERASE            2
>
> -#define UINPUT_MAX_NAME_SIZE   80
>  struct uinput_user_dev {
>         char name[UINPUT_MAX_NAME_SIZE];
>         struct input_id id;
> --
> 2.4.3
>

Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl

[Benjamin Tissoires]

Dmitry,

ping again on this one :)

any comments?

Cheers,
Benjamin

On Mon, Sep 21, 2015 at 6:45 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi Dmitry
>
> Any comment on this?
>
> Thanks
> David
>
> On Tue, Aug 25, 2015 at 5:12 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> This adds two new ioctls, UINPUT_DEV_SETUP and UI_ABS_SETUP, that
>> replaces the old device setup method (by write()'ing "struct
>> uinput_user_dev" to the node). The old method is not easily extendable
>> and requires huge payloads. Furthermore, overloading write() without
>> properly versioned objects is error-prone.
>>
>> Therefore, we introduce two new ioctls to replace the old method.
>> These ioctls support all features of the old method, plus a "resolution"
>> field for absinfo. Furthermore, it's properly forward-compatible to new
>> ABS codes and a growing "struct input_absinfo" structure.
>>
>> UI_ABS_SETUP also allows user-space to skip unknown axes if not set.
>> There is no need to copy the whole array temporarily into the kernel,
>> but instead the caller issues several ioctl where we copy each value
>> manually.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>>
>> Changes from v2:
>> - returned E2BIG when the userspace has been compiled against a newer kernel
>>   than the current one
>> - formatting changes
>> - remove extra memset
>>
>>
>>  drivers/input/misc/uinput.c | 80 +++++++++++++++++++++++++++++++++++++++++--
>>  include/linux/uinput.h      |  5 +++
>>  include/uapi/linux/uinput.h | 83 +++++++++++++++++++++++++++++++++++++++++++--
>>  3 files changed, 162 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> index 345df9b..7af1e54 100644
>> --- a/drivers/input/misc/uinput.c
>> +++ b/drivers/input/misc/uinput.c
>> @@ -370,8 +370,73 @@ static int uinput_allocate_device(struct uinput_device *udev)
>>         return 0;
>>  }
>>
>> -static int uinput_setup_device(struct uinput_device *udev,
>> -                              const char __user *buffer, size_t count)
>> +static int uinput_dev_setup(struct uinput_device *udev,
>> +                           struct uinput_setup __user *arg)
>> +{
>> +       struct uinput_setup setup;
>> +       struct input_dev *dev;
>> +       int retval;
>> +
>> +       if (udev->state == UIST_CREATED)
>> +               return -EINVAL;
>> +       if (copy_from_user(&setup, arg, sizeof(setup)))
>> +               return -EFAULT;
>> +       if (!setup.name[0])
>> +               return -EINVAL;
>> +
>> +       dev = udev->dev;
>> +       dev->id = setup.id;
>> +       udev->ff_effects_max = setup.ff_effects_max;
>> +
>> +       kfree(dev->name);
>> +       dev->name = kstrndup(setup.name, UINPUT_MAX_NAME_SIZE, GFP_KERNEL);
>> +       if (!dev->name)
>> +               return -ENOMEM;
>> +
>> +       retval = uinput_validate_absbits(dev);
>> +       if (retval < 0)
>> +               return retval;
>> +
>> +       udev->state = UIST_SETUP_COMPLETE;
>> +       return 0;
>> +}
>> +
>> +static int uinput_abs_setup(struct uinput_device *udev,
>> +                           struct uinput_setup __user *arg, size_t size)
>> +{
>> +       struct uinput_abs_setup setup = {};
>> +       struct input_dev *dev;
>> +
>> +       if (size > sizeof(setup))
>> +               return -E2BIG;
>> +       if (udev->state == UIST_CREATED)
>> +               return -EINVAL;
>> +       if (copy_from_user(&setup, arg, size))
>> +               return -EFAULT;
>> +       if (setup.code > ABS_MAX)
>> +               return -ERANGE;
>> +
>> +       dev = udev->dev;
>> +
>> +       input_alloc_absinfo(dev);
>> +       if (!dev->absinfo)
>> +               return -ENOMEM;
>> +
>> +       set_bit(setup.code, dev->absbit);
>> +       dev->absinfo[setup.code] = setup.absinfo;
>> +
>> +       /*
>> +        * We restore the state to UIST_NEW_DEVICE because the user has to call
>> +        * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the
>> +        * validity of the absbits.
>> +        */
>> +       udev->state = UIST_NEW_DEVICE;
>> +       return 0;
>> +}
>> +
>> +/* legacy setup via write() */
>> +static int uinput_setup_device_legacy(struct uinput_device *udev,
>> +                                     const char __user *buffer, size_t count)
>>  {
>>         struct uinput_user_dev  *user_dev;
>>         struct input_dev        *dev;
>> @@ -474,7 +539,7 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer,
>>
>>         retval = udev->state == UIST_CREATED ?
>>                         uinput_inject_events(udev, buffer, count) :
>> -                       uinput_setup_device(udev, buffer, count);
>> +                       uinput_setup_device_legacy(udev, buffer, count);
>>
>>         mutex_unlock(&udev->mutex);
>>
>> @@ -735,6 +800,12 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>>                         uinput_destroy_device(udev);
>>                         goto out;
>>
>> +               case UI_DEV_SETUP:
>> +                       retval = uinput_dev_setup(udev, p);
>> +                       goto out;
>> +
>> +               /* UI_ABS_SETUP is handled in the variable size ioctls */
>> +
>>                 case UI_SET_EVBIT:
>>                         retval = uinput_set_bit(arg, evbit, EV_MAX);
>>                         goto out;
>> @@ -879,6 +950,9 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>>                 name = dev_name(&udev->dev->dev);
>>                 retval = uinput_str_to_user(p, name, size);
>>                 goto out;
>> +       case UI_ABS_SETUP & ~IOCSIZE_MASK:
>> +               retval = uinput_abs_setup(udev, p, size);
>> +               goto out;
>>         }
>>
>>         retval = -EINVAL;
>> diff --git a/include/linux/uinput.h b/include/linux/uinput.h
>> index 0994c0d..75de43d 100644
>> --- a/include/linux/uinput.h
>> +++ b/include/linux/uinput.h
>> @@ -20,6 +20,11 @@
>>   * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
>>   *
>>   * Changes/Revisions:
>> + *     0.5     08/13/2015 (David Herrmann <dh.herrmann@gmail.com> &
>> + *                         Benjamin Tissoires <benjamin.tissoires@redhat.com>)
>> + *             - add UI_DEV_SETUP ioctl
>> + *             - add UI_ABS_SETUP ioctl
>> + *             - add UI_GET_VERSION ioctl
>>   *     0.4     01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
>>   *             - add UI_GET_SYSNAME ioctl
>>   *     0.3     24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
>> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
>> index 013c9d8..ef6c9f5 100644
>> --- a/include/uapi/linux/uinput.h
>> +++ b/include/uapi/linux/uinput.h
>> @@ -20,6 +20,11 @@
>>   * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
>>   *
>>   * Changes/Revisions:
>> + *     0.5     08/13/2015 (David Herrmann <dh.herrmann@gmail.com> &
>> + *                         Benjamin Tissoires <benjamin.tissoires@redhat.com>)
>> + *             - add UI_DEV_SETUP ioctl
>> + *             - add UI_ABS_SETUP ioctl
>> + *             - add UI_GET_VERSION ioctl
>>   *     0.4     01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
>>   *             - add UI_GET_SYSNAME ioctl
>>   *     0.3     24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
>> @@ -37,8 +42,8 @@
>>  #include <linux/types.h>
>>  #include <linux/input.h>
>>
>> -#define UINPUT_VERSION         4
>> -
>> +#define UINPUT_VERSION         5
>> +#define UINPUT_MAX_NAME_SIZE   80
>>
>>  struct uinput_ff_upload {
>>         __u32                   request_id;
>> @@ -58,6 +63,79 @@ struct uinput_ff_erase {
>>  #define UI_DEV_CREATE          _IO(UINPUT_IOCTL_BASE, 1)
>>  #define UI_DEV_DESTROY         _IO(UINPUT_IOCTL_BASE, 2)
>>
>> +struct uinput_setup {
>> +       struct input_id id;
>> +       char name[UINPUT_MAX_NAME_SIZE];
>> +       __u32 ff_effects_max;
>> +};
>> +
>> +/**
>> + * UI_DEV_SETUP - Set device parameters for setup
>> + *
>> + * This ioctl sets parameters for the input-device to be created. It must be
>> + * issued *before* calling UI_DEV_CREATE or it will fail. This ioctl supersedes
>> + * the old "struct uinput_user_dev" method, which wrote this data via write().
>> + * To actually set the absolute axes, you also need to call the ioctl
>> + * UI_ABS_SETUP *before* calling this ioctl.
>> + *
>> + * This ioctl takes a "struct uinput_setup" object as argument. The fields of
>> + * this object are as follows:
>> + *              id: See the description of "struct input_id". This field is
>> + *                  copied unchanged into the new device.
>> + *            name: This is used unchanged as name for the new device.
>> + *  ff_effects_max: This limits the maximum numbers of force-feedback effects.
>> + *                  See below for a description of FF with uinput.
>> + *
>> + * This ioctl can be called multiple times and will overwrite previous values.
>> + * If this ioctl fails with -EINVAL, you're recommended to use the old
>> + * "uinput_user_dev" method via write() as fallback, in case you run on an old
>> + * kernel that does not support this ioctl.
>> + *
>> + * This ioctl may fail with -EINVAL if it is not supported or if you passed
>> + * incorrect values, -ENOMEM if the kernel runs out of memory or -EFAULT if the
>> + * passed uinput_setup object cannot be read/written.
>> + * If this call fails, partial data may have already been applied to the
>> + * internal device.
>> + */
>> +#define UI_DEV_SETUP _IOW(UINPUT_IOCTL_BASE, 3, struct uinput_setup)
>> +
>> +struct uinput_abs_setup {
>> +       __u16  code; /* axis code */
>> +       /* __u16 filler; */
>> +       struct input_absinfo absinfo;
>> +};
>> +
>> +/**
>> + * UI_ABS_SETUP - Set absolute axis information for the device to setup
>> + *
>> + * This ioctl sets one absolute axis information for the input-device to be
>> + * created. It must be issued *before* calling UI_DEV_SETUP and UI_DEV_CREATE
>> + * for every absolute axis the device exports.
>> + * This ioctl supersedes the old "struct uinput_user_dev" method, which wrote
>> + * part of this data and the content of UI_DEV_SETUP via write().
>> + *
>> + * This ioctl takes a "struct uinput_abs_setup" object as argument. The fields
>> + * of this object are as follows:
>> + *            code: The corresponding input code associated with this axis
>> + *                  (ABS_X, ABS_Y, etc...)
>> + *         absinfo: See "struct input_absinfo" for a description of this field.
>> + *                  This field is copied unchanged into the kernel for the
>> + *                  specified axis. If the axis is not enabled via
>> + *                  UI_SET_ABSBIT, this ioctl will enable it.
>> + *
>> + * This ioctl can be called multiple times and will overwrite previous values.
>> + * If this ioctl fails with -EINVAL, you're recommended to use the old
>> + * "uinput_user_dev" method via write() as fallback, in case you run on an old
>> + * kernel that does not support this ioctl.
>> + *
>> + * This ioctl may fail with -EINVAL if it is not supported or if you passed
>> + * incorrect values, -ENOMEM if the kernel runs out of memory or -EFAULT if the
>> + * passed uinput_setup object cannot be read/written.
>> + * If this call fails, partial data may have already been applied to the
>> + * internal device.
>> + */
>> +#define UI_ABS_SETUP _IOW(UINPUT_IOCTL_BASE, 4, struct uinput_abs_setup)
>> +
>>  #define UI_SET_EVBIT           _IOW(UINPUT_IOCTL_BASE, 100, int)
>>  #define UI_SET_KEYBIT          _IOW(UINPUT_IOCTL_BASE, 101, int)
>>  #define UI_SET_RELBIT          _IOW(UINPUT_IOCTL_BASE, 102, int)
>> @@ -144,7 +222,6 @@ struct uinput_ff_erase {
>>  #define UI_FF_UPLOAD           1
>>  #define UI_FF_ERASE            2
>>
>> -#define UINPUT_MAX_NAME_SIZE   80
>>  struct uinput_user_dev {
>>         char name[UINPUT_MAX_NAME_SIZE];
>>         struct input_id id;
>> --
>> 2.4.3
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl

[Dmitry Torokhov]

Hi Benjamin,

On Tue, Aug 25, 2015 at 11:12:59AM -0400, Benjamin Tissoires wrote:
> +static int uinput_abs_setup(struct uinput_device *udev,
> +			    struct uinput_setup __user *arg, size_t size)
> +{
> +	struct uinput_abs_setup setup = {};
> +	struct input_dev *dev;
> +
> +	if (size > sizeof(setup))
> +		return -E2BIG;
> +	if (udev->state == UIST_CREATED)
> +		return -EINVAL;
> +	if (copy_from_user(&setup, arg, size))
> +		return -EFAULT;
> +	if (setup.code > ABS_MAX)
> +		return -ERANGE;
> +
> +	dev = udev->dev;
> +
> +	input_alloc_absinfo(dev);
> +	if (!dev->absinfo)
> +		return -ENOMEM;
> +
> +	set_bit(setup.code, dev->absbit);
> +	dev->absinfo[setup.code] = setup.absinfo;
> +
> +	/*
> +	 * We restore the state to UIST_NEW_DEVICE because the user has to call
> +	 * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the
> +	 * validity of the absbits.
> +	 */

Do we have to be this strict? It seems to me that with the new IOCTLs
you naturally want to use the new ioctl to setup the device, then adjust
various axes and bits and then validate everything.

Thanks.

-- 
Dmitry

Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl

[David Herrmann]

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

Hi

On Sun, Oct 25, 2015 at 12:53 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Benjamin,
>
> On Tue, Aug 25, 2015 at 11:12:59AM -0400, Benjamin Tissoires wrote:
>> +static int uinput_abs_setup(struct uinput_device *udev,
>> +                         struct uinput_setup __user *arg, size_t size)
>> +{
>> +     struct uinput_abs_setup setup = {};
>> +     struct input_dev *dev;
>> +
>> +     if (size > sizeof(setup))
>> +             return -E2BIG;
>> +     if (udev->state == UIST_CREATED)
>> +             return -EINVAL;
>> +     if (copy_from_user(&setup, arg, size))
>> +             return -EFAULT;
>> +     if (setup.code > ABS_MAX)
>> +             return -ERANGE;
>> +
>> +     dev = udev->dev;
>> +
>> +     input_alloc_absinfo(dev);
>> +     if (!dev->absinfo)
>> +             return -ENOMEM;
>> +
>> +     set_bit(setup.code, dev->absbit);
>> +     dev->absinfo[setup.code] = setup.absinfo;
>> +
>> +     /*
>> +      * We restore the state to UIST_NEW_DEVICE because the user has to call
>> +      * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the
>> +      * validity of the absbits.
>> +      */
>
> Do we have to be this strict? It seems to me that with the new IOCTLs
> you naturally want to use the new ioctl to setup the device, then adjust
> various axes and bits and then validate everything.

Indeed, we now force the order to be abs-setup first, then
device-setup as last step. Appended is a follow-up patch to cleanup
ABS handling in uinput. It is untested. Benjamin, care to give it a
spin?

Thanks
David

----
>From 2568f83bcc5c4b8aeb149be2c5fc5a743812f0fe Mon Sep 17 00:00:00 2001
From: David Herrmann <dh.herrmann@gmail.com>
Date: Sun, 25 Oct 2015 10:34:13 +0100
Subject: [PATCH] Input: uinput - rework ABS validation

Rework the uinput ABS validation to check passed absinfo data
immediately, but do ABS initialization as last step in UI_DEV_CREATE. The
behavior observed by user-space is not changed, as ABS initialization was
never checked for errors.

With this in place, the order of device-initialization and
abs-configuration is no longer fixed. User-space can initialize the
device and afterwards set absinfo just fine.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/input/misc/uinput.c | 89 +++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 44 deletions(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index baac903..1d93037 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -256,13 +256,22 @@ static void uinput_destroy_device(...)
 static int uinput_create_device(struct uinput_device *udev)
 {
  struct input_dev *dev = udev->dev;
- int error;
+ int error, nslot;

  if (udev->state != UIST_SETUP_COMPLETE) {
  printk(KERN_DEBUG "%s: write device info first\n", UINPUT_NAME);
  return -EINVAL;
  }

+ if (test_bit(ABS_MT_SLOT, dev->absbit)) {
+ nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
+ error = input_mt_init_slots(dev, nslot, 0);
+ if (error)
+ goto fail1;
+ } else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
+ input_set_events_per_packet(dev, 60);
+ }
+
  if (udev->ff_effects_max) {
  error = input_ff_create(dev, udev->ff_effects_max);
  if (error)
@@ -308,10 +317,35 @@ static int uinput_open(...)
  return 0;
 }

+static int uinput_validate_absinfo(struct input_dev *dev, unsigned int code,
+   const struct input_absinfo *abs)
+{
+ int min, max;
+
+ min = abs->minimum;
+ max = abs->maximum;
+
+ if ((min != 0 || max != 0) && max <= min) {
+ printk(KERN_DEBUG
+       "%s: invalid abs[%02x] min:%d max:%d\n",
+       UINPUT_NAME, code, min, max);
+ return -EINVAL;
+ }
+
+ if (abs->flat > max - min) {
+ printk(KERN_DEBUG
+       "%s: abs_flat #%02x out of range: %d (min:%d/max:%d)\n",
+       UINPUT_NAME, code, abs->flat, min, max);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
 static int uinput_validate_absbits(struct input_dev *dev)
 {
  unsigned int cnt;
- int nslot;
+ int error;

  if (!test_bit(EV_ABS, dev->evbit))
  return 0;
@@ -321,38 +355,12 @@ static int uinput_validate_absbits(struct input_dev *dev)
  */

  for_each_set_bit(cnt, dev->absbit, ABS_CNT) {
- int min, max;
-
- min = input_abs_get_min(dev, cnt);
- max = input_abs_get_max(dev, cnt);
-
- if ((min != 0 || max != 0) && max <= min) {
- printk(KERN_DEBUG
- "%s: invalid abs[%02x] min:%d max:%d\n",
- UINPUT_NAME, cnt,
- input_abs_get_min(dev, cnt),
- input_abs_get_max(dev, cnt));
+ if (!dev->absinfo)
  return -EINVAL;
- }
-
- if (input_abs_get_flat(dev, cnt) >
-    input_abs_get_max(dev, cnt) - input_abs_get_min(dev, cnt)) {
- printk(KERN_DEBUG
- "%s: abs_flat #%02x out of range: %d "
- "(min:%d/max:%d)\n",
- UINPUT_NAME, cnt,
- input_abs_get_flat(dev, cnt),
- input_abs_get_min(dev, cnt),
- input_abs_get_max(dev, cnt));
- return -EINVAL;
- }
- }

- if (test_bit(ABS_MT_SLOT, dev->absbit)) {
- nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
- input_mt_init_slots(dev, nslot, 0);
- } else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
- input_set_events_per_packet(dev, 60);
+ error = uinput_validate_absinfo(dev, cnt, &dev->absinfo[cnt]);
+ if (error)
+ return error;
  }

  return 0;
@@ -375,7 +383,6 @@ static int uinput_dev_setup(struct uinput_device *udev,
 {
  struct uinput_setup setup;
  struct input_dev *dev;
- int retval;

  if (udev->state == UIST_CREATED)
  return -EINVAL;
@@ -393,10 +400,6 @@ static int uinput_dev_setup(struct uinput_device *udev,
  if (!dev->name)
  return -ENOMEM;

- retval = uinput_validate_absbits(dev);
- if (retval < 0)
- return retval;
-
  udev->state = UIST_SETUP_COMPLETE;
  return 0;
 }
@@ -406,6 +409,7 @@ static int uinput_abs_setup(struct uinput_device *udev,
 {
  struct uinput_abs_setup setup = {};
  struct input_dev *dev;
+ int error;

  if (size > sizeof(setup))
  return -E2BIG;
@@ -418,19 +422,16 @@ static int uinput_abs_setup(struct uinput_device *udev,

  dev = udev->dev;

+ error = uinput_validate_absinfo(dev, setup.code, &setup.absinfo);
+ if (error)
+ return error;
+
  input_alloc_absinfo(dev);
  if (!dev->absinfo)
  return -ENOMEM;

  set_bit(setup.code, dev->absbit);
  dev->absinfo[setup.code] = setup.absinfo;
-
- /*
- * We restore the state to UIST_NEW_DEVICE because the user has to call
- * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the
- * validity of the absbits.
- */
- udev->state = UIST_NEW_DEVICE;
  return 0;
 }

-- 
2.6.1

[-- Attachment #2: 0001-Input-uinput-rework-ABS-validation.patch --]
[-- Type: text/x-patch, Size: 4943 bytes --]

From 2568f83bcc5c4b8aeb149be2c5fc5a743812f0fe Mon Sep 17 00:00:00 2001
From: David Herrmann <dh.herrmann@gmail.com>
Date: Sun, 25 Oct 2015 10:34:13 +0100
Subject: [PATCH] Input: uinput - rework ABS validation

Rework the uinput ABS validation to check passed absinfo data
immediately, but do ABS initialization as last step in UI_DEV_CREATE. The
behavior observed by user-space is not changed, as ABS initialization was
never checked for errors.

With this in place, the order of device-initialization and
abs-configuration is no longer fixed. User-space can initialize the
device and afterwards set absinfo just fine.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/input/misc/uinput.c | 89 +++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 44 deletions(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index baac903..1d93037 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -256,13 +256,22 @@ static void uinput_destroy_device(...)
 static int uinput_create_device(struct uinput_device *udev)
 {
 	struct input_dev *dev = udev->dev;
-	int error;
+	int error, nslot;
 
 	if (udev->state != UIST_SETUP_COMPLETE) {
 		printk(KERN_DEBUG "%s: write device info first\n", UINPUT_NAME);
 		return -EINVAL;
 	}
 
+	if (test_bit(ABS_MT_SLOT, dev->absbit)) {
+		nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
+		error = input_mt_init_slots(dev, nslot, 0);
+		if (error)
+			goto fail1;
+	} else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
+		input_set_events_per_packet(dev, 60);
+	}
+
 	if (udev->ff_effects_max) {
 		error = input_ff_create(dev, udev->ff_effects_max);
 		if (error)
@@ -308,10 +317,35 @@ static int uinput_open(...)
 	return 0;
 }
 
+static int uinput_validate_absinfo(struct input_dev *dev, unsigned int code,
+				   const struct input_absinfo *abs)
+{
+	int min, max;
+
+	min = abs->minimum;
+	max = abs->maximum;
+
+	if ((min != 0 || max != 0) && max <= min) {
+		printk(KERN_DEBUG
+		       "%s: invalid abs[%02x] min:%d max:%d\n",
+		       UINPUT_NAME, code, min, max);
+		return -EINVAL;
+	}
+
+	if (abs->flat > max - min) {
+		printk(KERN_DEBUG
+		       "%s: abs_flat #%02x out of range: %d (min:%d/max:%d)\n",
+		       UINPUT_NAME, code, abs->flat, min, max);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int uinput_validate_absbits(struct input_dev *dev)
 {
 	unsigned int cnt;
-	int nslot;
+	int error;
 
 	if (!test_bit(EV_ABS, dev->evbit))
 		return 0;
@@ -321,38 +355,12 @@ static int uinput_validate_absbits(struct input_dev *dev)
 	 */
 
 	for_each_set_bit(cnt, dev->absbit, ABS_CNT) {
-		int min, max;
-
-		min = input_abs_get_min(dev, cnt);
-		max = input_abs_get_max(dev, cnt);
-
-		if ((min != 0 || max != 0) && max <= min) {
-			printk(KERN_DEBUG
-				"%s: invalid abs[%02x] min:%d max:%d\n",
-				UINPUT_NAME, cnt,
-				input_abs_get_min(dev, cnt),
-				input_abs_get_max(dev, cnt));
+		if (!dev->absinfo)
 			return -EINVAL;
-		}
-
-		if (input_abs_get_flat(dev, cnt) >
-		    input_abs_get_max(dev, cnt) - input_abs_get_min(dev, cnt)) {
-			printk(KERN_DEBUG
-				"%s: abs_flat #%02x out of range: %d "
-				"(min:%d/max:%d)\n",
-				UINPUT_NAME, cnt,
-				input_abs_get_flat(dev, cnt),
-				input_abs_get_min(dev, cnt),
-				input_abs_get_max(dev, cnt));
-			return -EINVAL;
-		}
-	}
 
-	if (test_bit(ABS_MT_SLOT, dev->absbit)) {
-		nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
-		input_mt_init_slots(dev, nslot, 0);
-	} else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
-		input_set_events_per_packet(dev, 60);
+		error = uinput_validate_absinfo(dev, cnt, &dev->absinfo[cnt]);
+		if (error)
+			return error;
 	}
 
 	return 0;
@@ -375,7 +383,6 @@ static int uinput_dev_setup(struct uinput_device *udev,
 {
 	struct uinput_setup setup;
 	struct input_dev *dev;
-	int retval;
 
 	if (udev->state == UIST_CREATED)
 		return -EINVAL;
@@ -393,10 +400,6 @@ static int uinput_dev_setup(struct uinput_device *udev,
 	if (!dev->name)
 		return -ENOMEM;
 
-	retval = uinput_validate_absbits(dev);
-	if (retval < 0)
-		return retval;
-
 	udev->state = UIST_SETUP_COMPLETE;
 	return 0;
 }
@@ -406,6 +409,7 @@ static int uinput_abs_setup(struct uinput_device *udev,
 {
 	struct uinput_abs_setup setup = {};
 	struct input_dev *dev;
+	int error;
 
 	if (size > sizeof(setup))
 		return -E2BIG;
@@ -418,19 +422,16 @@ static int uinput_abs_setup(struct uinput_device *udev,
 
 	dev = udev->dev;
 
+	error = uinput_validate_absinfo(dev, setup.code, &setup.absinfo);
+	if (error)
+		return error;
+
 	input_alloc_absinfo(dev);
 	if (!dev->absinfo)
 		return -ENOMEM;
 
 	set_bit(setup.code, dev->absbit);
 	dev->absinfo[setup.code] = setup.absinfo;
-
-	/*
-	 * We restore the state to UIST_NEW_DEVICE because the user has to call
-	 * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the
-	 * validity of the absbits.
-	 */
-	udev->state = UIST_NEW_DEVICE;
 	return 0;
 }
 
-- 
2.6.1

Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl

[Benjamin Tissoires]

Hi,

On Oct 25 2015 or thereabouts, David Herrmann wrote:
> Hi
> 
> On Sun, Oct 25, 2015 at 12:53 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi Benjamin,
> >
> > On Tue, Aug 25, 2015 at 11:12:59AM -0400, Benjamin Tissoires wrote:
> >> +static int uinput_abs_setup(struct uinput_device *udev,
> >> +                         struct uinput_setup __user *arg, size_t size)
> >> +{
> >> +     struct uinput_abs_setup setup = {};
> >> +     struct input_dev *dev;
> >> +
> >> +     if (size > sizeof(setup))
> >> +             return -E2BIG;
> >> +     if (udev->state == UIST_CREATED)
> >> +             return -EINVAL;
> >> +     if (copy_from_user(&setup, arg, size))
> >> +             return -EFAULT;
> >> +     if (setup.code > ABS_MAX)
> >> +             return -ERANGE;
> >> +
> >> +     dev = udev->dev;
> >> +
> >> +     input_alloc_absinfo(dev);
> >> +     if (!dev->absinfo)
> >> +             return -ENOMEM;
> >> +
> >> +     set_bit(setup.code, dev->absbit);
> >> +     dev->absinfo[setup.code] = setup.absinfo;
> >> +
> >> +     /*
> >> +      * We restore the state to UIST_NEW_DEVICE because the user has to call
> >> +      * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the
> >> +      * validity of the absbits.
> >> +      */
> >
> > Do we have to be this strict? It seems to me that with the new IOCTLs
> > you naturally want to use the new ioctl to setup the device, then adjust
> > various axes and bits and then validate everything.
> 
> Indeed, we now force the order to be abs-setup first, then
> device-setup as last step. Appended is a follow-up patch to cleanup
> ABS handling in uinput. It is untested. Benjamin, care to give it a
> spin?
> 

Sorry it took so long for the tests/review.

So the test part is fine. It works as expected. (Tested-by: BT
<benjamin.tissoires@redhat.com>)

For the review part, everything looks good except that now the doc for
UI_ABS_SETUP in uapi/linux/uinput.h should be changed to reflect the
fact that UI_DEV_SETUP is not a pre-requirement before calling
UI_ABS_SETUP.

With the doc change, the patch is also Reviewed-by: me.

Cheers,
Benjamin

> Thanks
> David
> 
> ----
> From 2568f83bcc5c4b8aeb149be2c5fc5a743812f0fe Mon Sep 17 00:00:00 2001
> From: David Herrmann <dh.herrmann@gmail.com>
> Date: Sun, 25 Oct 2015 10:34:13 +0100
> Subject: [PATCH] Input: uinput - rework ABS validation
> 
> Rework the uinput ABS validation to check passed absinfo data
> immediately, but do ABS initialization as last step in UI_DEV_CREATE. The
> behavior observed by user-space is not changed, as ABS initialization was
> never checked for errors.
> 
> With this in place, the order of device-initialization and
> abs-configuration is no longer fixed. User-space can initialize the
> device and afterwards set absinfo just fine.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/input/misc/uinput.c | 89 +++++++++++++++++++++++----------------------
>  1 file changed, 45 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index baac903..1d93037 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -256,13 +256,22 @@ static void uinput_destroy_device(...)
>  static int uinput_create_device(struct uinput_device *udev)
>  {
>   struct input_dev *dev = udev->dev;
> - int error;
> + int error, nslot;
> 
>   if (udev->state != UIST_SETUP_COMPLETE) {
>   printk(KERN_DEBUG "%s: write device info first\n", UINPUT_NAME);
>   return -EINVAL;
>   }
> 
> + if (test_bit(ABS_MT_SLOT, dev->absbit)) {
> + nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
> + error = input_mt_init_slots(dev, nslot, 0);
> + if (error)
> + goto fail1;
> + } else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
> + input_set_events_per_packet(dev, 60);
> + }
> +
>   if (udev->ff_effects_max) {
>   error = input_ff_create(dev, udev->ff_effects_max);
>   if (error)
> @@ -308,10 +317,35 @@ static int uinput_open(...)
>   return 0;
>  }
> 
> +static int uinput_validate_absinfo(struct input_dev *dev, unsigned int code,
> +   const struct input_absinfo *abs)
> +{
> + int min, max;
> +
> + min = abs->minimum;
> + max = abs->maximum;
> +
> + if ((min != 0 || max != 0) && max <= min) {
> + printk(KERN_DEBUG
> +       "%s: invalid abs[%02x] min:%d max:%d\n",
> +       UINPUT_NAME, code, min, max);
> + return -EINVAL;
> + }
> +
> + if (abs->flat > max - min) {
> + printk(KERN_DEBUG
> +       "%s: abs_flat #%02x out of range: %d (min:%d/max:%d)\n",
> +       UINPUT_NAME, code, abs->flat, min, max);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
>  static int uinput_validate_absbits(struct input_dev *dev)
>  {
>   unsigned int cnt;
> - int nslot;
> + int error;
> 
>   if (!test_bit(EV_ABS, dev->evbit))
>   return 0;
> @@ -321,38 +355,12 @@ static int uinput_validate_absbits(struct input_dev *dev)
>   */
> 
>   for_each_set_bit(cnt, dev->absbit, ABS_CNT) {
> - int min, max;
> -
> - min = input_abs_get_min(dev, cnt);
> - max = input_abs_get_max(dev, cnt);
> -
> - if ((min != 0 || max != 0) && max <= min) {
> - printk(KERN_DEBUG
> - "%s: invalid abs[%02x] min:%d max:%d\n",
> - UINPUT_NAME, cnt,
> - input_abs_get_min(dev, cnt),
> - input_abs_get_max(dev, cnt));
> + if (!dev->absinfo)
>   return -EINVAL;
> - }
> -
> - if (input_abs_get_flat(dev, cnt) >
> -    input_abs_get_max(dev, cnt) - input_abs_get_min(dev, cnt)) {
> - printk(KERN_DEBUG
> - "%s: abs_flat #%02x out of range: %d "
> - "(min:%d/max:%d)\n",
> - UINPUT_NAME, cnt,
> - input_abs_get_flat(dev, cnt),
> - input_abs_get_min(dev, cnt),
> - input_abs_get_max(dev, cnt));
> - return -EINVAL;
> - }
> - }
> 
> - if (test_bit(ABS_MT_SLOT, dev->absbit)) {
> - nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
> - input_mt_init_slots(dev, nslot, 0);
> - } else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
> - input_set_events_per_packet(dev, 60);
> + error = uinput_validate_absinfo(dev, cnt, &dev->absinfo[cnt]);
> + if (error)
> + return error;
>   }
> 
>   return 0;
> @@ -375,7 +383,6 @@ static int uinput_dev_setup(struct uinput_device *udev,
>  {
>   struct uinput_setup setup;
>   struct input_dev *dev;
> - int retval;
> 
>   if (udev->state == UIST_CREATED)
>   return -EINVAL;
> @@ -393,10 +400,6 @@ static int uinput_dev_setup(struct uinput_device *udev,
>   if (!dev->name)
>   return -ENOMEM;
> 
> - retval = uinput_validate_absbits(dev);
> - if (retval < 0)
> - return retval;
> -
>   udev->state = UIST_SETUP_COMPLETE;
>   return 0;
>  }
> @@ -406,6 +409,7 @@ static int uinput_abs_setup(struct uinput_device *udev,
>  {
>   struct uinput_abs_setup setup = {};
>   struct input_dev *dev;
> + int error;
> 
>   if (size > sizeof(setup))
>   return -E2BIG;
> @@ -418,19 +422,16 @@ static int uinput_abs_setup(struct uinput_device *udev,
> 
>   dev = udev->dev;
> 
> + error = uinput_validate_absinfo(dev, setup.code, &setup.absinfo);
> + if (error)
> + return error;
> +
>   input_alloc_absinfo(dev);
>   if (!dev->absinfo)
>   return -ENOMEM;
> 
>   set_bit(setup.code, dev->absbit);
>   dev->absinfo[setup.code] = setup.absinfo;
> -
> - /*
> - * We restore the state to UIST_NEW_DEVICE because the user has to call
> - * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the
> - * validity of the absbits.
> - */
> - udev->state = UIST_NEW_DEVICE;
>   return 0;
>  }
> 
> -- 
> 2.6.1

> From 2568f83bcc5c4b8aeb149be2c5fc5a743812f0fe Mon Sep 17 00:00:00 2001
> From: David Herrmann <dh.herrmann@gmail.com>
> Date: Sun, 25 Oct 2015 10:34:13 +0100
> Subject: [PATCH] Input: uinput - rework ABS validation
> 
> Rework the uinput ABS validation to check passed absinfo data
> immediately, but do ABS initialization as last step in UI_DEV_CREATE. The
> behavior observed by user-space is not changed, as ABS initialization was
> never checked for errors.
> 
> With this in place, the order of device-initialization and
> abs-configuration is no longer fixed. User-space can initialize the
> device and afterwards set absinfo just fine.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/input/misc/uinput.c | 89 +++++++++++++++++++++++----------------------
>  1 file changed, 45 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index baac903..1d93037 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -256,13 +256,22 @@ static void uinput_destroy_device(...)
>  static int uinput_create_device(struct uinput_device *udev)
>  {
>  	struct input_dev *dev = udev->dev;
> -	int error;
> +	int error, nslot;
>  
>  	if (udev->state != UIST_SETUP_COMPLETE) {
>  		printk(KERN_DEBUG "%s: write device info first\n", UINPUT_NAME);
>  		return -EINVAL;
>  	}
>  
> +	if (test_bit(ABS_MT_SLOT, dev->absbit)) {
> +		nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
> +		error = input_mt_init_slots(dev, nslot, 0);
> +		if (error)
> +			goto fail1;
> +	} else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
> +		input_set_events_per_packet(dev, 60);
> +	}
> +
>  	if (udev->ff_effects_max) {
>  		error = input_ff_create(dev, udev->ff_effects_max);
>  		if (error)
> @@ -308,10 +317,35 @@ static int uinput_open(...)
>  	return 0;
>  }
>  
> +static int uinput_validate_absinfo(struct input_dev *dev, unsigned int code,
> +				   const struct input_absinfo *abs)
> +{
> +	int min, max;
> +
> +	min = abs->minimum;
> +	max = abs->maximum;
> +
> +	if ((min != 0 || max != 0) && max <= min) {
> +		printk(KERN_DEBUG
> +		       "%s: invalid abs[%02x] min:%d max:%d\n",
> +		       UINPUT_NAME, code, min, max);
> +		return -EINVAL;
> +	}
> +
> +	if (abs->flat > max - min) {
> +		printk(KERN_DEBUG
> +		       "%s: abs_flat #%02x out of range: %d (min:%d/max:%d)\n",
> +		       UINPUT_NAME, code, abs->flat, min, max);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int uinput_validate_absbits(struct input_dev *dev)
>  {
>  	unsigned int cnt;
> -	int nslot;
> +	int error;
>  
>  	if (!test_bit(EV_ABS, dev->evbit))
>  		return 0;
> @@ -321,38 +355,12 @@ static int uinput_validate_absbits(struct input_dev *dev)
>  	 */
>  
>  	for_each_set_bit(cnt, dev->absbit, ABS_CNT) {
> -		int min, max;
> -
> -		min = input_abs_get_min(dev, cnt);
> -		max = input_abs_get_max(dev, cnt);
> -
> -		if ((min != 0 || max != 0) && max <= min) {
> -			printk(KERN_DEBUG
> -				"%s: invalid abs[%02x] min:%d max:%d\n",
> -				UINPUT_NAME, cnt,
> -				input_abs_get_min(dev, cnt),
> -				input_abs_get_max(dev, cnt));
> +		if (!dev->absinfo)
>  			return -EINVAL;
> -		}
> -
> -		if (input_abs_get_flat(dev, cnt) >
> -		    input_abs_get_max(dev, cnt) - input_abs_get_min(dev, cnt)) {
> -			printk(KERN_DEBUG
> -				"%s: abs_flat #%02x out of range: %d "
> -				"(min:%d/max:%d)\n",
> -				UINPUT_NAME, cnt,
> -				input_abs_get_flat(dev, cnt),
> -				input_abs_get_min(dev, cnt),
> -				input_abs_get_max(dev, cnt));
> -			return -EINVAL;
> -		}
> -	}
>  
> -	if (test_bit(ABS_MT_SLOT, dev->absbit)) {
> -		nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
> -		input_mt_init_slots(dev, nslot, 0);
> -	} else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
> -		input_set_events_per_packet(dev, 60);
> +		error = uinput_validate_absinfo(dev, cnt, &dev->absinfo[cnt]);
> +		if (error)
> +			return error;
>  	}
>  
>  	return 0;
> @@ -375,7 +383,6 @@ static int uinput_dev_setup(struct uinput_device *udev,
>  {
>  	struct uinput_setup setup;
>  	struct input_dev *dev;
> -	int retval;
>  
>  	if (udev->state == UIST_CREATED)
>  		return -EINVAL;
> @@ -393,10 +400,6 @@ static int uinput_dev_setup(struct uinput_device *udev,
>  	if (!dev->name)
>  		return -ENOMEM;
>  
> -	retval = uinput_validate_absbits(dev);
> -	if (retval < 0)
> -		return retval;
> -
>  	udev->state = UIST_SETUP_COMPLETE;
>  	return 0;
>  }
> @@ -406,6 +409,7 @@ static int uinput_abs_setup(struct uinput_device *udev,
>  {
>  	struct uinput_abs_setup setup = {};
>  	struct input_dev *dev;
> +	int error;
>  
>  	if (size > sizeof(setup))
>  		return -E2BIG;
> @@ -418,19 +422,16 @@ static int uinput_abs_setup(struct uinput_device *udev,
>  
>  	dev = udev->dev;
>  
> +	error = uinput_validate_absinfo(dev, setup.code, &setup.absinfo);
> +	if (error)
> +		return error;
> +
>  	input_alloc_absinfo(dev);
>  	if (!dev->absinfo)
>  		return -ENOMEM;
>  
>  	set_bit(setup.code, dev->absbit);
>  	dev->absinfo[setup.code] = setup.absinfo;
> -
> -	/*
> -	 * We restore the state to UIST_NEW_DEVICE because the user has to call
> -	 * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the
> -	 * validity of the absbits.
> -	 */
> -	udev->state = UIST_NEW_DEVICE;
>  	return 0;
>  }
>  
> -- 
> 2.6.1
> 

Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl

[Elias Vanderstuyft]

Hi,

On Tue, Aug 25, 2015 at 5:12 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index 013c9d8..ef6c9f5 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -20,6 +20,11 @@
>   * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
>   *
>   * Changes/Revisions:
> + *     0.5     08/13/2015 (David Herrmann <dh.herrmann@gmail.com> &
> + *                         Benjamin Tissoires <benjamin.tissoires@redhat.com>)
> + *             - add UI_DEV_SETUP ioctl
> + *             - add UI_ABS_SETUP ioctl
> + *             - add UI_GET_VERSION ioctl
>   *     0.4     01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
>   *             - add UI_GET_SYSNAME ioctl
>   *     0.3     24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
> @@ -37,8 +42,8 @@
>  #include <linux/types.h>
>  #include <linux/input.h>
>
> -#define UINPUT_VERSION         4
> -
> +#define UINPUT_VERSION         5
> +#define UINPUT_MAX_NAME_SIZE   80
>
>  struct uinput_ff_upload {
>         __u32                   request_id;
> @@ -58,6 +63,79 @@ struct uinput_ff_erase {
>  #define UI_DEV_CREATE          _IO(UINPUT_IOCTL_BASE, 1)
>  #define UI_DEV_DESTROY         _IO(UINPUT_IOCTL_BASE, 2)
>
> +struct uinput_setup {
> +       struct input_id id;
> +       char name[UINPUT_MAX_NAME_SIZE];
> +       __u32 ff_effects_max;
> +};

Is there a reason to not follow the same field order as in struct
uinput_user_dev?
I.e., why not:

struct uinput_setup {
        char name[UINPUT_MAX_NAME_SIZE];
        struct input_id id;
        __u32 ff_effects_max;
};

In case you would change this, also make sure to change the order in
the documentation of UI_DEV_SETUP.

Cheers,
Elias

Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl

[Benjamin Tissoires]

----- Original Message -----
> From: "Elias Vanderstuyft" <elias.vds@gmail.com>
> To: "Benjamin Tissoires" <benjamin.tissoires@redhat.com>
> Cc: "Dmitry Torokhov" <dmitry.torokhov@gmail.com>, "David Herrmann" <dh.herrmann@gmail.com>, "Peter Hutterer"
> <peter.hutterer@who-t.net>, "open list:HID CORE LAYER" <linux-input@vger.kernel.org>, linux-kernel@vger.kernel.org
> Sent: Sunday, November 8, 2015 11:55:04 AM
> Subject: Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl
> 
> Hi,
> 
> On Tue, Aug 25, 2015 at 5:12 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > index 013c9d8..ef6c9f5 100644
> > --- a/include/uapi/linux/uinput.h
> > +++ b/include/uapi/linux/uinput.h
> > @@ -20,6 +20,11 @@
> >   * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
> >   *
> >   * Changes/Revisions:
> > + *     0.5     08/13/2015 (David Herrmann <dh.herrmann@gmail.com> &
> > + *                         Benjamin Tissoires
> > <benjamin.tissoires@redhat.com>)
> > + *             - add UI_DEV_SETUP ioctl
> > + *             - add UI_ABS_SETUP ioctl
> > + *             - add UI_GET_VERSION ioctl
> >   *     0.4     01/09/2014 (Benjamin Tissoires
> >   <benjamin.tissoires@redhat.com>)
> >   *             - add UI_GET_SYSNAME ioctl
> >   *     0.3     24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
> > @@ -37,8 +42,8 @@
> >  #include <linux/types.h>
> >  #include <linux/input.h>
> >
> > -#define UINPUT_VERSION         4
> > -
> > +#define UINPUT_VERSION         5
> > +#define UINPUT_MAX_NAME_SIZE   80
> >
> >  struct uinput_ff_upload {
> >         __u32                   request_id;
> > @@ -58,6 +63,79 @@ struct uinput_ff_erase {
> >  #define UI_DEV_CREATE          _IO(UINPUT_IOCTL_BASE, 1)
> >  #define UI_DEV_DESTROY         _IO(UINPUT_IOCTL_BASE, 2)
> >
> > +struct uinput_setup {
> > +       struct input_id id;
> > +       char name[UINPUT_MAX_NAME_SIZE];
> > +       __u32 ff_effects_max;
> > +};
> 
> Is there a reason to not follow the same field order as in struct
> uinput_user_dev?

Not really, no.

> I.e., why not:
> 
> struct uinput_setup {
>         char name[UINPUT_MAX_NAME_SIZE];
>         struct input_id id;
>         __u32 ff_effects_max;
> };
> 
> In case you would change this, also make sure to change the order in
> the documentation of UI_DEV_SETUP.
> 

Works for me.
Dmitry, how do you want to handle this change? Me re-sending the whole series or you applying the change directly on your tree?

Cheers,
Benjamin

Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl

[Elias Vanderstuyft]

On Mon, Nov 9, 2015 at 8:50 AM, Benjamin Tissoires <btissoir@redhat.com> wrote:
>
> ----- Original Message -----
>> From: "Elias Vanderstuyft" <elias.vds@gmail.com>
>> To: "Benjamin Tissoires" <benjamin.tissoires@redhat.com>
>> Cc: "Dmitry Torokhov" <dmitry.torokhov@gmail.com>, "David Herrmann" <dh.herrmann@gmail.com>, "Peter Hutterer"
>> <peter.hutterer@who-t.net>, "open list:HID CORE LAYER" <linux-input@vger.kernel.org>, linux-kernel@vger.kernel.org
>> Sent: Sunday, November 8, 2015 11:55:04 AM
>> Subject: Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl
>>
>> I.e., why not:
>>
>> struct uinput_setup {
>>         char name[UINPUT_MAX_NAME_SIZE];
>>         struct input_id id;
>>         __u32 ff_effects_max;
>> };
>>
>> In case you would change this, also make sure to change the order in
>> the documentation of UI_DEV_SETUP.
>>
>
> Works for me.
> Dmitry, how do you want to handle this change? Me re-sending the whole series or you applying the change directly on your tree?

If this is too much of a hassle, please leave it as it was, I was just
being pedantic ;-)

Cheers,
Elias

Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl

[Dmitry Torokhov]

On Wed, Oct 28, 2015 at 11:10:06AM -0400, Benjamin Tissoires wrote:
> Hi,
> 
> On Oct 25 2015 or thereabouts, David Herrmann wrote:
> > Hi
> > 
> > On Sun, Oct 25, 2015 at 12:53 AM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > Hi Benjamin,
> > >
> > > On Tue, Aug 25, 2015 at 11:12:59AM -0400, Benjamin Tissoires wrote:
> > >> +static int uinput_abs_setup(struct uinput_device *udev,
> > >> +                         struct uinput_setup __user *arg, size_t size)
> > >> +{
> > >> +     struct uinput_abs_setup setup = {};
> > >> +     struct input_dev *dev;
> > >> +
> > >> +     if (size > sizeof(setup))
> > >> +             return -E2BIG;
> > >> +     if (udev->state == UIST_CREATED)
> > >> +             return -EINVAL;
> > >> +     if (copy_from_user(&setup, arg, size))
> > >> +             return -EFAULT;
> > >> +     if (setup.code > ABS_MAX)
> > >> +             return -ERANGE;
> > >> +
> > >> +     dev = udev->dev;
> > >> +
> > >> +     input_alloc_absinfo(dev);
> > >> +     if (!dev->absinfo)
> > >> +             return -ENOMEM;
> > >> +
> > >> +     set_bit(setup.code, dev->absbit);
> > >> +     dev->absinfo[setup.code] = setup.absinfo;
> > >> +
> > >> +     /*
> > >> +      * We restore the state to UIST_NEW_DEVICE because the user has to call
> > >> +      * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the
> > >> +      * validity of the absbits.
> > >> +      */
> > >
> > > Do we have to be this strict? It seems to me that with the new IOCTLs
> > > you naturally want to use the new ioctl to setup the device, then adjust
> > > various axes and bits and then validate everything.
> > 
> > Indeed, we now force the order to be abs-setup first, then
> > device-setup as last step. Appended is a follow-up patch to cleanup
> > ABS handling in uinput. It is untested. Benjamin, care to give it a
> > spin?
> > 
> 
> Sorry it took so long for the tests/review.
> 
> So the test part is fine. It works as expected. (Tested-by: BT
> <benjamin.tissoires@redhat.com>)
> 
> For the review part, everything looks good except that now the doc for
> UI_ABS_SETUP in uapi/linux/uinput.h should be changed to reflect the
> fact that UI_DEV_SETUP is not a pre-requirement before calling
> UI_ABS_SETUP.
> 
> With the doc change, the patch is also Reviewed-by: me.

OK, I applied the original and also adjusted the docs and applied this
one as a separate patch.

Thanks.

-- 
Dmitry