Re: [PATCH RESEND 4/5] Input: uinput - add new UINPUT_DEV_SETUP ioctl

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi David,

On Sat, Jul 19, 2014 at 03:10:44PM +0200, David Herrmann wrote:
> This adds a new ioctl UINPUT_DEV_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 a new ioctl to replace the old method. The ioctl
> supports 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.
> 
> The ioctl also allows user-space to skip unknown axes if not set. The
> payload-size can now be specified by the caller. There is no need to copy
> the whole array temporarily into the kernel, but instead we can iterate
> over it and copy each value manually.
> 
> Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/input/misc/uinput.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
>  include/uapi/linux/uinput.h | 55 ++++++++++++++++++++++++++++++++++--
>  2 files changed, 118 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index a2a3895..0f45595 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -371,8 +371,67 @@ 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 i, retval;
> +
> +	if (udev->state == UIST_CREATED)
> +		return -EINVAL;
> +	if (copy_from_user(&setup, arg, sizeof(setup)))
> +		return -EFAULT;
> +	if (!setup.name[0])
> +		return -EINVAL;
> +	/* So far we only support the original "struct input_absinfo", but be
> +	 * forward compatible and allow larger payloads. */
> +	if (setup.absinfo_size < sizeof(struct input_absinfo))
> +		return -EINVAL;

No, we can not do this, as it breaks backward compatibility (the most
important one!). If we were to increase size of in-kernel input_absinfo
in let's say 3.20, userspace compiled against older kernel headers
(but using the new ioctl available let's say since 3.16 - don't hold me
to the numbers ;) ), would break since it wold start tripping on thi
check.

The proper way to handle it is to convert "old" absinfo into new one,
applying as much as we can.

Thanks.

-- 
Dmitry