| Message ID | 1405775445-4454-5-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
Hi On Mon, Jul 21, 2014 at 3:01 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > 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. 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 -- 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
On Mon, Jul 21, 2014 at 08:22:09AM +0200, David Herrmann wrote: > Hi > > On Mon, Jul 21, 2014 at 3:01 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > 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. > > 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. I think for now I would do: /* * Whoever is changing struct input_absinfo will have to take * care of backwards compatibility. */ BUILD_BUG_ON(sizeof(struct input_absinfo)) != 24); if (setup.absinfo_size != sizeof(struct input_absinfo)) return -EINVAL; ... and later when we detect setup.absinfo_size < sizeof(struct input_absinfo) we'll have to take care about backwards compatibility. We do not need to take care of forward compatibility as we do not know if userspace will be satisfied with partial results or not and newer userspace can deal with proper handling of older kernels. By the way, I realize I do not like the new IOCTL as it is - it's too big and would be hard to extend if we want to change items other than absinfo. Why don't we create UI_DEV_SETUP that only sets name, id, and number of effects, and add UI_SET_ABSAXIS that would take: struct uinput_abs_setup { __u16 code; /* axis code */ /* __u16 filler; */ struct input_absinfo absinfo; } By the way, while you are hacking on uinput can we also fix formatting style of switch/case and switch printk() to pr_debug() and friends? I'd do it myself but do not want to step on your toes. Thanks!
Hi On Mon, Jul 21, 2014 at 10:11 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Mon, Jul 21, 2014 at 08:22:09AM +0200, David Herrmann wrote: >> 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. > > I think for now I would do: > > /* > * Whoever is changing struct input_absinfo will have to take > * care of backwards compatibility. > */ > BUILD_BUG_ON(sizeof(struct input_absinfo)) != 24); > if (setup.absinfo_size != sizeof(struct input_absinfo)) > return -EINVAL; > > ... > > and later when we detect setup.absinfo_size < sizeof(struct > input_absinfo) we'll have to take care about backwards compatibility. We > do not need to take care of forward compatibility as we do not know if > userspace will be satisfied with partial results or not and newer > userspace can deal with proper handling of older kernels. I'm not sure I agree. I'm a fan of forward-compatibility, but that's probably a matter of taste. I will change my code accordingly. > By the way, I realize I do not like the new IOCTL as it is - it's too > big and would be hard to extend if we want to change items other than > absinfo. Why don't we create UI_DEV_SETUP that only sets name, id, and > number of effects, and add UI_SET_ABSAXIS that would take: > > struct uinput_abs_setup { > __u16 code; /* axis code */ > /* __u16 filler; */ > struct input_absinfo absinfo; > } Hm, that is actually a good idea. I will give it a try. > By the way, while you are hacking on uinput can we also fix formatting > style of switch/case and switch printk() to pr_debug() and friends? I'd > do it myself but do not want to step on your toes. Yepp, I will include those in the next revision. Thanks David -- 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
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; + + 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; + + if (setup.abs_cnt > ABS_CNT) + setup.abs_cnt = ABS_CNT; + + if (setup.abs_cnt > 0) { + u8 __user *p = (u8 __user*)arg->abs; + + input_alloc_absinfo(dev); + if (!dev->absinfo) + return -ENOMEM; + + for (i = 0; i < setup.abs_cnt; ++i, p += setup.absinfo_size) { + struct input_absinfo absinfo; + + if (!test_bit(i, dev->absbit)) + continue; + + if (copy_from_user(&absinfo, p, sizeof(absinfo))) + return -EFAULT; + + dev->absinfo[i] = absinfo; + } + } + + retval = uinput_validate_absbits(dev); + if (retval < 0) + return retval; + + udev->state = UIST_SETUP_COMPLETE; + 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; @@ -475,7 +534,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); @@ -736,6 +795,10 @@ 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; + case UI_SET_EVBIT: retval = uinput_set_bit(arg, evbit, EV_MAX); goto out; diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h index 19339cf..04a3876 100644 --- a/include/uapi/linux/uinput.h +++ b/include/uapi/linux/uinput.h @@ -20,6 +20,8 @@ * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org> * * Changes/Revisions: + * 0.5 06/20/2014 (David Herrmann <dh.herrmann@gmail.com> + * - add UI_DEV_SETUP 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 +39,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; @@ -93,6 +95,54 @@ struct uinput_ff_erase { */ #define UI_GET_VERSION _IOR(UINPUT_IOCTL_BASE, 301, unsigned int) +struct uinput_setup { + __u64 absinfo_size; + struct input_id id; + char name[UINPUT_MAX_NAME_SIZE]; + __u32 ff_effects_max; + __u32 abs_cnt; + struct input_absinfo abs[]; +}; + +/** + * 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(). + * + * This ioctl takes a "struct uinput_setup" object as argument. The fields of + * this object are as follows: + * absinfo_size: This field *must* be initialized to + * "sizeof(struct input_absinfo)". It allows to extend the API + * to support more absinfo fields. Older kernels ignore unknown + * extensions to "struct input_absinfo". + * 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. + * abs_cnt: This field defines the amount of elements in the following + * "abs" array. Axes beyond the kernel's ABS_CNT are ignored. + * abs: This is an appended array that contains parameters for ABS + * axes. See "struct input_absinfo" for a description of these + * fields. This array is copied unchanged into the kernel for + * all specified axes. Axes not enabled via UI_SET_ABSBIT are + * ignored. + * + * 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 _IOWR(UINPUT_IOCTL_BASE, 302, struct uinput_setup) + /* * To write a force-feedback-capable driver, the upload_effect * and erase_effect callbacks in input_dev must be implemented. @@ -144,7 +194,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;