[PATCH] Input: evdev - add event-mask API

[PATCH] Input: evdev - add event-mask API

[David Herrmann]

Hardware manufacturers group keys in the weirdest way possible. This may
cause a power-key to be grouped together with normal keyboard keys and
thus be reported on the same kernel interface.

However, user-space is often only interested in specific sets of events.
For instance, daemons dealing with system-reboot (like systemd-logind)
listen for KEY_POWER, but are not interested in any main keyboard keys.
Usually, power keys are reported via separate interfaces, however,
some i8042 boards report it in the AT matrix. To avoid waking up those
system daemons on each key-press, we had two ideas:
 - split off KEY_POWER into a separate interface unconditionally
 - allow masking a specific set of events on evdev FDs

Splitting of KEY_POWER is a rather weird way to deal with this and may
break backwards-compatibility. It is also specific to KEY_POWER and might
be required for other stuff, too. Moreover, we might end up with a huge
set of input-devices just to have them properly split.

Hence, this patchset implements the second idea: An event-mask to specify
which events you're interested in. Two ioctls allow setting this mask for
each event-type. If not set, all events are reported. The type==0 entry is
used same as in EVIOCGBIT to set the actual EV_* mask of masked events.
This way, you have a two-level filter.

We are heavily forward-compatible to new event-types and event-codes. So
new user-space will be able to run on an old kernel which doesn't know the
given event-codes or event event-types.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/input/evdev.c      | 155 +++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/input.h |   8 +++
 2 files changed, 163 insertions(+)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 398648b..86778c3 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -51,10 +51,139 @@ struct evdev_client {
 	struct list_head node;
 	int clkid;
 	bool revoked;
+	unsigned long *evmasks[EV_CNT];
 	unsigned int bufsize;
 	struct input_event buffer[];
 };
 
+static size_t evdev_get_mask_cnt(unsigned int type)
+{
+	switch (type) {
+	case 0:
+		/* 0 is special (EV-bits instead of EV_SYN) like EVIOCGBIT */
+		return EV_CNT;
+	case EV_KEY:
+		return KEY_CNT;
+	case EV_REL:
+		return REL_CNT;
+	case EV_ABS:
+		return ABS_CNT;
+	case EV_MSC:
+		return MSC_CNT;
+	case EV_SW:
+		return SW_CNT;
+	case EV_LED:
+		return LED_CNT;
+	case EV_SND:
+		return SND_CNT;
+	case EV_FF:
+		return FF_CNT;
+	}
+
+	return 0;
+}
+
+/* must be called with evdev-mutex held */
+static int evdev_set_mask(struct evdev_client *client,
+			  unsigned int type,
+			  const void __user *codes,
+			  u32 codes_size)
+{
+	unsigned long flags, *mask, *oldmask;
+	size_t cnt, size;
+
+	/* unknown masks are simply ignored for forward-compat */
+	cnt = evdev_get_mask_cnt(type);
+	if (!cnt)
+		return 0;
+
+	/* we allow 'codes_size > size' for forward-compat */
+	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
+
+	mask = kzalloc(size, GFP_KERNEL);
+	if (!mask)
+		return -ENOMEM;
+
+	if (copy_from_user(mask, codes, min_t(size_t, codes_size, size))) {
+		kfree(mask);
+		return -EFAULT;
+	}
+
+	spin_lock_irqsave(&client->buffer_lock, flags);
+	oldmask = client->evmasks[type];
+	client->evmasks[type] = mask;
+	spin_unlock_irqrestore(&client->buffer_lock, flags);
+
+	kfree(oldmask);
+
+	return 0;
+}
+
+/* must be called with evdev-mutex held */
+static int evdev_get_mask(struct evdev_client *client,
+			  unsigned int type,
+			  void __user *codes,
+			  u32 codes_size)
+{
+	unsigned long *mask;
+	size_t cnt, size, min, i;
+	u8 __user *out;
+
+	/* we allow unknown types and 'codes_size > size' for forward-compat */
+	cnt = evdev_get_mask_cnt(type);
+	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
+	min = min_t(size_t, codes_size, size);
+
+	if (cnt > 0) {
+		mask = client->evmasks[type];
+		if (mask) {
+			if (copy_to_user(codes, mask, min))
+				return -EFAULT;
+		} else {
+			/* fake mask with all bits set */
+			out = (u8 __user*)codes;
+			for (i = 0; i < min; ++i) {
+				if (put_user((u8)0xff,  out + i))
+					return -EFAULT;
+			}
+		}
+	}
+
+	codes = (u8 __user*)codes + min;
+	codes_size -= min;
+
+	if (codes_size > 0 && clear_user(codes, codes_size))
+		return -EFAULT;
+
+	return 0;
+}
+
+/* requires the buffer lock to be held */
+static bool __evdev_is_masked(struct evdev_client *client,
+			      unsigned int type,
+			      unsigned int code)
+{
+	unsigned long *mask;
+	size_t cnt;
+
+	/* EV_SYN and unknown codes are never masked */
+	if (!type || type >= EV_CNT)
+		return false;
+
+	/* first test whether the type is masked */
+	mask = client->evmasks[0];
+	if (mask && !test_bit(type, mask))
+		return true;
+
+	/* unknown values are never masked */
+	cnt = evdev_get_mask_cnt(type);
+	if (!cnt || code >= cnt)
+		return false;
+
+	mask = client->evmasks[type];
+	return mask && !test_bit(code, mask);
+}
+
 /* Flush queued events of given type @type and code @code. A negative code
  * is interpreted as catch-all. Caller must hold client->buffer_lock. */
 static void __evdev_flush_queue(struct evdev_client *client,
@@ -137,6 +266,9 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
 static void __pass_event(struct evdev_client *client,
 			 const struct input_event *event)
 {
+	if (__evdev_is_masked(client, event->type, event->code))
+		return;
+
 	client->buffer[client->head++] = *event;
 	client->head &= client->bufsize - 1;
 
@@ -368,6 +500,7 @@ static int evdev_release(struct inode *inode, struct file *file)
 {
 	struct evdev_client *client = file->private_data;
 	struct evdev *evdev = client->evdev;
+	unsigned int i;
 
 	mutex_lock(&evdev->mutex);
 	evdev_ungrab(evdev, client);
@@ -375,6 +508,9 @@ static int evdev_release(struct inode *inode, struct file *file)
 
 	evdev_detach_client(evdev, client);
 
+	for (i = 0; i < EV_CNT; ++i)
+		kfree(client->evmasks[i]);
+
 	if (is_vmalloc_addr(client))
 		vfree(client);
 	else
@@ -866,6 +1002,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 	struct evdev *evdev = client->evdev;
 	struct input_dev *dev = evdev->handle.dev;
 	struct input_absinfo abs;
+	struct input_mask mask;
 	struct ff_effect effect;
 	int __user *ip = (int __user *)p;
 	unsigned int i, t, u, v;
@@ -927,6 +1064,24 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_revoke(evdev, client, file);
 
+	case EVIOCGMASK:
+		if (copy_from_user(&mask, p, sizeof(mask)))
+			return -EFAULT;
+
+		return evdev_get_mask(client,
+				      mask.type,
+				      (void*)(long)mask.codes_ptr,
+				      mask.codes_size);
+
+	case EVIOCSMASK:
+		if (copy_from_user(&mask, p, sizeof(mask)))
+			return -EFAULT;
+
+		return evdev_set_mask(client,
+				      mask.type,
+				      (const void*)(long)mask.codes_ptr,
+				      mask.codes_size);
+
 	case EVIOCSCLOCKID:
 		if (copy_from_user(&i, p, sizeof(unsigned int)))
 			return -EFAULT;
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index bd24470..5b73712 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -97,6 +97,12 @@ struct input_keymap_entry {
 	__u8  scancode[32];
 };
 
+struct input_mask {
+	u32 type;
+	u32 codes_size;
+	u64 codes_ptr;
+};
+
 #define EVIOCGVERSION		_IOR('E', 0x01, int)			/* get driver version */
 #define EVIOCGID		_IOR('E', 0x02, struct input_id)	/* get device ID */
 #define EVIOCGREP		_IOR('E', 0x03, unsigned int[2])	/* get repeat settings */
@@ -153,6 +159,8 @@ struct input_keymap_entry {
 
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
 #define EVIOCREVOKE		_IOW('E', 0x91, int)			/* Revoke device access */
+#define EVIOCGMASK		_IOR('E', 0x92, struct input_mask)	/* Get event-masks */
+#define EVIOCSMASK		_IOW('E', 0x93, struct input_mask)	/* Set event-masks */
 
 #define EVIOCSCLOCKID		_IOW('E', 0xa0, int)			/* Set clockid to be used for timestamps */
 
-- 
1.9.2

Re: [PATCH] Input: evdev - add event-mask API

[Dmitry Torokhov]

Hi David,

On Wed, Apr 16, 2014 at 12:31:45AM +0200, David Herrmann wrote:
> Hardware manufacturers group keys in the weirdest way possible. This may
> cause a power-key to be grouped together with normal keyboard keys and
> thus be reported on the same kernel interface.
> 
> However, user-space is often only interested in specific sets of events.
> For instance, daemons dealing with system-reboot (like systemd-logind)
> listen for KEY_POWER, but are not interested in any main keyboard keys.
> Usually, power keys are reported via separate interfaces, however,
> some i8042 boards report it in the AT matrix. To avoid waking up those
> system daemons on each key-press, we had two ideas:
>  - split off KEY_POWER into a separate interface unconditionally
>  - allow masking a specific set of events on evdev FDs
> 
> Splitting of KEY_POWER is a rather weird way to deal with this and may
> break backwards-compatibility. It is also specific to KEY_POWER and might
> be required for other stuff, too. Moreover, we might end up with a huge
> set of input-devices just to have them properly split.
> 
> Hence, this patchset implements the second idea: An event-mask to specify
> which events you're interested in. Two ioctls allow setting this mask for
> each event-type. If not set, all events are reported. The type==0 entry is
> used same as in EVIOCGBIT to set the actual EV_* mask of masked events.
> This way, you have a two-level filter.

That is something I had in mind for a long time, great job!

> 
> We are heavily forward-compatible to new event-types and event-codes. So
> new user-space will be able to run on an old kernel which doesn't know the
> given event-codes or event event-types.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/input/evdev.c      | 155 +++++++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/input.h |   8 +++
>  2 files changed, 163 insertions(+)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 398648b..86778c3 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -51,10 +51,139 @@ struct evdev_client {
>  	struct list_head node;
>  	int clkid;
>  	bool revoked;
> +	unsigned long *evmasks[EV_CNT];
>  	unsigned int bufsize;
>  	struct input_event buffer[];
>  };
>  
> +static size_t evdev_get_mask_cnt(unsigned int type)
> +{
> +	switch (type) {
> +	case 0:
> +		/* 0 is special (EV-bits instead of EV_SYN) like EVIOCGBIT */
> +		return EV_CNT;
> +	case EV_KEY:
> +		return KEY_CNT;
> +	case EV_REL:
> +		return REL_CNT;
> +	case EV_ABS:
> +		return ABS_CNT;
> +	case EV_MSC:
> +		return MSC_CNT;
> +	case EV_SW:
> +		return SW_CNT;
> +	case EV_LED:
> +		return LED_CNT;
> +	case EV_SND:
> +		return SND_CNT;
> +	case EV_FF:
> +		return FF_CNT;
> +	}

Maybe we need a static array of code->count mapping instead of a switch?

> +
> +	return 0;
> +}
> +
> +/* must be called with evdev-mutex held */
> +static int evdev_set_mask(struct evdev_client *client,
> +			  unsigned int type,
> +			  const void __user *codes,
> +			  u32 codes_size)
> +{
> +	unsigned long flags, *mask, *oldmask;
> +	size_t cnt, size;
> +
> +	/* unknown masks are simply ignored for forward-compat */
> +	cnt = evdev_get_mask_cnt(type);
> +	if (!cnt)
> +		return 0;
> +
> +	/* we allow 'codes_size > size' for forward-compat */
> +	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
> +
> +	mask = kzalloc(size, GFP_KERNEL);
> +	if (!mask)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(mask, codes, min_t(size_t, codes_size, size))) {
> +		kfree(mask);
> +		return -EFAULT;
> +	}
> +
> +	spin_lock_irqsave(&client->buffer_lock, flags);
> +	oldmask = client->evmasks[type];
> +	client->evmasks[type] = mask;
> +	spin_unlock_irqrestore(&client->buffer_lock, flags);
> +
> +	kfree(oldmask);
> +
> +	return 0;
> +}
> +
> +/* must be called with evdev-mutex held */
> +static int evdev_get_mask(struct evdev_client *client,
> +			  unsigned int type,
> +			  void __user *codes,
> +			  u32 codes_size)
> +{
> +	unsigned long *mask;
> +	size_t cnt, size, min, i;
> +	u8 __user *out;
> +
> +	/* we allow unknown types and 'codes_size > size' for forward-compat */
> +	cnt = evdev_get_mask_cnt(type);
> +	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
> +	min = min_t(size_t, codes_size, size);
> +
> +	if (cnt > 0) {
> +		mask = client->evmasks[type];
> +		if (mask) {
> +			if (copy_to_user(codes, mask, min))
> +				return -EFAULT;
> +		} else {
> +			/* fake mask with all bits set */
> +			out = (u8 __user*)codes;
> +			for (i = 0; i < min; ++i) {
> +				if (put_user((u8)0xff,  out + i))
> +					return -EFAULT;
> +			}
> +		}
> +	}
> +
> +	codes = (u8 __user*)codes + min;
> +	codes_size -= min;
> +
> +	if (codes_size > 0 && clear_user(codes, codes_size))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +/* requires the buffer lock to be held */
> +static bool __evdev_is_masked(struct evdev_client *client,
> +			      unsigned int type,
> +			      unsigned int code)
> +{
> +	unsigned long *mask;
> +	size_t cnt;
> +
> +	/* EV_SYN and unknown codes are never masked */

So won't this mean that client is still woken up by "empty" packet if we
filter out everything but EV_SYN?

Thanks.

-- 
Dmitry

Re: [PATCH] Input: evdev - add event-mask API

[Peter Hutterer]

On Wed, Apr 16, 2014 at 12:31:45AM +0200, David Herrmann wrote:
> Hardware manufacturers group keys in the weirdest way possible. This may
> cause a power-key to be grouped together with normal keyboard keys and
> thus be reported on the same kernel interface.
> 
> However, user-space is often only interested in specific sets of events.
> For instance, daemons dealing with system-reboot (like systemd-logind)
> listen for KEY_POWER, but are not interested in any main keyboard keys.
> Usually, power keys are reported via separate interfaces, however,
> some i8042 boards report it in the AT matrix. To avoid waking up those
> system daemons on each key-press, we had two ideas:
>  - split off KEY_POWER into a separate interface unconditionally
>  - allow masking a specific set of events on evdev FDs
> 
> Splitting of KEY_POWER is a rather weird way to deal with this and may
> break backwards-compatibility. It is also specific to KEY_POWER and might
> be required for other stuff, too. Moreover, we might end up with a huge
> set of input-devices just to have them properly split.
> 
> Hence, this patchset implements the second idea: An event-mask to specify
> which events you're interested in. Two ioctls allow setting this mask for
> each event-type. If not set, all events are reported. The type==0 entry is
> used same as in EVIOCGBIT to set the actual EV_* mask of masked events.
> This way, you have a two-level filter.
> 
> We are heavily forward-compatible to new event-types and event-codes. So
> new user-space will be able to run on an old kernel which doesn't know the
> given event-codes or event event-types.

Looks good in principle, a couple of nitpicks below but on the whole
Acked-by: Peter Hutterer <peter.hutterer@who-t.net>

> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/input/evdev.c      | 155 +++++++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/input.h |   8 +++
>  2 files changed, 163 insertions(+)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 398648b..86778c3 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -51,10 +51,139 @@ struct evdev_client {
>  	struct list_head node;
>  	int clkid;
>  	bool revoked;
> +	unsigned long *evmasks[EV_CNT];
>  	unsigned int bufsize;
>  	struct input_event buffer[];
>  };
>  
> +static size_t evdev_get_mask_cnt(unsigned int type)
> +{
> +	switch (type) {
> +	case 0:
> +		/* 0 is special (EV-bits instead of EV_SYN) like EVIOCGBIT */
> +		return EV_CNT;
> +	case EV_KEY:
> +		return KEY_CNT;
> +	case EV_REL:
> +		return REL_CNT;
> +	case EV_ABS:
> +		return ABS_CNT;
> +	case EV_MSC:
> +		return MSC_CNT;
> +	case EV_SW:
> +		return SW_CNT;
> +	case EV_LED:
> +		return LED_CNT;
> +	case EV_SND:
> +		return SND_CNT;
> +	case EV_FF:
> +		return FF_CNT;
> +	}
> +
> +	return 0;
> +}
> +
> +/* must be called with evdev-mutex held */
> +static int evdev_set_mask(struct evdev_client *client,
> +			  unsigned int type,
> +			  const void __user *codes,
> +			  u32 codes_size)
> +{
> +	unsigned long flags, *mask, *oldmask;
> +	size_t cnt, size;
> +
> +	/* unknown masks are simply ignored for forward-compat */
> +	cnt = evdev_get_mask_cnt(type);
> +	if (!cnt)
> +		return 0;
> +
> +	/* we allow 'codes_size > size' for forward-compat */
> +	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
> +
> +	mask = kzalloc(size, GFP_KERNEL);
> +	if (!mask)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(mask, codes, min_t(size_t, codes_size, size))) {
> +		kfree(mask);
> +		return -EFAULT;
> +	}
> +
> +	spin_lock_irqsave(&client->buffer_lock, flags);
> +	oldmask = client->evmasks[type];
> +	client->evmasks[type] = mask;
> +	spin_unlock_irqrestore(&client->buffer_lock, flags);
> +
> +	kfree(oldmask);
> +
> +	return 0;
> +}
> +
> +/* must be called with evdev-mutex held */
> +static int evdev_get_mask(struct evdev_client *client,
> +			  unsigned int type,
> +			  void __user *codes,
> +			  u32 codes_size)
> +{
> +	unsigned long *mask;
> +	size_t cnt, size, min, i;
> +	u8 __user *out;
> +
> +	/* we allow unknown types and 'codes_size > size' for forward-compat */
> +	cnt = evdev_get_mask_cnt(type);
> +	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
> +	min = min_t(size_t, codes_size, size);
> +
> +	if (cnt > 0) {
> +		mask = client->evmasks[type];
> +		if (mask) {
> +			if (copy_to_user(codes, mask, min))
> +				return -EFAULT;
> +		} else {
> +			/* fake mask with all bits set */
> +			out = (u8 __user*)codes;
> +			for (i = 0; i < min; ++i) {
> +				if (put_user((u8)0xff,  out + i))
> +					return -EFAULT;
> +			}
> +		}
> +	}
> +
> +	codes = (u8 __user*)codes + min;
> +	codes_size -= min;
> +
> +	if (codes_size > 0 && clear_user(codes, codes_size))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +/* requires the buffer lock to be held */
> +static bool __evdev_is_masked(struct evdev_client *client,
> +			      unsigned int type,
> +			      unsigned int code)
> +{
> +	unsigned long *mask;
> +	size_t cnt;
> +
> +	/* EV_SYN and unknown codes are never masked */
> +	if (!type || type >= EV_CNT)

why not use type == EV_SYN?

> +		return false;
> +
> +	/* first test whether the type is masked */
> +	mask = client->evmasks[0];

if mask is NULL, you already know it's not mask, you can return early.

> +	if (mask && !test_bit(type, mask))
> +		return true;
> +
> +	/* unknown values are never masked */
> +	cnt = evdev_get_mask_cnt(type);
> +	if (!cnt || code >= cnt)
> +		return false;
> +
> +	mask = client->evmasks[type];
> +	return mask && !test_bit(code, mask);
> +}
> +
>  /* Flush queued events of given type @type and code @code. A negative code
>   * is interpreted as catch-all. Caller must hold client->buffer_lock. */
>  static void __evdev_flush_queue(struct evdev_client *client,
> @@ -137,6 +266,9 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
>  static void __pass_event(struct evdev_client *client,
>  			 const struct input_event *event)
>  {
> +	if (__evdev_is_masked(client, event->type, event->code))
> +		return;
> +
>  	client->buffer[client->head++] = *event;
>  	client->head &= client->bufsize - 1;
>  
> @@ -368,6 +500,7 @@ static int evdev_release(struct inode *inode, struct file *file)
>  {
>  	struct evdev_client *client = file->private_data;
>  	struct evdev *evdev = client->evdev;
> +	unsigned int i;
>  
>  	mutex_lock(&evdev->mutex);
>  	evdev_ungrab(evdev, client);
> @@ -375,6 +508,9 @@ static int evdev_release(struct inode *inode, struct file *file)
>  
>  	evdev_detach_client(evdev, client);
>  
> +	for (i = 0; i < EV_CNT; ++i)
> +		kfree(client->evmasks[i]);
> +
>  	if (is_vmalloc_addr(client))
>  		vfree(client);
>  	else
> @@ -866,6 +1002,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>  	struct evdev *evdev = client->evdev;
>  	struct input_dev *dev = evdev->handle.dev;
>  	struct input_absinfo abs;
> +	struct input_mask mask;
>  	struct ff_effect effect;
>  	int __user *ip = (int __user *)p;
>  	unsigned int i, t, u, v;
> @@ -927,6 +1064,24 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>  		else
>  			return evdev_revoke(evdev, client, file);
>  
> +	case EVIOCGMASK:
> +		if (copy_from_user(&mask, p, sizeof(mask)))
> +			return -EFAULT;
> +
> +		return evdev_get_mask(client,
> +				      mask.type,
> +				      (void*)(long)mask.codes_ptr,
> +				      mask.codes_size);
> +
> +	case EVIOCSMASK:
> +		if (copy_from_user(&mask, p, sizeof(mask)))
> +			return -EFAULT;
> +
> +		return evdev_set_mask(client,
> +				      mask.type,
> +				      (const void*)(long)mask.codes_ptr,
> +				      mask.codes_size);
> +
>  	case EVIOCSCLOCKID:
>  		if (copy_from_user(&i, p, sizeof(unsigned int)))
>  			return -EFAULT;
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index bd24470..5b73712 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -97,6 +97,12 @@ struct input_keymap_entry {
>  	__u8  scancode[32];
>  };
>  
> +struct input_mask {
> +	u32 type;
> +	u32 codes_size;
> +	u64 codes_ptr;
> +};
> +
>  #define EVIOCGVERSION		_IOR('E', 0x01, int)			/* get driver version */
>  #define EVIOCGID		_IOR('E', 0x02, struct input_id)	/* get device ID */
>  #define EVIOCGREP		_IOR('E', 0x03, unsigned int[2])	/* get repeat settings */
> @@ -153,6 +159,8 @@ struct input_keymap_entry {
>  
>  #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
>  #define EVIOCREVOKE		_IOW('E', 0x91, int)			/* Revoke device access */
> +#define EVIOCGMASK		_IOR('E', 0x92, struct input_mask)	/* Get event-masks */
> +#define EVIOCSMASK		_IOW('E', 0x93, struct input_mask)	/* Set event-masks */

This is missing from all other ioctls but while you're adding a new one
anyway: please add documentation on what the ioctl does, the input and
return value/output expected, side-effects etc. right now, understanding the
evdev ioctls requires either reading the kernel code or existing user-space
code, with the usual risk of getting it wrong.

Cheers,
   Peter

>  
>  #define EVIOCSCLOCKID		_IOW('E', 0xa0, int)			/* Set clockid to be used for timestamps */
>  
> -- 
> 1.9.2
> 

Re: [PATCH] Input: evdev - add event-mask API

[David Herrmann]

Hi

On Sat, Apr 19, 2014 at 11:16 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>> +static size_t evdev_get_mask_cnt(unsigned int type)
>> +{
>> +     switch (type) {
>> +     case 0:
>> +             /* 0 is special (EV-bits instead of EV_SYN) like EVIOCGBIT */
>> +             return EV_CNT;
>> +     case EV_KEY:
>> +             return KEY_CNT;
>> +     case EV_REL:
>> +             return REL_CNT;
>> +     case EV_ABS:
>> +             return ABS_CNT;
>> +     case EV_MSC:
>> +             return MSC_CNT;
>> +     case EV_SW:
>> +             return SW_CNT;
>> +     case EV_LED:
>> +             return LED_CNT;
>> +     case EV_SND:
>> +             return SND_CNT;
>> +     case EV_FF:
>> +             return FF_CNT;
>> +     }
>
> Maybe we need a static array of code->count mapping instead of a switch?

Sure, I can change that.

>> +
>> +     return 0;
>> +}
>> +
>> +/* must be called with evdev-mutex held */
>> +static int evdev_set_mask(struct evdev_client *client,
>> +                       unsigned int type,
>> +                       const void __user *codes,
>> +                       u32 codes_size)
>> +{
>> +     unsigned long flags, *mask, *oldmask;
>> +     size_t cnt, size;
>> +
>> +     /* unknown masks are simply ignored for forward-compat */
>> +     cnt = evdev_get_mask_cnt(type);
>> +     if (!cnt)
>> +             return 0;
>> +
>> +     /* we allow 'codes_size > size' for forward-compat */
>> +     size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
>> +
>> +     mask = kzalloc(size, GFP_KERNEL);
>> +     if (!mask)
>> +             return -ENOMEM;
>> +
>> +     if (copy_from_user(mask, codes, min_t(size_t, codes_size, size))) {
>> +             kfree(mask);
>> +             return -EFAULT;
>> +     }
>> +
>> +     spin_lock_irqsave(&client->buffer_lock, flags);
>> +     oldmask = client->evmasks[type];
>> +     client->evmasks[type] = mask;
>> +     spin_unlock_irqrestore(&client->buffer_lock, flags);
>> +
>> +     kfree(oldmask);
>> +
>> +     return 0;
>> +}
>> +
>> +/* must be called with evdev-mutex held */
>> +static int evdev_get_mask(struct evdev_client *client,
>> +                       unsigned int type,
>> +                       void __user *codes,
>> +                       u32 codes_size)
>> +{
>> +     unsigned long *mask;
>> +     size_t cnt, size, min, i;
>> +     u8 __user *out;
>> +
>> +     /* we allow unknown types and 'codes_size > size' for forward-compat */
>> +     cnt = evdev_get_mask_cnt(type);
>> +     size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
>> +     min = min_t(size_t, codes_size, size);
>> +
>> +     if (cnt > 0) {
>> +             mask = client->evmasks[type];
>> +             if (mask) {
>> +                     if (copy_to_user(codes, mask, min))
>> +                             return -EFAULT;
>> +             } else {
>> +                     /* fake mask with all bits set */
>> +                     out = (u8 __user*)codes;
>> +                     for (i = 0; i < min; ++i) {
>> +                             if (put_user((u8)0xff,  out + i))
>> +                                     return -EFAULT;
>> +                     }
>> +             }
>> +     }
>> +
>> +     codes = (u8 __user*)codes + min;
>> +     codes_size -= min;
>> +
>> +     if (codes_size > 0 && clear_user(codes, codes_size))
>> +             return -EFAULT;
>> +
>> +     return 0;
>> +}
>> +
>> +/* requires the buffer lock to be held */
>> +static bool __evdev_is_masked(struct evdev_client *client,
>> +                           unsigned int type,
>> +                           unsigned int code)
>> +{
>> +     unsigned long *mask;
>> +     size_t cnt;
>> +
>> +     /* EV_SYN and unknown codes are never masked */
>
> So won't this mean that client is still woken up by "empty" packet if we
> filter out everything but EV_SYN?

Whoops, indeed. I will skip SYN_REPORT events if the queue is empty or
if the previous event was already a SYN_REPORT.

Thanks
David

Re: [PATCH] Input: evdev - add event-mask API

[David Herrmann]

Hi

On Tue, Apr 22, 2014 at 6:29 AM, Peter Hutterer
<peter.hutterer@who-t.net> wrote:
>> +/* requires the buffer lock to be held */
>> +static bool __evdev_is_masked(struct evdev_client *client,
>> +                           unsigned int type,
>> +                           unsigned int code)
>> +{
>> +     unsigned long *mask;
>> +     size_t cnt;
>> +
>> +     /* EV_SYN and unknown codes are never masked */
>> +     if (!type || type >= EV_CNT)
>
> why not use type == EV_SYN?

You mean instead of "!type"? Yeah, probably easier to read.

>> +             return false;
>> +
>> +     /* first test whether the type is masked */
>> +     mask = client->evmasks[0];
>
> if mask is NULL, you already know it's not mask, you can return early.

Nope. If evmasks[0] is NULL, then no type-mask has been set. That
doesn't mean the given code-mask is NULL.

For instance:
  evmasks[0] == NULL
  evmasks[EV_KEY][KEY_A] == 0

In that case, the user wants all events but KEY_A (even though
evmasks[0] is NULL).

>>  #define EVIOCGRAB            _IOW('E', 0x90, int)                    /* Grab/Release device */
>>  #define EVIOCREVOKE          _IOW('E', 0x91, int)                    /* Revoke device access */
>> +#define EVIOCGMASK           _IOR('E', 0x92, struct input_mask)      /* Get event-masks */
>> +#define EVIOCSMASK           _IOW('E', 0x93, struct input_mask)      /* Set event-masks */
>
> This is missing from all other ioctls but while you're adding a new one
> anyway: please add documentation on what the ioctl does, the input and
> return value/output expected, side-effects etc. right now, understanding the
> evdev ioctls requires either reading the kernel code or existing user-space
> code, with the usual risk of getting it wrong.

Meh.. I thought no-one will notice.. Clearly, I was wrong. I will add
proper docs in v2.

Thanks a lot!
David

[PATCH] Input: evdev - add event-mask API

[David Herrmann]

Hardware manufacturers group keys in the weirdest way possible. This may
cause a power-key to be grouped together with normal keyboard keys and
thus be reported on the same kernel interface.

However, user-space is often only interested in specific sets of events.
For instance, daemons dealing with system-reboot (like systemd-logind)
listen for KEY_POWER, but are not interested in any main keyboard keys.
Usually, power keys are reported via separate interfaces, however,
some i8042 boards report it in the AT matrix. To avoid waking up those
system daemons on each key-press, we had two ideas:
 - split off KEY_POWER into a separate interface unconditionally
 - allow filtering a specific set of events on evdev FDs

Splitting of KEY_POWER is a rather weird way to deal with this and may
break backwards-compatibility. It is also specific to KEY_POWER and might
be required for other stuff, too. Moreover, we might end up with a huge
set of input-devices just to have them properly split.

Hence, this patchset implements the second idea: An event-mask to specify
which events you're interested in. Two ioctls allow setting this mask for
each event-type. If not set, all events are reported. The type==0 entry is
used same as in EVIOCGBIT to set the actual EV_* mask of filtered events.
This way, you have a two-level filter.

We are heavily forward-compatible to new event-types and event-codes. So
new user-space will be able to run on an old kernel which doesn't know the
given event-codes or event-types.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/input/evdev.c      | 206 ++++++++++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/input.h |  60 +++++++++++++
 2 files changed, 264 insertions(+), 2 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 9d35499..37876bb 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -58,10 +58,55 @@ struct evdev_client {
 	struct list_head node;
 	int clk_type;
 	bool revoked;
+	unsigned long *evmasks[EV_CNT];
 	unsigned int bufsize;
 	struct input_event buffer[];
 };
 
+static size_t evdev_get_mask_cnt(unsigned int type)
+{
+	static const size_t counts[EV_CNT] = {
+		/* EV_SYN==0 is EV_CNT, _not_ SYN_CNT, see EVIOCGBIT */
+		[EV_SYN]	= EV_CNT,
+		[EV_KEY]	= KEY_CNT,
+		[EV_REL]	= REL_CNT,
+		[EV_ABS]	= ABS_CNT,
+		[EV_MSC]	= MSC_CNT,
+		[EV_SW]		= SW_CNT,
+		[EV_LED]	= LED_CNT,
+		[EV_SND]	= SND_CNT,
+		[EV_FF]		= FF_CNT,
+	};
+
+	return (type < EV_CNT) ? counts[type] : 0;
+}
+
+/* requires the buffer lock to be held */
+static bool __evdev_is_filtered(struct evdev_client *client,
+				unsigned int type,
+				unsigned int code)
+{
+	unsigned long *mask;
+	size_t cnt;
+
+	/* EV_SYN and unknown codes are never filtered */
+	if (type == EV_SYN || type >= EV_CNT)
+		return false;
+
+	/* first test whether the type is filtered */
+	mask = client->evmasks[0];
+	if (mask && !test_bit(type, mask))
+		return true;
+
+	/* unknown values are never filtered */
+	cnt = evdev_get_mask_cnt(type);
+	if (!cnt || code >= cnt)
+		return false;
+
+	mask = client->evmasks[type];
+	return mask && !test_bit(code, mask);
+}
+
 /* flush queued events of type @type, caller must hold client->buffer_lock */
 static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
 {
@@ -226,12 +271,21 @@ static void evdev_pass_values(struct evdev_client *client,
 	spin_lock(&client->buffer_lock);
 
 	for (v = vals; v != vals + count; v++) {
+		if (__evdev_is_filtered(client, v->type, v->code))
+			continue;
+
+		if (v->type == EV_SYN && v->code == SYN_REPORT) {
+			/* drop empty SYN_REPORT */
+			if (client->packet_head == client->head)
+				continue;
+
+			wakeup = true;
+		}
+
 		event.type = v->type;
 		event.code = v->code;
 		event.value = v->value;
 		__pass_event(client, &event);
-		if (v->type == EV_SYN && v->code == SYN_REPORT)
-			wakeup = true;
 	}
 
 	spin_unlock(&client->buffer_lock);
@@ -415,6 +469,7 @@ static int evdev_release(struct inode *inode, struct file *file)
 {
 	struct evdev_client *client = file->private_data;
 	struct evdev *evdev = client->evdev;
+	unsigned int i;
 
 	mutex_lock(&evdev->mutex);
 	evdev_ungrab(evdev, client);
@@ -422,6 +477,9 @@ static int evdev_release(struct inode *inode, struct file *file)
 
 	evdev_detach_client(evdev, client);
 
+	for (i = 0; i < EV_CNT; ++i)
+		kfree(client->evmasks[i]);
+
 	kvfree(client);
 
 	evdev_close_device(evdev);
@@ -662,6 +720,47 @@ static int bits_to_user(unsigned long *bits, unsigned int maxbit,
 
 #endif /* CONFIG_COMPAT */
 
+static int bits_from_user(unsigned long *bits, unsigned int maxbit,
+			  unsigned int maxlen, const void __user *p, int compat)
+{
+	int len;
+
+#if IS_ENABLED(CONFIG_COMPAT)
+	if (compat) {
+		if (maxlen % sizeof(compat_long_t))
+			return -EINVAL;
+		len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
+	} else
+#endif
+	{
+		if (maxlen % sizeof(long))
+			return -EINVAL;
+		len = BITS_TO_LONGS(maxbit) * sizeof(long);
+	}
+
+	if (len > maxlen)
+		len = maxlen;
+
+#if IS_ENABLED(CONFIG_COMPAT) && defined(__BIG_ENDIAN)
+	if (compat) {
+		int i;
+
+		for (i = 0; i < len / sizeof(compat_long_t); i++)
+			if (copy_from_user((compat_long_t *) bits +
+						i + 1 - ((i % 2) << 1),
+					   (compat_long_t __user *) p + i,
+					   sizeof(compat_long_t)))
+				return -EFAULT;
+		if (i % 2)
+			*((compat_long_t *) bits + i - 1) = 0;
+	} else
+#endif
+		if (copy_from_user(bits, p, len))
+			return -EFAULT;
+
+	return len;
+}
+
 static int str_to_user(const char *str, unsigned int maxlen, void __user *p)
 {
 	int len;
@@ -854,6 +953,86 @@ static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
 	return 0;
 }
 
+/* must be called with evdev-mutex held */
+static int evdev_set_mask(struct evdev_client *client,
+			  unsigned int type,
+			  const void __user *codes,
+			  u32 codes_size,
+			  int compat)
+{
+	unsigned long flags, *mask, *oldmask;
+	size_t cnt, size, min;
+	int error;
+
+	/* we allow unknown types and 'codes_size > size' for forward-compat */
+	cnt = evdev_get_mask_cnt(type);
+	if (!cnt)
+		return 0;
+
+	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
+	min = min_t(size_t, codes_size, size);
+
+	mask = kzalloc(size, GFP_KERNEL);
+	if (!mask)
+		return -ENOMEM;
+
+	error = bits_from_user(mask, cnt - 1, min, codes, compat);
+	if (error < 0) {
+		kfree(mask);
+		return error;
+	}
+
+	spin_lock_irqsave(&client->buffer_lock, flags);
+	oldmask = client->evmasks[type];
+	client->evmasks[type] = mask;
+	spin_unlock_irqrestore(&client->buffer_lock, flags);
+
+	kfree(oldmask);
+
+	return 0;
+}
+
+/* must be called with evdev-mutex held */
+static int evdev_get_mask(struct evdev_client *client,
+			  unsigned int type,
+			  void __user *codes,
+			  u32 codes_size,
+			  int compat)
+{
+	unsigned long *mask;
+	size_t cnt, size, min, i;
+	u8 __user *out;
+	int error;
+
+	/* we allow unknown types and 'codes_size > size' for forward-compat */
+	cnt = evdev_get_mask_cnt(type);
+	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
+	min = min_t(size_t, codes_size, size);
+
+	if (cnt > 0) {
+		mask = client->evmasks[type];
+		if (mask) {
+			error = bits_to_user(mask, cnt - 1, min, codes, compat);
+			if (error < 0)
+				return error;
+		} else {
+			/* fake mask with all bits set */
+			out = (u8 __user*)codes;
+			for (i = 0; i < min; ++i)
+				if (put_user((u8)0xff,  out + i))
+					return -EFAULT;
+		}
+	}
+
+	codes = (u8*)codes + min;
+	codes_size -= min;
+
+	if (codes_size > 0 && clear_user(codes, codes_size))
+		return -EFAULT;
+
+	return 0;
+}
+
 static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 			   void __user *p, int compat_mode)
 {
@@ -861,6 +1040,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 	struct evdev *evdev = client->evdev;
 	struct input_dev *dev = evdev->handle.dev;
 	struct input_absinfo abs;
+	struct input_mask mask;
 	struct ff_effect effect;
 	int __user *ip = (int __user *)p;
 	unsigned int i, t, u, v;
@@ -922,6 +1102,28 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_revoke(evdev, client, file);
 
+	case EVIOCGMASK:
+		if (copy_from_user(&mask, p, sizeof(mask)))
+			return -EFAULT;
+
+		return evdev_get_mask(client,
+				      mask.type,
+				      (void __user*)
+				      (unsigned long)mask.codes_ptr,
+				      mask.codes_size,
+				      compat_mode);
+
+	case EVIOCSMASK:
+		if (copy_from_user(&mask, p, sizeof(mask)))
+			return -EFAULT;
+
+		return evdev_set_mask(client,
+				      mask.type,
+				      (const void __user*)
+				      (unsigned long)mask.codes_ptr,
+				      mask.codes_size,
+				      compat_mode);
+
 	case EVIOCSCLOCKID:
 		if (copy_from_user(&i, p, sizeof(unsigned int)))
 			return -EFAULT;
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 731417c..ce68f86 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -97,6 +97,12 @@ struct input_keymap_entry {
 	__u8  scancode[32];
 };
 
+struct input_mask {
+	__u32 type;
+	__u32 codes_size;
+	__u64 codes_ptr;
+};
+
 #define EVIOCGVERSION		_IOR('E', 0x01, int)			/* get driver version */
 #define EVIOCGID		_IOR('E', 0x02, struct input_id)	/* get device ID */
 #define EVIOCGREP		_IOR('E', 0x03, unsigned int[2])	/* get repeat settings */
@@ -154,6 +160,60 @@ struct input_keymap_entry {
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
 #define EVIOCREVOKE		_IOW('E', 0x91, int)			/* Revoke device access */
 
+/**
+ * EVIOCGMASK - Retrieve current event mask
+ *
+ * This ioctl allows user to retrieve the current event mask for specific
+ * event type. The argument must be of type "struct input_mask" and
+ * specifies the event type to query, the address of the receive buffer and
+ * the size of the receive buffer.
+ *
+ * The event mask is a per-client mask that specifies which events are
+ * forwarded to the client. Each event code is represented by a single bit
+ * in the event mask. If the bit is set, the event is passed to the client
+ * normally. Otherwise, the event is filtered and will never be queued on
+ * the client's receive buffer.
+ *
+ * Event masks do not affect global state of the input device. They only
+ * affect the file descriptor they are applied to.
+ *
+ * The default event mask for a client has all bits set, i.e. all events
+ * are forwarded to the client. If the kernel is queried for an unknown
+ * event type or if the receive buffer is larger than the number of
+ * event codes known to the kernel, the kernel returns all zeroes for those
+ * codes.
+ *
+ * At maximum, codes_size bytes are copied.
+ *
+ * This ioctl may fail with ENODEV in case the file is revoked, EFAULT
+ * if the receive-buffer points to invalid memory, or EINVAL if the kernel
+ * does not implement the ioctl.
+ */
+#define EVIOCGMASK		_IOR('E', 0x92, struct input_mask)	/* Get event-masks */
+
+/**
+ * EVIOCSMASK - Set event mask
+ *
+ * This ioctl is the counterpart to EVIOCGMASK. Instead of receiving the
+ * current event mask, this changes the client's event mask for a specific
+ * type.  See EVIOCGMASK for a description of event-masks and the
+ * argument-type.
+ *
+ * This ioctl provides full forward compatibility. If the passed event type
+ * is unknown to the kernel, or if the number of event codes specified in
+ * the mask is bigger than what is known to the kernel, the ioctl is still
+ * accepted and applied. However, any unknown codes are left untouched and
+ * stay cleared. That means, the kernel always filters unknown codes
+ * regardless of what the client requests.  If the new mask doesn't cover
+ * all known event-codes, all remaining codes are automatically cleared and
+ * thus filtered.
+ *
+ * This ioctl may fail with ENODEV in case the file is revoked. EFAULT is
+ * returned if the receive-buffer points to invalid memory. EINVAL is returned
+ * if the kernel does not implement the ioctl.
+ */
+#define EVIOCSMASK		_IOW('E', 0x93, struct input_mask)	/* Set event-masks */
+
 #define EVIOCSCLOCKID		_IOW('E', 0xa0, int)			/* Set clockid to be used for timestamps */
 
 /*
-- 
2.5.1

Re: [PATCH] Input: evdev - add event-mask API

[David Herrmann]

Hey

On Thu, Sep 3, 2015 at 6:14 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hardware manufacturers group keys in the weirdest way possible. This may
> cause a power-key to be grouped together with normal keyboard keys and
> thus be reported on the same kernel interface.
>
> However, user-space is often only interested in specific sets of events.
> For instance, daemons dealing with system-reboot (like systemd-logind)
> listen for KEY_POWER, but are not interested in any main keyboard keys.
> Usually, power keys are reported via separate interfaces, however,
> some i8042 boards report it in the AT matrix. To avoid waking up those
> system daemons on each key-press, we had two ideas:
>  - split off KEY_POWER into a separate interface unconditionally
>  - allow filtering a specific set of events on evdev FDs
>
> Splitting of KEY_POWER is a rather weird way to deal with this and may
> break backwards-compatibility. It is also specific to KEY_POWER and might
> be required for other stuff, too. Moreover, we might end up with a huge
> set of input-devices just to have them properly split.
>
> Hence, this patchset implements the second idea: An event-mask to specify
> which events you're interested in. Two ioctls allow setting this mask for
> each event-type. If not set, all events are reported. The type==0 entry is
> used same as in EVIOCGBIT to set the actual EV_* mask of filtered events.
> This way, you have a two-level filter.
>
> We are heavily forward-compatible to new event-types and event-codes. So
> new user-space will be able to run on an old kernel which doesn't know the
> given event-codes or event-types.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Ping?

David

Re: [PATCH] Input: evdev - add event-mask API

[Dmitry Torokhov]

Hi David,

On Thu, Sep 03, 2015 at 06:14:01PM +0200, David Herrmann wrote:
> +static int bits_from_user(unsigned long *bits, unsigned int maxbit,
> +			  unsigned int maxlen, const void __user *p, int compat)
> +{
> +	int len;
> +
> +#if IS_ENABLED(CONFIG_COMPAT)
> +	if (compat) {
> +		if (maxlen % sizeof(compat_long_t))
> +			return -EINVAL;
> +		len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
> +	} else
> +#endif
> +	{
> +		if (maxlen % sizeof(long))
> +			return -EINVAL;
> +		len = BITS_TO_LONGS(maxbit) * sizeof(long);
> +	}
> +
> +	if (len > maxlen)
> +		len = maxlen;
> +
> +#if IS_ENABLED(CONFIG_COMPAT) && defined(__BIG_ENDIAN)
> +	if (compat) {
> +		int i;
> +
> +		for (i = 0; i < len / sizeof(compat_long_t); i++)
> +			if (copy_from_user((compat_long_t *) bits +
> +						i + 1 - ((i % 2) << 1),
> +					   (compat_long_t __user *) p + i,
> +					   sizeof(compat_long_t)))
> +				return -EFAULT;
> +		if (i % 2)
> +			*((compat_long_t *) bits + i - 1) = 0;
> +	} else
> +#endif
> +		if (copy_from_user(bits, p, len))
> +			return -EFAULT;
> +
> +	return len;
> +}

I do not quite like how we sprinkle ifdefs throughout, I prefer the way
we have bits_to_user defined, even if it is more verbose.


> +
>  static int str_to_user(const char *str, unsigned int maxlen, void __user *p)
>  {
>  	int len;
> @@ -854,6 +953,86 @@ static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
>  	return 0;
>  }
>  
> +/* must be called with evdev-mutex held */
> +static int evdev_set_mask(struct evdev_client *client,
> +			  unsigned int type,
> +			  const void __user *codes,
> +			  u32 codes_size,
> +			  int compat)
> +{
> +	unsigned long flags, *mask, *oldmask;
> +	size_t cnt, size, min;
> +	int error;
> +
> +	/* we allow unknown types and 'codes_size > size' for forward-compat */
> +	cnt = evdev_get_mask_cnt(type);
> +	if (!cnt)
> +		return 0;
> +
> +	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
> +	min = min_t(size_t, codes_size, size);
> +
> +	mask = kzalloc(size, GFP_KERNEL);
> +	if (!mask)
> +		return -ENOMEM;
> +
> +	error = bits_from_user(mask, cnt - 1, min, codes, compat);

I do not think we need to calculate and pass min here: bits_from_user()
will limit the output for us already.

Does it still work if you apply the patch below?

Thanks!

-- 
Dmitry

Input: evdev - mask api misc changes

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/evdev.c |  148 +++++++++++++++++++++++++++++--------------------
 1 file changed, 88 insertions(+), 60 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 83d699f..ce35ea3 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -685,7 +685,46 @@ static int bits_to_user(unsigned long *bits, unsigned int maxbit,
 
 	return len;
 }
+
+static int bits_from_user(unsigned long *bits, unsigned int maxbit,
+			  unsigned int maxlen, const void __user *p, int compat)
+{
+	int len, i;
+
+	if (compat) {
+		if (maxlen % sizeof(compat_long_t))
+			return -EINVAL;
+
+		len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
+		if (len > maxlen)
+			len = maxlen;
+
+		for (i = 0; i < len / sizeof(compat_long_t); i++)
+			if (copy_from_user((compat_long_t *) bits +
+						i + 1 - ((i % 2) << 1),
+					   (compat_long_t __user *) p + i,
+					   sizeof(compat_long_t)))
+				return -EFAULT;
+		if (i % 2)
+			*((compat_long_t *) bits + i - 1) = 0;
+
+	} else {
+		if (maxlen % sizeof(long))
+			return -EINVAL;
+
+		len = BITS_TO_LONGS(maxbit) * sizeof(long);
+		if (len > maxlen)
+			len = maxlen;
+
+		if (copy_from_user(p, bits, len))
+			return -EFAULT;
+	}
+
+	return len;
+}
+
 #else
+
 static int bits_to_user(unsigned long *bits, unsigned int maxbit,
 			unsigned int maxlen, void __user *p, int compat)
 {
@@ -698,6 +737,24 @@ static int bits_to_user(unsigned long *bits, unsigned int maxbit,
 
 	return copy_to_user(p, bits, len) ? -EFAULT : len;
 }
+
+static int bits_from_user(unsigned long *bits, unsigned int maxbit,
+			  unsigned int maxlen, const void __user *p, int compat)
+{
+	size_t chunk_size = compat ? sizeof(compat_long_t) : sizeof(long);
+	int len;
+
+	if (maxlen % chunk_size)
+		return -EINVAL;
+
+	len = compat ? BITS_TO_LONGS_COMPAT(maxbit) : BITS_TO_LONGS(maxbit);
+	len *= chunk_size;
+	if (len > maxlen)
+		len = maxlen;
+
+	return copy_from_user(bits, p, len) ? -EFAULT : len;
+}
+
 #endif /* __BIG_ENDIAN */
 
 #else
@@ -713,49 +770,23 @@ static int bits_to_user(unsigned long *bits, unsigned int maxbit,
 	return copy_to_user(p, bits, len) ? -EFAULT : len;
 }
 
-#endif /* CONFIG_COMPAT */
-
 static int bits_from_user(unsigned long *bits, unsigned int maxbit,
 			  unsigned int maxlen, const void __user *p, int compat)
 {
 	int len;
 
-#if IS_ENABLED(CONFIG_COMPAT)
-	if (compat) {
-		if (maxlen % sizeof(compat_long_t))
-			return -EINVAL;
-		len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
-	} else
-#endif
-	{
-		if (maxlen % sizeof(long))
-			return -EINVAL;
-		len = BITS_TO_LONGS(maxbit) * sizeof(long);
-	}
+	if (maxlen % sizeof(long))
+		return -EINVAL;
 
+	len = BITS_TO_LONGS(maxbit) * sizeof(long);
 	if (len > maxlen)
 		len = maxlen;
 
-#if IS_ENABLED(CONFIG_COMPAT) && defined(__BIG_ENDIAN)
-	if (compat) {
-		int i;
-
-		for (i = 0; i < len / sizeof(compat_long_t); i++)
-			if (copy_from_user((compat_long_t *) bits +
-						i + 1 - ((i % 2) << 1),
-					   (compat_long_t __user *) p + i,
-					   sizeof(compat_long_t)))
-				return -EFAULT;
-		if (i % 2)
-			*((compat_long_t *) bits + i - 1) = 0;
-	} else
-#endif
-		if (copy_from_user(bits, p, len))
-			return -EFAULT;
-
-	return len;
+	return copy_from_user(bits, p, len) ? -EFAULT : len;
 }
 
+#endif /* CONFIG_COMPAT */
+
 static int str_to_user(const char *str, unsigned int maxlen, void __user *p)
 {
 	int len;
@@ -956,7 +987,7 @@ static int evdev_set_mask(struct evdev_client *client,
 			  int compat)
 {
 	unsigned long flags, *mask, *oldmask;
-	size_t cnt, size, min;
+	size_t cnt;
 	int error;
 
 	/* we allow unknown types and 'codes_size > size' for forward-compat */
@@ -964,14 +995,11 @@ static int evdev_set_mask(struct evdev_client *client,
 	if (!cnt)
 		return 0;
 
-	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
-	min = min_t(size_t, codes_size, size);
-
-	mask = kzalloc(size, GFP_KERNEL);
+	mask = kcalloc(sizeof(unsigned long), BITS_TO_LONGS(cnt), GFP_KERNEL);
 	if (!mask)
 		return -ENOMEM;
 
-	error = bits_from_user(mask, cnt - 1, min, codes, compat);
+	error = bits_from_user(mask, cnt - 1, codes_size, codes, compat);
 	if (error < 0) {
 		kfree(mask);
 		return error;
@@ -995,35 +1023,33 @@ static int evdev_get_mask(struct evdev_client *client,
 			  int compat)
 {
 	unsigned long *mask;
-	size_t cnt, size, min, i;
-	u8 __user *out;
+	size_t cnt, size, xfer_size;
+	int i;
 	int error;
 
 	/* we allow unknown types and 'codes_size > size' for forward-compat */
 	cnt = evdev_get_mask_cnt(type);
 	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
-	min = min_t(size_t, codes_size, size);
+	xfer_size = min_t(size_t, codes_size, size);
 
 	if (cnt > 0) {
 		mask = client->evmasks[type];
 		if (mask) {
-			error = bits_to_user(mask, cnt - 1, min, codes, compat);
+			error = bits_to_user(mask, cnt - 1,
+					     xfer_size, codes, compat);
 			if (error < 0)
 				return error;
 		} else {
 			/* fake mask with all bits set */
-			out = (u8 __user*)codes;
-			for (i = 0; i < min; ++i)
-				if (put_user((u8)0xff,  out + i))
+			for (i = 0; i < xfer_size; i++)
+				if (put_user(0xffU, (u8 __user *)codes + i))
 					return -EFAULT;
 		}
 	}
 
-	codes = (u8*)codes + min;
-	codes_size -= min;
-
-	if (codes_size > 0 && clear_user(codes, codes_size))
-		return -EFAULT;
+	if (xfer_size < codes_size)
+		if (clear_user(codes + xfer_size, codes_size - xfer_size))
+			return -EFAULT;
 
 	return 0;
 }
@@ -1097,27 +1123,29 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_revoke(evdev, client, file);
 
-	case EVIOCGMASK:
+	case EVIOCGMASK: {
+		void __user *codes_ptr;
+
 		if (copy_from_user(&mask, p, sizeof(mask)))
 			return -EFAULT;
 
+		codes_ptr = (void __user *)(unsigned long)mask.codes_ptr;
 		return evdev_get_mask(client,
-				      mask.type,
-				      (void __user*)
-				      (unsigned long)mask.codes_ptr,
-				      mask.codes_size,
+				      mask.type, codes_ptr, mask.codes_size,
 				      compat_mode);
+	}
+
+	case EVIOCSMASK: {
+		const void __user *codes_ptr;
 
-	case EVIOCSMASK:
 		if (copy_from_user(&mask, p, sizeof(mask)))
 			return -EFAULT;
 
+		codes_ptr = (const void __user *)(unsigned long)mask.codes_ptr;
 		return evdev_set_mask(client,
-				      mask.type,
-				      (const void __user*)
-				      (unsigned long)mask.codes_ptr,
-				      mask.codes_size,
+				      mask.type, codes_ptr, mask.codes_size,
 				      compat_mode);
+	}
 
 	case EVIOCSCLOCKID:
 		if (copy_from_user(&i, p, sizeof(unsigned int)))

Re: [PATCH] Input: evdev - add event-mask API

[David Herrmann]

Hi

On Sun, Oct 25, 2015 at 2:17 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi David,
>
> On Thu, Sep 03, 2015 at 06:14:01PM +0200, David Herrmann wrote:
>> +static int bits_from_user(unsigned long *bits, unsigned int maxbit,
>> +                       unsigned int maxlen, const void __user *p, int compat)
>> +{
>> +     int len;
>> +
>> +#if IS_ENABLED(CONFIG_COMPAT)
>> +     if (compat) {
>> +             if (maxlen % sizeof(compat_long_t))
>> +                     return -EINVAL;
>> +             len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
>> +     } else
>> +#endif
>> +     {
>> +             if (maxlen % sizeof(long))
>> +                     return -EINVAL;
>> +             len = BITS_TO_LONGS(maxbit) * sizeof(long);
>> +     }
>> +
>> +     if (len > maxlen)
>> +             len = maxlen;
>> +
>> +#if IS_ENABLED(CONFIG_COMPAT) && defined(__BIG_ENDIAN)
>> +     if (compat) {
>> +             int i;
>> +
>> +             for (i = 0; i < len / sizeof(compat_long_t); i++)
>> +                     if (copy_from_user((compat_long_t *) bits +
>> +                                             i + 1 - ((i % 2) << 1),
>> +                                        (compat_long_t __user *) p + i,
>> +                                        sizeof(compat_long_t)))
>> +                             return -EFAULT;
>> +             if (i % 2)
>> +                     *((compat_long_t *) bits + i - 1) = 0;
>> +     } else
>> +#endif
>> +             if (copy_from_user(bits, p, len))
>> +                     return -EFAULT;
>> +
>> +     return len;
>> +}
>
> I do not quite like how we sprinkle ifdefs throughout, I prefer the way
> we have bits_to_user defined, even if it is more verbose.

Makes sense.

>> +
>>  static int str_to_user(const char *str, unsigned int maxlen, void __user *p)
>>  {
>>       int len;
>> @@ -854,6 +953,86 @@ static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
>>       return 0;
>>  }
>>
>> +/* must be called with evdev-mutex held */
>> +static int evdev_set_mask(struct evdev_client *client,
>> +                       unsigned int type,
>> +                       const void __user *codes,
>> +                       u32 codes_size,
>> +                       int compat)
>> +{
>> +     unsigned long flags, *mask, *oldmask;
>> +     size_t cnt, size, min;
>> +     int error;
>> +
>> +     /* we allow unknown types and 'codes_size > size' for forward-compat */
>> +     cnt = evdev_get_mask_cnt(type);
>> +     if (!cnt)
>> +             return 0;
>> +
>> +     size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
>> +     min = min_t(size_t, codes_size, size);
>> +
>> +     mask = kzalloc(size, GFP_KERNEL);
>> +     if (!mask)
>> +             return -ENOMEM;
>> +
>> +     error = bits_from_user(mask, cnt - 1, min, codes, compat);
>
> I do not think we need to calculate and pass min here: bits_from_user()
> will limit the output for us already.
>
> Does it still work if you apply the patch below?

One comment on void*-arithmetic below. Otherwise, this is reviewed and
tested by me.

Thanks a lot!
David

> ---
>  drivers/input/evdev.c |  148 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 88 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 83d699f..ce35ea3 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -685,7 +685,46 @@ static int bits_to_user(unsigned long *bits, unsigned int maxbit,
>
>         return len;
>  }
> +
> +static int bits_from_user(unsigned long *bits, unsigned int maxbit,
> +                         unsigned int maxlen, const void __user *p, int compat)
> +{
> +       int len, i;
> +
> +       if (compat) {
> +               if (maxlen % sizeof(compat_long_t))
> +                       return -EINVAL;
> +
> +               len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
> +               if (len > maxlen)
> +                       len = maxlen;
> +
> +               for (i = 0; i < len / sizeof(compat_long_t); i++)
> +                       if (copy_from_user((compat_long_t *) bits +
> +                                               i + 1 - ((i % 2) << 1),
> +                                          (compat_long_t __user *) p + i,
> +                                          sizeof(compat_long_t)))
> +                               return -EFAULT;
> +               if (i % 2)
> +                       *((compat_long_t *) bits + i - 1) = 0;
> +
> +       } else {
> +               if (maxlen % sizeof(long))
> +                       return -EINVAL;
> +
> +               len = BITS_TO_LONGS(maxbit) * sizeof(long);
> +               if (len > maxlen)
> +                       len = maxlen;
> +
> +               if (copy_from_user(p, bits, len))
> +                       return -EFAULT;
> +       }
> +
> +       return len;
> +}
> +
>  #else
> +
>  static int bits_to_user(unsigned long *bits, unsigned int maxbit,
>                         unsigned int maxlen, void __user *p, int compat)
>  {
> @@ -698,6 +737,24 @@ static int bits_to_user(unsigned long *bits, unsigned int maxbit,
>
>         return copy_to_user(p, bits, len) ? -EFAULT : len;
>  }
> +
> +static int bits_from_user(unsigned long *bits, unsigned int maxbit,
> +                         unsigned int maxlen, const void __user *p, int compat)
> +{
> +       size_t chunk_size = compat ? sizeof(compat_long_t) : sizeof(long);
> +       int len;
> +
> +       if (maxlen % chunk_size)
> +               return -EINVAL;
> +
> +       len = compat ? BITS_TO_LONGS_COMPAT(maxbit) : BITS_TO_LONGS(maxbit);
> +       len *= chunk_size;
> +       if (len > maxlen)
> +               len = maxlen;
> +
> +       return copy_from_user(bits, p, len) ? -EFAULT : len;
> +}
> +
>  #endif /* __BIG_ENDIAN */
>
>  #else
> @@ -713,49 +770,23 @@ static int bits_to_user(unsigned long *bits, unsigned int maxbit,
>         return copy_to_user(p, bits, len) ? -EFAULT : len;
>  }
>
> -#endif /* CONFIG_COMPAT */
> -
>  static int bits_from_user(unsigned long *bits, unsigned int maxbit,
>                           unsigned int maxlen, const void __user *p, int compat)
>  {
>         int len;
>
> -#if IS_ENABLED(CONFIG_COMPAT)
> -       if (compat) {
> -               if (maxlen % sizeof(compat_long_t))
> -                       return -EINVAL;
> -               len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
> -       } else
> -#endif
> -       {
> -               if (maxlen % sizeof(long))
> -                       return -EINVAL;
> -               len = BITS_TO_LONGS(maxbit) * sizeof(long);
> -       }
> +       if (maxlen % sizeof(long))
> +               return -EINVAL;
>
> +       len = BITS_TO_LONGS(maxbit) * sizeof(long);
>         if (len > maxlen)
>                 len = maxlen;
>
> -#if IS_ENABLED(CONFIG_COMPAT) && defined(__BIG_ENDIAN)
> -       if (compat) {
> -               int i;
> -
> -               for (i = 0; i < len / sizeof(compat_long_t); i++)
> -                       if (copy_from_user((compat_long_t *) bits +
> -                                               i + 1 - ((i % 2) << 1),
> -                                          (compat_long_t __user *) p + i,
> -                                          sizeof(compat_long_t)))
> -                               return -EFAULT;
> -               if (i % 2)
> -                       *((compat_long_t *) bits + i - 1) = 0;
> -       } else
> -#endif
> -               if (copy_from_user(bits, p, len))
> -                       return -EFAULT;
> -
> -       return len;
> +       return copy_from_user(bits, p, len) ? -EFAULT : len;
>  }
>
> +#endif /* CONFIG_COMPAT */
> +
>  static int str_to_user(const char *str, unsigned int maxlen, void __user *p)
>  {
>         int len;
> @@ -956,7 +987,7 @@ static int evdev_set_mask(struct evdev_client *client,
>                           int compat)
>  {
>         unsigned long flags, *mask, *oldmask;
> -       size_t cnt, size, min;
> +       size_t cnt;
>         int error;
>
>         /* we allow unknown types and 'codes_size > size' for forward-compat */
> @@ -964,14 +995,11 @@ static int evdev_set_mask(struct evdev_client *client,
>         if (!cnt)
>                 return 0;
>
> -       size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
> -       min = min_t(size_t, codes_size, size);
> -
> -       mask = kzalloc(size, GFP_KERNEL);
> +       mask = kcalloc(sizeof(unsigned long), BITS_TO_LONGS(cnt), GFP_KERNEL);
>         if (!mask)
>                 return -ENOMEM;
>
> -       error = bits_from_user(mask, cnt - 1, min, codes, compat);
> +       error = bits_from_user(mask, cnt - 1, codes_size, codes, compat);
>         if (error < 0) {
>                 kfree(mask);
>                 return error;
> @@ -995,35 +1023,33 @@ static int evdev_get_mask(struct evdev_client *client,
>                           int compat)
>  {
>         unsigned long *mask;
> -       size_t cnt, size, min, i;
> -       u8 __user *out;
> +       size_t cnt, size, xfer_size;
> +       int i;
>         int error;
>
>         /* we allow unknown types and 'codes_size > size' for forward-compat */
>         cnt = evdev_get_mask_cnt(type);
>         size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
> -       min = min_t(size_t, codes_size, size);
> +       xfer_size = min_t(size_t, codes_size, size);
>
>         if (cnt > 0) {
>                 mask = client->evmasks[type];
>                 if (mask) {
> -                       error = bits_to_user(mask, cnt - 1, min, codes, compat);
> +                       error = bits_to_user(mask, cnt - 1,
> +                                            xfer_size, codes, compat);
>                         if (error < 0)
>                                 return error;
>                 } else {
>                         /* fake mask with all bits set */
> -                       out = (u8 __user*)codes;
> -                       for (i = 0; i < min; ++i)
> -                               if (put_user((u8)0xff,  out + i))
> +                       for (i = 0; i < xfer_size; i++)
> +                               if (put_user(0xffU, (u8 __user *)codes + i))
>                                         return -EFAULT;
>                 }
>         }
>
> -       codes = (u8*)codes + min;
> -       codes_size -= min;
> -
> -       if (codes_size > 0 && clear_user(codes, codes_size))
> -               return -EFAULT;
> +       if (xfer_size < codes_size)
> +               if (clear_user(codes + xfer_size, codes_size - xfer_size))

'codes' is void*. Want to cast it to u8 first? void* arithmetic is a
gnu extension, iirc. But maybe kernel code doesn't care, not sure.

> +                       return -EFAULT;
>
>         return 0;
>  }
> @@ -1097,27 +1123,29 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>                 else
>                         return evdev_revoke(evdev, client, file);
>
> -       case EVIOCGMASK:
> +       case EVIOCGMASK: {
> +               void __user *codes_ptr;
> +
>                 if (copy_from_user(&mask, p, sizeof(mask)))
>                         return -EFAULT;
>
> +               codes_ptr = (void __user *)(unsigned long)mask.codes_ptr;
>                 return evdev_get_mask(client,
> -                                     mask.type,
> -                                     (void __user*)
> -                                     (unsigned long)mask.codes_ptr,
> -                                     mask.codes_size,
> +                                     mask.type, codes_ptr, mask.codes_size,
>                                       compat_mode);
> +       }
> +
> +       case EVIOCSMASK: {
> +               const void __user *codes_ptr;
>
> -       case EVIOCSMASK:
>                 if (copy_from_user(&mask, p, sizeof(mask)))
>                         return -EFAULT;
>
> +               codes_ptr = (const void __user *)(unsigned long)mask.codes_ptr;
>                 return evdev_set_mask(client,
> -                                     mask.type,
> -                                     (const void __user*)
> -                                     (unsigned long)mask.codes_ptr,
> -                                     mask.codes_size,
> +                                     mask.type, codes_ptr, mask.codes_size,
>                                       compat_mode);
> +       }
>
>         case EVIOCSCLOCKID:
>                 if (copy_from_user(&i, p, sizeof(unsigned int)))

Re: [PATCH] Input: evdev - add event-mask API

[Dmitry Torokhov]

On Sun, Oct 25, 2015 at 10:00:17AM +0100, David Herrmann wrote:
> Hi
> 
> On Sun, Oct 25, 2015 at 2:17 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi David,
> >
> > On Thu, Sep 03, 2015 at 06:14:01PM +0200, David Herrmann wrote:
> >> +static int bits_from_user(unsigned long *bits, unsigned int maxbit,
> >> +                       unsigned int maxlen, const void __user *p, int compat)
> >> +{
> >> +     int len;
> >> +
> >> +#if IS_ENABLED(CONFIG_COMPAT)
> >> +     if (compat) {
> >> +             if (maxlen % sizeof(compat_long_t))
> >> +                     return -EINVAL;
> >> +             len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
> >> +     } else
> >> +#endif
> >> +     {
> >> +             if (maxlen % sizeof(long))
> >> +                     return -EINVAL;
> >> +             len = BITS_TO_LONGS(maxbit) * sizeof(long);
> >> +     }
> >> +
> >> +     if (len > maxlen)
> >> +             len = maxlen;
> >> +
> >> +#if IS_ENABLED(CONFIG_COMPAT) && defined(__BIG_ENDIAN)
> >> +     if (compat) {
> >> +             int i;
> >> +
> >> +             for (i = 0; i < len / sizeof(compat_long_t); i++)
> >> +                     if (copy_from_user((compat_long_t *) bits +
> >> +                                             i + 1 - ((i % 2) << 1),
> >> +                                        (compat_long_t __user *) p + i,
> >> +                                        sizeof(compat_long_t)))
> >> +                             return -EFAULT;
> >> +             if (i % 2)
> >> +                     *((compat_long_t *) bits + i - 1) = 0;
> >> +     } else
> >> +#endif
> >> +             if (copy_from_user(bits, p, len))
> >> +                     return -EFAULT;
> >> +
> >> +     return len;
> >> +}
> >
> > I do not quite like how we sprinkle ifdefs throughout, I prefer the way
> > we have bits_to_user defined, even if it is more verbose.
> 
> Makes sense.
> 
> >> +
> >>  static int str_to_user(const char *str, unsigned int maxlen, void __user *p)
> >>  {
> >>       int len;
> >> @@ -854,6 +953,86 @@ static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
> >>       return 0;
> >>  }
> >>
> >> +/* must be called with evdev-mutex held */
> >> +static int evdev_set_mask(struct evdev_client *client,
> >> +                       unsigned int type,
> >> +                       const void __user *codes,
> >> +                       u32 codes_size,
> >> +                       int compat)
> >> +{
> >> +     unsigned long flags, *mask, *oldmask;
> >> +     size_t cnt, size, min;
> >> +     int error;
> >> +
> >> +     /* we allow unknown types and 'codes_size > size' for forward-compat */
> >> +     cnt = evdev_get_mask_cnt(type);
> >> +     if (!cnt)
> >> +             return 0;
> >> +
> >> +     size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
> >> +     min = min_t(size_t, codes_size, size);
> >> +
> >> +     mask = kzalloc(size, GFP_KERNEL);
> >> +     if (!mask)
> >> +             return -ENOMEM;
> >> +
> >> +     error = bits_from_user(mask, cnt - 1, min, codes, compat);
> >
> > I do not think we need to calculate and pass min here: bits_from_user()
> > will limit the output for us already.
> >
> > Does it still work if you apply the patch below?
> 
> One comment on void*-arithmetic below. Otherwise, this is reviewed and
> tested by me.

Yeah, I am not concerned about using this extension - it is used in
kernel quite often I think and it actually makes sense to me.

Thank you for giving it a spin, I'll fold into original patch and apply.

-- 
Dmitry