[PATCH] evdev - relax checks when reopening devices

[Peter Hutterer]

On Mon, Nov 02, 2009 at 11:11:55PM -0800, Dmitry Torokhov wrote:
> Evdev is way too paranoid when checking whether it deals with the same
> device when switching consoles which causes people lose keyboards if
> they happen to adjust keymap after X starts. PLease consider applying
> the patch below.

Thanks for the patch Dmitry. I certainly agree with the principle but I do
have a few minor comments.

> From: Dmitry Torokhov <dmitry.torokhov at gmail.com>
> Subject: [PATCH] evdev - relax checks when reopening devices
> 
> When checking whether we are dealing with the same device as before
> when we try to reopen it evdev should not require exact match of
> entire keymap. Users should be allowed to adjust keymaps to better
> match their hardware even after X starts. However we don't expect
> changes in [BTN_MISC, KEY_OK) range since these codes are reserved for
> mice, joysticks, tablets and so forth, so we will limit the check to
> this range.
> 
> The same goes for absinfo - limits can change and it should not result
> in device being disabled.
> 
> Also check the length of the data returned by ioctl and don't try to
> compare more than we were given.
> 
> Signed-off-by: Dmitry Torokhov <dtor at mail.ru>
> ---
>  src/evdev.c |  126 +++++++++++++++++++++++++++++++++--------------------------
>  1 files changed, 71 insertions(+), 55 deletions(-)
> 
> diff --git a/src/evdev.c b/src/evdev.c
> index 0dff271..747f3b1 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -1671,6 +1671,7 @@ static int
>  EvdevCacheCompare(InputInfoPtr pInfo, BOOL compare)
>  {
>      EvdevPtr pEvdev = pInfo->private;
> +    size_t len;
>      int i;
>  
>      char name[1024]                  = {0};
> @@ -1679,107 +1680,122 @@ EvdevCacheCompare(InputInfoPtr pInfo, BOOL compare)
>      unsigned long rel_bitmask[NLONGS(REL_CNT)] = {0};
>      unsigned long abs_bitmask[NLONGS(ABS_CNT)] = {0};
>      unsigned long led_bitmask[NLONGS(LED_CNT)] = {0};
> -    struct input_absinfo absinfo[ABS_CNT];
>  
> -    if (ioctl(pInfo->fd,
> -              EVIOCGNAME(sizeof(name) - 1), name) < 0) {
> +    if (ioctl(pInfo->fd, EVIOCGNAME(sizeof(name) - 1), name) < 0) {

this line is white-space only, no?

>          xf86Msg(X_ERROR, "ioctl EVIOCGNAME failed: %s\n", strerror(errno));
>          goto error;
>      }
>  
[...]
  
> -    if (ioctl(pInfo->fd,
> -              EVIOCGBIT(EV_KEY, sizeof(key_bitmask)), key_bitmask) < 0) {
> -        xf86Msg(X_ERROR, "%s: ioctl EVIOCGBIT failed: %s\n", pInfo->name, strerror(errno));
> +    len = ioctl(pInfo->fd, EVIOCGBIT(EV_KEY, sizeof(key_bitmask)), key_bitmask);
> +    if (len < 0) {
> +        xf86Msg(X_ERROR, "%s: ioctl EVIOCGBIT failed: %s\n",
> +                pInfo->name, strerror(errno));
>          goto error;
>      }
>  
> -    if (compare && memcmp(pEvdev->key_bitmask, key_bitmask, sizeof(key_bitmask))) {
> -        xf86Msg(X_ERROR, "%s: device key_bitmask has changed\n", pInfo->name);
> -        goto error;
> +    if (compare) {
> +        /*
> +         * Keys are special as user can adjust keymap at any time (on
> +         * devices that support EVIOCSKEYCODE. However we do not expect
> +         * buttons reserved fir mice/tablets/digitizers and so on to

typo: "for"

> +         * appear/disappear so we will check only those in
> +         * [BTN_MISC, KEY_OK) range.
> +         */
> +        size_t start_word = BTN_MISC / LONG_BITS;
> +        size_t start_byte = start_word * sizeof(unsigned long);
> +        size_t end_word = KEY_OK / LONG_BITS;
> +        size_t end_byte = end_word * sizeof(unsigned long);
> +
> +        if (len >= start_byte &&
> +            memcmp(&pEvdev->key_bitmask[start_word], &key_bitmask[start_word],
> +                   min(len, end_byte) - start_byte + 1)) {
> +            xf86Msg(X_ERROR, "%s: device key_bitmask has changed\n", pInfo->name);
> +            goto error;
> +        }
>      }
>  
> -    if (ioctl(pInfo->fd,
> -              EVIOCGBIT(EV_LED, sizeof(led_bitmask)), led_bitmask) < 0) {
> -        xf86Msg(X_ERROR, "%s: ioctl EVIOCGBIT failed: %s\n", pInfo->name, strerror(errno));
> +    /* Copy the data so we have reasonably up-to-date info */
> +    memcpy(pEvdev->key_bitmask, key_bitmask, len);

I think we should move this whole keyinfo bit below the absinfo stuff
before. Otherwise there is a narrow case where the key info is updated but
the compare still then still fail because something else has changed.

Other than that, I'm happy with the patch. Eric - have you managed to test
this patch yet?

Cheers,
  Peter

> +
> +    len = ioctl(pInfo->fd, EVIOCGBIT(EV_LED, sizeof(led_bitmask)), led_bitmask);
> +    if (len < 0) {
> +        xf86Msg(X_ERROR, "%s: ioctl EVIOCGBIT failed: %s\n",
> +                pInfo->name, strerror(errno));
>          goto error;
>      }
>  
> -    if (compare && memcmp(pEvdev->led_bitmask, led_bitmask, sizeof(led_bitmask))) {
> +    if (!compare) {
> +        memcpy(pEvdev->led_bitmask, led_bitmask, len);
> +    } else if (memcmp(pEvdev->led_bitmask, led_bitmask, len)) {
>          xf86Msg(X_ERROR, "%s: device led_bitmask has changed\n", pInfo->name);
>          goto error;
>      }

> -    memset(absinfo, 0, sizeof(absinfo));
> -
> -    for (i = ABS_X; i <= ABS_MAX; i++)
> -    {
> -        if (TestBit(i, abs_bitmask))
> -        {
> -            if (ioctl(pInfo->fd, EVIOCGABS(i), &absinfo[i]) < 0) {
> -                xf86Msg(X_ERROR, "%s: ioctl EVIOCGABS failed: %s\n", pInfo->name, strerror(errno));
> +    /*
> +     * Do not try to validate absinfo data since it is not expected
> +     * to be static, always refresh it in evdev structure.
> +     */
> +    for (i = ABS_X; i <= ABS_MAX; i++) {
> +        if (TestBit(i, abs_bitmask)) {
> +            len = ioctl(pInfo->fd, EVIOCGABS(i), &pEvdev->absinfo[i]);
> +            if (len < 0) {
> +                xf86Msg(X_ERROR, "%s: ioctl EVIOCGABSi(%d) failed: %s\n",
> +                        pInfo->name, i, strerror(errno));
>                  goto error;
>              }
> -            /* ignore current position (value) in comparison (bug #19819) */
> -            absinfo[i].value = pEvdev->absinfo[i].value;
>          }
>      }
>  
> -    if (compare && memcmp(pEvdev->absinfo, absinfo, sizeof(absinfo))) {
> -        xf86Msg(X_ERROR, "%s: device absinfo has changed\n", pInfo->name);
> -        goto error;
> -    }
> -
> -    /* cache info */
> -    if (!compare)
> -    {
> -        strcpy(pEvdev->name, name);
> -        memcpy(pEvdev->bitmask, bitmask, sizeof(bitmask));
> -        memcpy(pEvdev->key_bitmask, key_bitmask, sizeof(key_bitmask));
> -        memcpy(pEvdev->rel_bitmask, rel_bitmask, sizeof(rel_bitmask));
> -        memcpy(pEvdev->abs_bitmask, abs_bitmask, sizeof(abs_bitmask));
> -        memcpy(pEvdev->led_bitmask, led_bitmask, sizeof(led_bitmask));
> -        memcpy(pEvdev->absinfo, absinfo, sizeof(absinfo));
> -    }
> -
>      return Success;
>  
>  error: