Hi

On Mon, Jul 21, 2014 at 3:01 AM, Dmitry Torokhov
[off-list ref] wrote:

Hi David,

On Sat, Jul 19, 2014 at 03:10:44PM +0200, David Herrmann wrote:

quoted

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 <redacted>
Signed-off-by: David Herrmann <redacted>
---
 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.

I know, but there is no "old absinfo". Once we extend "struct absinfo"
I expect this code to change to:

{
        /* initially supported absinfo had size 24 */
        if (setup.absinfo_size < 24)
               return -EINVAL;

        /* ...pseudo code... */
        memset(&absinfo, 0, sizeof(absinfo));
        memcpy(&absinfo, sth->absinfo, MIN(setup.absinfo_size,
sizeof(absinfo)));
}

This allows you to use this ioctl with old absinfo objects. I can
change the current code to this already, if you want? I tried to avoid
it, because a memset() is not neccessarily an appropriate way to
initialize unset fields.
I cal also add support for "absinfo" without the "resolution" field,
which I think is the only field that wasn't available in the initial
structure.

Let me know which way you prefer.

Thanks
David