Re: [PATCH v2] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl
From: David Herrmann
Date: Mon Aug 24 2015 - 05:36:29 EST
Hi
On Fri, Aug 21, 2015 at 4:36 PM, Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> ---
>
> Hi,
>
> this is the v2 of the patch David submitted last year. I tried to take
> Dmitry's remarks into account and came out with this.
>
> Cheers,
> Benjamin
>
> drivers/input/misc/uinput.c | 83 +++++++++++++++++++++++++++++++++++++++++++--
> include/linux/uinput.h | 5 +++
> include/uapi/linux/uinput.h | 83 +++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 165 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 345df9b..afbcc41 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -370,8 +370,76 @@ 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;
> +
> + memset(&setup, 0, sizeof(setup));
This could be simplified as:
struct uinput_abs_setup setup = {};
> +
> + if (size > sizeof(setup))
> + return -EINVAL;
Maybe use something more specific here. This is something user-space
may want to match for if the ioctl struct gets extended. 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);
> +
If you do a second round of this patch, maybe drop that newline?
> + 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 +542,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 +803,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 +953,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@xxxxxxxxxxxxxxxxx>
> *
> * Changes/Revisions:
> + * 0.5 08/13/2015 (David Herrmann <dh.herrmann@xxxxxxxxx> &
> + * Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>)
> + * - add UI_DEV_SETUP ioctl
> + * - add UI_ABS_SETUP ioctl
> + * - add UI_GET_VERSION ioctl
> * 0.4 01/09/2014 (Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>)
> * - 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@xxxxxxxxxxxxxxxxx>
> *
> * Changes/Revisions:
> + * 0.5 08/13/2015 (David Herrmann <dh.herrmann@xxxxxxxxx> &
> + * Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>)
> + * - add UI_DEV_SETUP ioctl
> + * - add UI_ABS_SETUP ioctl
> + * - add UI_GET_VERSION ioctl
> * 0.4 01/09/2014 (Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>)
> * - 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;
Looks good to me! Thanks for picking it up! Feel free to append:
Reviewed-by: David Herrmann <dh.herrmann@xxxxxxxxx>
Thanks
David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/