[PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case

[PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case

[Aniroop Mathur]

Currently, when EVIOCG[type] ioctl call is issued and bits_to_user fails,
then SYN_DROPPED event is inserted in the event queue always.

However, it is not compulsory that some events are flushed out on every
EVIOCG[type] ioctl call like in case of empty event queue and in case when
EVIOCG[type] ioctl is issued for say A type of events but event queue does
not have any A type of events but some other type of events.

Therefore, insert SYN_DROPPED event only when some events have been flushed
out from event queue plus bits_to_user fails.

Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
---
 drivers/input/evdev.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..f8b295e 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -108,9 +108,11 @@ static bool __evdev_is_filtered(struct evdev_client *client,
 }
 
 /* flush queued events of type @type, caller must hold client->buffer_lock */
-static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
+static unsigned int __evdev_flush_queue(struct evdev_client *client,
+					unsigned int type)
 {
 	unsigned int i, head, num;
+	unsigned int drop_count = 0;
 	unsigned int mask = client->bufsize - 1;
 	bool is_report;
 	struct input_event *ev;
@@ -129,9 +131,11 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
 
 		if (ev->type == type) {
 			/* drop matched entry */
+			drop_count++;
 			continue;
 		} else if (is_report && !num) {
 			/* drop empty SYN_REPORT groups */
+			drop_count++;
 			continue;
 		} else if (head != i) {
 			/* move entry to fill the gap */
@@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
 	}
 
 	client->head = head;
+	return drop_count;
 }
 
 static void __evdev_queue_syn_dropped(struct evdev_client *client)
@@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
 	int ret;
 	unsigned long *mem;
 	size_t len;
+	unsigned int drop_count = 0;
 
 	len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
 	mem = kmalloc(len, GFP_KERNEL);
@@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client *client,
 
 	spin_unlock(&dev->event_lock);
 
-	__evdev_flush_queue(client, type);
+	drop_count = __evdev_flush_queue(client, type);
 
 	spin_unlock_irq(&client->buffer_lock);
 
 	ret = bits_to_user(mem, maxbit, maxlen, p, compat);
-	if (ret < 0)
+	if (ret < 0 && drop_count > 0)
 		evdev_queue_syn_dropped(client);
 
 	kfree(mem);
-- 
2.6.2

Re: [PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case

[Aniroop Mathur]

Hello Mr. Torokhov,

Would you kindly update about this patch?
Thanks!

Best Regards,
Aniroop Mathur


On Fri, Nov 25, 2016 at 1:41 AM, Aniroop Mathur <a.mathur@samsung.com> wrote:
> Currently, when EVIOCG[type] ioctl call is issued and bits_to_user fails,
> then SYN_DROPPED event is inserted in the event queue always.
>
> However, it is not compulsory that some events are flushed out on every
> EVIOCG[type] ioctl call like in case of empty event queue and in case when
> EVIOCG[type] ioctl is issued for say A type of events but event queue does
> not have any A type of events but some other type of events.
>
> Therefore, insert SYN_DROPPED event only when some events have been flushed
> out from event queue plus bits_to_user fails.
>
> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
> ---
>  drivers/input/evdev.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index e9ae3d5..f8b295e 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -108,9 +108,11 @@ static bool __evdev_is_filtered(struct evdev_client *client,
>  }
>
>  /* flush queued events of type @type, caller must hold client->buffer_lock */
> -static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
> +static unsigned int __evdev_flush_queue(struct evdev_client *client,
> +                                       unsigned int type)
>  {
>         unsigned int i, head, num;
> +       unsigned int drop_count = 0;
>         unsigned int mask = client->bufsize - 1;
>         bool is_report;
>         struct input_event *ev;
> @@ -129,9 +131,11 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>
>                 if (ev->type == type) {
>                         /* drop matched entry */
> +                       drop_count++;
>                         continue;
>                 } else if (is_report && !num) {
>                         /* drop empty SYN_REPORT groups */
> +                       drop_count++;
>                         continue;
>                 } else if (head != i) {
>                         /* move entry to fill the gap */
> @@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>         }
>
>         client->head = head;
> +       return drop_count;
>  }
>
>  static void __evdev_queue_syn_dropped(struct evdev_client *client)
> @@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
>         int ret;
>         unsigned long *mem;
>         size_t len;
> +       unsigned int drop_count = 0;
>
>         len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>         mem = kmalloc(len, GFP_KERNEL);
> @@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client *client,
>
>         spin_unlock(&dev->event_lock);
>
> -       __evdev_flush_queue(client, type);
> +       drop_count = __evdev_flush_queue(client, type);
>
>         spin_unlock_irq(&client->buffer_lock);
>
>         ret = bits_to_user(mem, maxbit, maxlen, p, compat);
> -       if (ret < 0)
> +       if (ret < 0 && drop_count > 0)
>                 evdev_queue_syn_dropped(client);
>
>         kfree(mem);
> --
> 2.6.2
>

Re: [PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case

[Aniroop Mathur]

Dear Mr. Dmitry Torokhov,

Could you please update about this patch?
Thank you.

-- Aniroop Mathur


On Wed, Dec 14, 2016 at 11:54 PM, Aniroop Mathur
<aniroop.mathur@gmail.com> wrote:
> Hello Mr. Torokhov,
>
> Would you kindly update about this patch?
> Thanks!
>
> Best Regards,
> Aniroop Mathur
>
>
> On Fri, Nov 25, 2016 at 1:41 AM, Aniroop Mathur <a.mathur@samsung.com> wrote:
>> Currently, when EVIOCG[type] ioctl call is issued and bits_to_user fails,
>> then SYN_DROPPED event is inserted in the event queue always.
>>
>> However, it is not compulsory that some events are flushed out on every
>> EVIOCG[type] ioctl call like in case of empty event queue and in case when
>> EVIOCG[type] ioctl is issued for say A type of events but event queue does
>> not have any A type of events but some other type of events.
>>
>> Therefore, insert SYN_DROPPED event only when some events have been flushed
>> out from event queue plus bits_to_user fails.
>>
>> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
>> ---
>>  drivers/input/evdev.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index e9ae3d5..f8b295e 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -108,9 +108,11 @@ static bool __evdev_is_filtered(struct evdev_client *client,
>>  }
>>
>>  /* flush queued events of type @type, caller must hold client->buffer_lock */
>> -static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>> +static unsigned int __evdev_flush_queue(struct evdev_client *client,
>> +                                       unsigned int type)
>>  {
>>         unsigned int i, head, num;
>> +       unsigned int drop_count = 0;
>>         unsigned int mask = client->bufsize - 1;
>>         bool is_report;
>>         struct input_event *ev;
>> @@ -129,9 +131,11 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>>
>>                 if (ev->type == type) {
>>                         /* drop matched entry */
>> +                       drop_count++;
>>                         continue;
>>                 } else if (is_report && !num) {
>>                         /* drop empty SYN_REPORT groups */
>> +                       drop_count++;
>>                         continue;
>>                 } else if (head != i) {
>>                         /* move entry to fill the gap */
>> @@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>>         }
>>
>>         client->head = head;
>> +       return drop_count;
>>  }
>>
>>  static void __evdev_queue_syn_dropped(struct evdev_client *client)
>> @@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>         int ret;
>>         unsigned long *mem;
>>         size_t len;
>> +       unsigned int drop_count = 0;
>>
>>         len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>>         mem = kmalloc(len, GFP_KERNEL);
>> @@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>
>>         spin_unlock(&dev->event_lock);
>>
>> -       __evdev_flush_queue(client, type);
>> +       drop_count = __evdev_flush_queue(client, type);
>>
>>         spin_unlock_irq(&client->buffer_lock);
>>
>>         ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>> -       if (ret < 0)
>> +       if (ret < 0 && drop_count > 0)
>>                 evdev_queue_syn_dropped(client);
>>
>>         kfree(mem);
>> --
>> 2.6.2
>>

Re: [PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case

[David Herrmann]

Hi

On Thu, Nov 24, 2016 at 9:11 PM, Aniroop Mathur <a.mathur@samsung.com> wrote:
> Currently, when EVIOCG[type] ioctl call is issued and bits_to_user fails,
> then SYN_DROPPED event is inserted in the event queue always.
>
> However, it is not compulsory that some events are flushed out on every
> EVIOCG[type] ioctl call like in case of empty event queue and in case when
> EVIOCG[type] ioctl is issued for say A type of events but event queue does
> not have any A type of events but some other type of events.
>
> Therefore, insert SYN_DROPPED event only when some events have been flushed
> out from event queue plus bits_to_user fails.
>
> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
> ---
>  drivers/input/evdev.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index e9ae3d5..f8b295e 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -108,9 +108,11 @@ static bool __evdev_is_filtered(struct evdev_client *client,
>  }
>
>  /* flush queued events of type @type, caller must hold client->buffer_lock */
> -static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
> +static unsigned int __evdev_flush_queue(struct evdev_client *client,
> +                                       unsigned int type)
>  {
>         unsigned int i, head, num;
> +       unsigned int drop_count = 0;
>         unsigned int mask = client->bufsize - 1;
>         bool is_report;
>         struct input_event *ev;
> @@ -129,9 +131,11 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>
>                 if (ev->type == type) {
>                         /* drop matched entry */
> +                       drop_count++;
>                         continue;
>                 } else if (is_report && !num) {
>                         /* drop empty SYN_REPORT groups */
> +                       drop_count++;

Dropping an empty report-group should not be considered in
`drop_count` here. Empty report groups carry no information and should
not affect client's state. You should only increment `drop_count` in
the first block.

>                         continue;
>                 } else if (head != i) {
>                         /* move entry to fill the gap */
> @@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>         }
>
>         client->head = head;
> +       return drop_count;
>  }
>
>  static void __evdev_queue_syn_dropped(struct evdev_client *client)
> @@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
>         int ret;
>         unsigned long *mem;
>         size_t len;
> +       unsigned int drop_count = 0;
>
>         len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>         mem = kmalloc(len, GFP_KERNEL);
> @@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client *client,
>
>         spin_unlock(&dev->event_lock);
>
> -       __evdev_flush_queue(client, type);
> +       drop_count = __evdev_flush_queue(client, type);
>
>         spin_unlock_irq(&client->buffer_lock);
>
>         ret = bits_to_user(mem, maxbit, maxlen, p, compat);
> -       if (ret < 0)
> +       if (ret < 0 && drop_count > 0)
>                 evdev_queue_syn_dropped(client);

I don't see the point. If bits_to_user() fails, you get EFAULT.
User-space cannot assume anything is still valid if they get EFAULT.
This is not like ENOMEM or other errors that you can recover from.
EFAULT means _programming_ error, not runtime exception.

IOW, EFAULT is special nearly everywhere in the kernel. Usually,
whenever a syscall returns an error, you can rely on it to not have
modified state. EFAULT is an exception on most paths, since it might
occur when copying over results, and it is overly expensive to handle
EFAULT gracefully there (you'd have to copy _results_ to user-space,
before making them visible to the system).

Long story short: I don't see the point in this patch. This path is
*never* triggered, except if your client is buggy. And even if you
trigger it, placing SYN_DROPPED in the queue does no harm at all.

Care to elaborate why exactly you want this modification?
David

>
>         kfree(mem);
> --
> 2.6.2
>
> --
> 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

RE: Re: [PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case

[Aniroop Mathur]

[-- Attachment #1: Type: text/plain, Size: 6631 bytes --]


On Sun, Jan 22, 2017 at 6:45 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Thu, Nov 24, 2016 at 9:11 PM, Aniroop Mathur <a.mathur@samsung.com> wrote:
>> Currently, when EVIOCG[type] ioctl call is issued and bits_to_user fails,
>> then SYN_DROPPED event is inserted in the event queue always.
>>
>> However, it is not compulsory that some events are flushed out on every
>> EVIOCG[type] ioctl call like in case of empty event queue and in case when
>> EVIOCG[type] ioctl is issued for say A type of events but event queue does
>> not have any A type of events but some other type of events.
>>
>> Therefore, insert SYN_DROPPED event only when some events have been flushed
>> out from event queue plus bits_to_user fails.
>>
>> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
>> ---
>>  drivers/input/evdev.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index e9ae3d5..f8b295e 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -108,9 +108,11 @@ static bool __evdev_is_filtered(struct evdev_client *client,
>>  }
>>
>>  /* flush queued events of type @type, caller must hold client->buffer_lock */
>> -static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>> +static unsigned int __evdev_flush_queue(struct evdev_client *client,
>> +                                       unsigned int type)
>>  {
>>         unsigned int i, head, num;
>> +       unsigned int drop_count = 0;
>>         unsigned int mask = client->bufsize - 1;
>>         bool is_report;
>>         struct input_event *ev;
>> @@ -129,9 +131,11 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>>
>>                 if (ev->type == type) {
>>                         /* drop matched entry */
>> +                       drop_count++;
>>                         continue;
>>                 } else if (is_report && !num) {
>>                         /* drop empty SYN_REPORT groups */
>> +                       drop_count++;
>
> Dropping an empty report-group should not be considered in
> `drop_count` here. Empty report groups carry no information and should
> not affect client's state. You should only increment `drop_count` in
> the first block.
>

All right, I am good with increasing the drop_count for only matched entries.

>>                         continue;
>>                 } else if (head != i) {
>>                         /* move entry to fill the gap */
>> @@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>>         }
>>
>>         client->head = head;
>> +       return drop_count;
>>  }
>>
>>  static void __evdev_queue_syn_dropped(struct evdev_client *client)
>> @@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>         int ret;
>>         unsigned long *mem;
>>         size_t len;
>> +       unsigned int drop_count = 0;
>>
>>         len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>>         mem = kmalloc(len, GFP_KERNEL);
>> @@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>
>>         spin_unlock(&dev->event_lock);
>>
>> -       __evdev_flush_queue(client, type);
>> +       drop_count = __evdev_flush_queue(client, type);
>>
>>         spin_unlock_irq(&client->buffer_lock);
>>
>>         ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>> -       if (ret < 0)
>> +       if (ret < 0 && drop_count > 0)
>>                 evdev_queue_syn_dropped(client);
>
> I don't see the point. If bits_to_user() fails, you get EFAULT.
> User-space cannot assume anything is still valid if they get EFAULT.
> This is not like ENOMEM or other errors that you can recover from.
> EFAULT means _programming_ error, not runtime exception.
>
> IOW, EFAULT is special nearly everywhere in the kernel. Usually,
> whenever a syscall returns an error, you can rely on it to not have
> modified state. EFAULT is an exception on most paths, since it might
> occur when copying over results, and it is overly expensive to handle
> EFAULT gracefully there (you'd have to copy _results_ to user-space,
> before making them visible to the system).
>
> Long story short: I don't see the point in this patch. This path is
> *never* triggered, except if your client is buggy. And even if you
> trigger it, placing SYN_DROPPED in the queue does no harm at all.
>
> Care to elaborate why exactly you want this modification?
> David
>

Sure, I will elaborate for you.
Basically, the bug is that if the last event dropped in the queue is
EV_SYN/SYN_REPORT and next event inserted is EV_SYN/SYN_DROPPED, then the
userspace client will ignore the next complete event packet as per rule
defined in the document although the client should not ignore the events
until EV_SYN/SYN_REPORT because the events for this case are not partial
events but the full packet indeed. So we need to make sure whenever this
happens we immediately insert another EV_SYN/SYN_REPORT after EV_SYN/DROPPED
event so that client do not ignore the next full packet.
I already fixed this bug and you can see the patch here (not submitted yet)
https://patchwork.kernel.org/patch/9362233/

For this patch, we had no problem with the case of kernel buffer overrun and
also had no problem for the case of clock change request, but only had problem
for the case of EVIOCG ioctl call which I have already explained in this patch
description.
In short, if we insert SYN_DROPPED event wrongly then client will ignore
events until SYN_REPORT event which we do not want to happen. So that is
why I want this modification in order to have correct insertion of
SYN_DROPPED event and hence go ahead with another patch I mentioned above.

Next, you have also mentioned that this path is never triggered which I am not
sure of. However, if this path is never triggered then it is best to delete it
to avoid such confusion but I dont think thats a good idea. And if this path
can be triggered rarely (even once) which I believe it can like in case of buggy
client you mentioned or in case of bit flip or for any possible reason, then
we need to make this modification.

--
Aniroop Mathur

>>
>>         kfree(mem);
>> --
>> 2.6.2
>>
>> --
>> 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

RE: Re: [PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case

[Aniroop Mathur]

[-- Attachment #1: Type: text/plain, Size: 6629 bytes --]

On Sun, Jan 22, 2017 at 6:45 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Thu, Nov 24, 2016 at 9:11 PM, Aniroop Mathur <a.mathur@samsung.com> wrote:
>> Currently, when EVIOCG[type] ioctl call is issued and bits_to_user fails,
>> then SYN_DROPPED event is inserted in the event queue always.
>>
>> However, it is not compulsory that some events are flushed out on every
>> EVIOCG[type] ioctl call like in case of empty event queue and in case when
>> EVIOCG[type] ioctl is issued for say A type of events but event queue does
>> not have any A type of events but some other type of events.
>>
>> Therefore, insert SYN_DROPPED event only when some events have been flushed
>> out from event queue plus bits_to_user fails.
>>
>> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
>> ---
>>  drivers/input/evdev.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index e9ae3d5..f8b295e 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -108,9 +108,11 @@ static bool __evdev_is_filtered(struct evdev_client *client,
>>  }
>>
>>  /* flush queued events of type @type, caller must hold client->buffer_lock */
>> -static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>> +static unsigned int __evdev_flush_queue(struct evdev_client *client,
>> +                                       unsigned int type)
>>  {
>>         unsigned int i, head, num;
>> +       unsigned int drop_count = 0;
>>         unsigned int mask = client->bufsize - 1;
>>         bool is_report;
>>         struct input_event *ev;
>> @@ -129,9 +131,11 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>>
>>                 if (ev->type == type) {
>>                         /* drop matched entry */
>> +                       drop_count++;
>>                         continue;
>>                 } else if (is_report && !num) {
>>                         /* drop empty SYN_REPORT groups */
>> +                       drop_count++;
>
> Dropping an empty report-group should not be considered in
> `drop_count` here. Empty report groups carry no information and should
> not affect client's state. You should only increment `drop_count` in
> the first block.
>

All right, I am good with increasing the drop_count for only matched entries.

>>                         continue;
>>                 } else if (head != i) {
>>                         /* move entry to fill the gap */
>> @@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>>         }
>>
>>         client->head = head;
>> +       return drop_count;
>>  }
>>
>>  static void __evdev_queue_syn_dropped(struct evdev_client *client)
>> @@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>         int ret;
>>         unsigned long *mem;
>>         size_t len;
>> +       unsigned int drop_count = 0;
>>
>>         len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>>         mem = kmalloc(len, GFP_KERNEL);
>> @@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>
>>         spin_unlock(&dev->event_lock);
>>
>> -       __evdev_flush_queue(client, type);
>> +       drop_count = __evdev_flush_queue(client, type);
>>
>>         spin_unlock_irq(&client->buffer_lock);
>>
>>         ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>> -       if (ret < 0)
>> +       if (ret < 0 && drop_count > 0)
>>                 evdev_queue_syn_dropped(client);
>
> I don't see the point. If bits_to_user() fails, you get EFAULT.
> User-space cannot assume anything is still valid if they get EFAULT.
> This is not like ENOMEM or other errors that you can recover from.
> EFAULT means _programming_ error, not runtime exception.
>
> IOW, EFAULT is special nearly everywhere in the kernel. Usually,
> whenever a syscall returns an error, you can rely on it to not have
> modified state. EFAULT is an exception on most paths, since it might
> occur when copying over results, and it is overly expensive to handle
> EFAULT gracefully there (you'd have to copy _results_ to user-space,
> before making them visible to the system).
>
> Long story short: I don't see the point in this patch. This path is
> *never* triggered, except if your client is buggy. And even if you
> trigger it, placing SYN_DROPPED in the queue does no harm at all.
>
> Care to elaborate why exactly you want this modification?
> David
>

Sure, I will elaborate for you.
Basically, the bug is that if the last event dropped in the queue is
EV_SYN/SYN_REPORT and next event inserted is EV_SYN/SYN_DROPPED, then the
userspace client will ignore the next complete event packet as per rule
defined in the document although the client should not ignore the events
until EV_SYN/SYN_REPORT because the events for this case are not partial
events but the full packet indeed. So we need to make sure whenever this
happens we immediately insert another EV_SYN/SYN_REPORT after EV_SYN/DROPPED
event so that client do not ignore the next full packet.
I already fixed this bug and you can see the patch here (not submitted yet)
https://patchwork.kernel.org/patch/9362233/

For this patch, we had no problem with the case of kernel buffer overrun and
also had no problem for the case of clock change request, but only had problem
for the case of EVIOCG ioctl call which I have already explained in this patch
description.
In short, if we insert SYN_DROPPED event wrongly then client will ignore
events until SYN_REPORT event which we do not want to happen. So that is
why I want this modification in order to have correct insertion of
SYN_DROPPED event and hence go ahead with another patch I mentioned above.

Next, you have also mentioned that this path is never triggered which I am not
sure of. However, if this path is never triggered then it is best to delete it
to avoid such confusion but I dont think thats a good idea. And if this path
can be triggered rarely (even once) which I believe it can like in case of buggy
client you mentioned or in case of bit flip or for any possible reason, then
we need to make this modification.

--
Aniroop Mathur

>>
>>         kfree(mem);
>> --
>> 2.6.2
>>
>> --
>> 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

Re: Re: [PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case

[David Herrmann]

Hi

On Tue, Jan 31, 2017 at 5:09 PM, Aniroop Mathur <a.mathur@samsung.com> wrote:
> On Sun, Jan 22, 2017 at 6:45 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> On Thu, Nov 24, 2016 at 9:11 PM, Aniroop Mathur <a.mathur@samsung.com> wrote:
>>>                         continue;
>>>                 } else if (head != i) {
>>>                         /* move entry to fill the gap */
>>> @@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>>>         }
>>>
>>>         client->head = head;
>>> +       return drop_count;
>>>  }
>>>
>>>  static void __evdev_queue_syn_dropped(struct evdev_client *client)
>>> @@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>>         int ret;
>>>         unsigned long *mem;
>>>         size_t len;
>>> +       unsigned int drop_count = 0;
>>>
>>>         len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>>>         mem = kmalloc(len, GFP_KERNEL);
>>> @@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>>
>>>         spin_unlock(&dev->event_lock);
>>>
>>> -       __evdev_flush_queue(client, type);
>>> +       drop_count = __evdev_flush_queue(client, type);
>>>
>>>         spin_unlock_irq(&client->buffer_lock);
>>>
>>>         ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>>> -       if (ret < 0)
>>> +       if (ret < 0 && drop_count > 0)
>>>                 evdev_queue_syn_dropped(client);
>>
>> I don't see the point. If bits_to_user() fails, you get EFAULT.
>> User-space cannot assume anything is still valid if they get EFAULT.
>> This is not like ENOMEM or other errors that you can recover from.
>> EFAULT means _programming_ error, not runtime exception.
>>
>> IOW, EFAULT is special nearly everywhere in the kernel. Usually,
>> whenever a syscall returns an error, you can rely on it to not have
>> modified state. EFAULT is an exception on most paths, since it might
>> occur when copying over results, and it is overly expensive to handle
>> EFAULT gracefully there (you'd have to copy _results_ to user-space,
>> before making them visible to the system).
>>
>> Long story short: I don't see the point in this patch. This path is
>> *never* triggered, except if your client is buggy. And even if you
>> trigger it, placing SYN_DROPPED in the queue does no harm at all.
>>
>> Care to elaborate why exactly you want this modification?
>> David
>>
>
> Sure, I will elaborate for you.
> Basically, the bug is that if the last event dropped in the queue is
> EV_SYN/SYN_REPORT and next event inserted is EV_SYN/SYN_DROPPED, then the
> userspace client will ignore the next complete event packet as per rule
> defined in the document although the client should not ignore the events
> until EV_SYN/SYN_REPORT because the events for this case are not partial
> events but the full packet indeed. So we need to make sure whenever this
> happens we immediately insert another EV_SYN/SYN_REPORT after EV_SYN/DROPPED
> event so that client do not ignore the next full packet.
> I already fixed this bug and you can see the patch here (not submitted yet)
> https://patchwork.kernel.org/patch/9362233/
>
> For this patch, we had no problem with the case of kernel buffer overrun and
> also had no problem for the case of clock change request, but only had problem
> for the case of EVIOCG ioctl call which I have already explained in this patch
> description.
> In short, if we insert SYN_DROPPED event wrongly then client will ignore
> events until SYN_REPORT event which we do not want to happen. So that is
> why I want this modification in order to have correct insertion of
> SYN_DROPPED event and hence go ahead with another patch I mentioned above.

Fair enough. A SYN_DROPPED should be followed by a SYN_REPORT. The
normal insertion path guarantees that (since it keeps the last event
alive), the other 2 fake SYN_DROPPED insertions don't. But...

> Next, you have also mentioned that this path is never triggered which I am not
> sure of. However, if this path is never triggered then it is best to delete it
> to avoid such confusion but I dont think thats a good idea. And if this path
> can be triggered rarely (even once) which I believe it can like in case of buggy
> client you mentioned or in case of bit flip or for any possible reason, then
> we need to make this modification.

...you seem to misunderstand when this code-path is triggered. This is
an EFAULT handler. So it is only triggered if user-space is buggy
(which the kernel *must* handle gracefully in some regard). That is,
your application will never ever trigger this code-path, unless you're
doing something wrong. But this does not imply that we can ignore this
scenario. The kernel must be prepared to handle buggy applications.

However, we can of course reason about what to do in that case. The
original idea was that if user-space passes incorrect buffers to
EVIOCG* it will be unable to access the events we already flushed.
Hence, we queued SYN_DROPPED to make them realize that. But this seems
counter-intuitive. EFAULT is a hint that user-space passed wrong
pointers, but it is not guaranteed. We might just end up copying into
valid memory, and never realize that user-space passed wrong pointers.
Sure, this ignores that user-space could rely on EFAULT when passing
NULL, but that sounds overly pedantic.
If any user-space continues after getting EFAULT, they must recover by
resyncing, anyway. So the SYN_DROPPED is nothing but cosmetics.

Long story short, I am completely fine with something like this:

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d500a55..28bac2df2982 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -179,15 +179,6 @@
        }
 }

-static void evdev_queue_syn_dropped(struct evdev_client *client)
-{
-       unsigned long flags;
-
-       spin_lock_irqsave(&client->buffer_lock, flags);
-       __evdev_queue_syn_dropped(client);
-       spin_unlock_irqrestore(&client->buffer_lock, flags);
-}
-
 static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
 {
        unsigned long flags;
@@ -938,11 +929,7 @@
        spin_unlock_irq(&client->buffer_lock);

        ret = bits_to_user(mem, maxbit, maxlen, p, compat);
-       if (ret < 0)
-               evdev_queue_syn_dropped(client);
-
        kfree(mem);
-
        return ret;
 }

RE: Re: Re: [PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case

[Aniroop Mathur]

[-- Attachment #1: Type: text/plain, Size: 7366 bytes --]

> Hi
> 
> On Tue, Jan 31, 2017 at 5:09 PM, Aniroop Mathur <a.mathur@samsung.com> wrote:
>> On Sun, Jan 22, 2017 at 6:45 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>>> On Thu, Nov 24, 2016 at 9:11 PM, Aniroop Mathur <a.mathur@samsung.com> wrote:
>>>>                         continue;
>>>>                 } else if (head != i) {
>>>>                         /* move entry to fill the gap */
>>>> @@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>>>>         }
>>>>
>>>>         client->head = head;
>>>> +       return drop_count;
>>>>  }
>>>>
>>>>  static void __evdev_queue_syn_dropped(struct evdev_client *client)
>>>> @@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>>>         int ret;
>>>>         unsigned long *mem;
>>>>         size_t len;
>>>> +       unsigned int drop_count = 0;
>>>>
>>>>         len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>>>>         mem = kmalloc(len, GFP_KERNEL);
>>>> @@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>>>
>>>>         spin_unlock(&dev->event_lock);
>>>>
>>>> -       __evdev_flush_queue(client, type);
>>>> +       drop_count = __evdev_flush_queue(client, type);
>>>>
>>>>         spin_unlock_irq(&client->buffer_lock);
>>>>
>>>>         ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>>>> -       if (ret < 0)
>>>> +       if (ret < 0 && drop_count > 0)
>>>>                 evdev_queue_syn_dropped(client);
>>>
>>> I don't see the point. If bits_to_user() fails, you get EFAULT.
>>> User-space cannot assume anything is still valid if they get EFAULT.
>>> This is not like ENOMEM or other errors that you can recover from.
>>> EFAULT means _programming_ error, not runtime exception.
>>>
>>> IOW, EFAULT is special nearly everywhere in the kernel. Usually,
>>> whenever a syscall returns an error, you can rely on it to not have
>>> modified state. EFAULT is an exception on most paths, since it might
>>> occur when copying over results, and it is overly expensive to handle
>>> EFAULT gracefully there (you'd have to copy _results_ to user-space,
>>> before making them visible to the system).
>>>
>>> Long story short: I don't see the point in this patch. This path is
>>> *never* triggered, except if your client is buggy. And even if you
>>> trigger it, placing SYN_DROPPED in the queue does no harm at all.
>>>
>>> Care to elaborate why exactly you want this modification?
>>> David
>>>
>>
>> Sure, I will elaborate for you.
>> Basically, the bug is that if the last event dropped in the queue is
>> EV_SYN/SYN_REPORT and next event inserted is EV_SYN/SYN_DROPPED, then the
>> userspace client will ignore the next complete event packet as per rule
>> defined in the document although the client should not ignore the events
>> until EV_SYN/SYN_REPORT because the events for this case are not partial
>> events but the full packet indeed. So we need to make sure whenever this
>> happens we immediately insert another EV_SYN/SYN_REPORT after EV_SYN/DROPPED
>> event so that client do not ignore the next full packet.
>> I already fixed this bug and you can see the patch here (not submitted yet)
>> https://patchwork.kernel.org/patch/9362233/
>>
>> For this patch, we had no problem with the case of kernel buffer overrun and
>> also had no problem for the case of clock change request, but only had problem
>> for the case of EVIOCG ioctl call which I have already explained in this patch
>> description.
>> In short, if we insert SYN_DROPPED event wrongly then client will ignore
>> events until SYN_REPORT event which we do not want to happen. So that is
>> why I want this modification in order to have correct insertion of
>> SYN_DROPPED event and hence go ahead with another patch I mentioned above.
>  
> Fair enough. A SYN_DROPPED should be followed by a SYN_REPORT. The
> normal insertion path guarantees that (since it keeps the last event
> alive), the other 2 fake SYN_DROPPED insertions don't. But...
>

the other 2?
Anyways, only problematic case for SYN_DROPPED event was EVIOCG[*] ioctl,
for which you mentioned below that it is completely fine to not add
SYN_DROPPED event when EFAULT occurs which seems good to me too, so I
think this case is resolved.

Thank you for checking this issue and reviewing the patch!

>> Next, you have also mentioned that this path is never triggered which I am not
>> sure of. However, if this path is never triggered then it is best to delete it
>> to avoid such confusion but I dont think thats a good idea. And if this path
>> can be triggered rarely (even once) which I believe it can like in case of buggy
>> client you mentioned or in case of bit flip or for any possible reason, then
>> we need to make this modification.
> 
> ...you seem to misunderstand when this code-path is triggered. This is
> an EFAULT handler. So it is only triggered if user-space is buggy
> (which the kernel *must* handle gracefully in some regard). That is,
> your application will never ever trigger this code-path, unless you're
> doing something wrong. But this does not imply that we can ignore this
> scenario. The kernel must be prepared to handle buggy applications.
>  
> However, we can of course reason about what to do in that case. The
> original idea was that if user-space passes incorrect buffers to
> EVIOCG* it will be unable to access the events we already flushed.
> Hence, we queued SYN_DROPPED to make them realize that. But this seems
> counter-intuitive. EFAULT is a hint that user-space passed wrong
> pointers, but it is not guaranteed. We might just end up copying into
> valid memory, and never realize that user-space passed wrong pointers.
> Sure, this ignores that user-space could rely on EFAULT when passing
> NULL, but that sounds overly pedantic.
> If any user-space continues after getting EFAULT, they must recover by
> resyncing, anyway. So the SYN_DROPPED is nothing but cosmetics.
>  
> Long story short, I am completely fine with something like this:
> 

@Mr. Dmitry Torokhov,
If you are satisfied with the change then could you please apply
this patch and another patch? (may together as a single patch)

Another patch: https://patchwork.kernel.org/patch/9362233/
(fix bug of dropping valid packet after syn_dropped event)

--
Aniroop Mathur

> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index e9ae3d500a55..28bac2df2982 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -179,15 +179,6 @@
>         }
>  }
>  
> -static void evdev_queue_syn_dropped(struct evdev_client *client)
> -{
> -       unsigned long flags;
> -
> -       spin_lock_irqsave(&client->buffer_lock, flags);
> -       __evdev_queue_syn_dropped(client);
> -       spin_unlock_irqrestore(&client->buffer_lock, flags);
> -}
> -
>  static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
>  {
>         unsigned long flags;
> @@ -938,11 +929,7 @@
>         spin_unlock_irq(&client->buffer_lock);
>  
>         ret = bits_to_user(mem, maxbit, maxlen, p, compat);
> -       if (ret < 0)
> -               evdev_queue_syn_dropped(client);
> -
>         kfree(mem);
> -
>         return ret;
>  }
>  
>  

Re: Re: Re: [PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case

[Aniroop Mathur]

On Fri, Feb 24, 2017 at 5:06 PM, Aniroop Mathur <a.mathur@samsung.com> wrote:
>> Hi
>>
>> On Tue, Jan 31, 2017 at 5:09 PM, Aniroop Mathur <a.mathur@samsung.com> wrote:
>>> On Sun, Jan 22, 2017 at 6:45 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>>>> On Thu, Nov 24, 2016 at 9:11 PM, Aniroop Mathur <a.mathur@samsung.com> wrote:
>>>>>                         continue;
>>>>>                 } else if (head != i) {
>>>>>                         /* move entry to fill the gap */
>>>>> @@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>>>>>         }
>>>>>
>>>>>         client->head = head;
>>>>> +       return drop_count;
>>>>>  }
>>>>>
>>>>>  static void __evdev_queue_syn_dropped(struct evdev_client *client)
>>>>> @@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>>>>         int ret;
>>>>>         unsigned long *mem;
>>>>>         size_t len;
>>>>> +       unsigned int drop_count = 0;
>>>>>
>>>>>         len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>>>>>         mem = kmalloc(len, GFP_KERNEL);
>>>>> @@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>>>>
>>>>>         spin_unlock(&dev->event_lock);
>>>>>
>>>>> -       __evdev_flush_queue(client, type);
>>>>> +       drop_count = __evdev_flush_queue(client, type);
>>>>>
>>>>>         spin_unlock_irq(&client->buffer_lock);
>>>>>
>>>>>         ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>>>>> -       if (ret < 0)
>>>>> +       if (ret < 0 && drop_count > 0)
>>>>>                 evdev_queue_syn_dropped(client);
>>>>
>>>> I don't see the point. If bits_to_user() fails, you get EFAULT.
>>>> User-space cannot assume anything is still valid if they get EFAULT.
>>>> This is not like ENOMEM or other errors that you can recover from.
>>>> EFAULT means _programming_ error, not runtime exception.
>>>>
>>>> IOW, EFAULT is special nearly everywhere in the kernel. Usually,
>>>> whenever a syscall returns an error, you can rely on it to not have
>>>> modified state. EFAULT is an exception on most paths, since it might
>>>> occur when copying over results, and it is overly expensive to handle
>>>> EFAULT gracefully there (you'd have to copy _results_ to user-space,
>>>> before making them visible to the system).
>>>>
>>>> Long story short: I don't see the point in this patch. This path is
>>>> *never* triggered, except if your client is buggy. And even if you
>>>> trigger it, placing SYN_DROPPED in the queue does no harm at all.
>>>>
>>>> Care to elaborate why exactly you want this modification?
>>>> David
>>>>
>>>
>>> Sure, I will elaborate for you.
>>> Basically, the bug is that if the last event dropped in the queue is
>>> EV_SYN/SYN_REPORT and next event inserted is EV_SYN/SYN_DROPPED, then the
>>> userspace client will ignore the next complete event packet as per rule
>>> defined in the document although the client should not ignore the events
>>> until EV_SYN/SYN_REPORT because the events for this case are not partial
>>> events but the full packet indeed. So we need to make sure whenever this
>>> happens we immediately insert another EV_SYN/SYN_REPORT after EV_SYN/DROPPED
>>> event so that client do not ignore the next full packet.
>>> I already fixed this bug and you can see the patch here (not submitted yet)
>>> https://patchwork.kernel.org/patch/9362233/
>>>
>>> For this patch, we had no problem with the case of kernel buffer overrun and
>>> also had no problem for the case of clock change request, but only had problem
>>> for the case of EVIOCG ioctl call which I have already explained in this patch
>>> description.
>>> In short, if we insert SYN_DROPPED event wrongly then client will ignore
>>> events until SYN_REPORT event which we do not want to happen. So that is
>>> why I want this modification in order to have correct insertion of
>>> SYN_DROPPED event and hence go ahead with another patch I mentioned above.
>>
>> Fair enough. A SYN_DROPPED should be followed by a SYN_REPORT. The
>> normal insertion path guarantees that (since it keeps the last event
>> alive), the other 2 fake SYN_DROPPED insertions don't. But...
>>
>
> the other 2?
> Anyways, only problematic case for SYN_DROPPED event was EVIOCG[*] ioctl,
> for which you mentioned below that it is completely fine to not add
> SYN_DROPPED event when EFAULT occurs which seems good to me too, so I
> think this case is resolved.
>
> Thank you for checking this issue and reviewing the patch!
>
>>> Next, you have also mentioned that this path is never triggered which I am not
>>> sure of. However, if this path is never triggered then it is best to delete it
>>> to avoid such confusion but I dont think thats a good idea. And if this path
>>> can be triggered rarely (even once) which I believe it can like in case of buggy
>>> client you mentioned or in case of bit flip or for any possible reason, then
>>> we need to make this modification.
>>
>> ...you seem to misunderstand when this code-path is triggered. This is
>> an EFAULT handler. So it is only triggered if user-space is buggy
>> (which the kernel *must* handle gracefully in some regard). That is,
>> your application will never ever trigger this code-path, unless you're
>> doing something wrong. But this does not imply that we can ignore this
>> scenario. The kernel must be prepared to handle buggy applications.
>>
>> However, we can of course reason about what to do in that case. The
>> original idea was that if user-space passes incorrect buffers to
>> EVIOCG* it will be unable to access the events we already flushed.
>> Hence, we queued SYN_DROPPED to make them realize that. But this seems
>> counter-intuitive. EFAULT is a hint that user-space passed wrong
>> pointers, but it is not guaranteed. We might just end up copying into
>> valid memory, and never realize that user-space passed wrong pointers.
>> Sure, this ignores that user-space could rely on EFAULT when passing
>> NULL, but that sounds overly pedantic.
>> If any user-space continues after getting EFAULT, they must recover by
>> resyncing, anyway. So the SYN_DROPPED is nothing but cosmetics.
>>
>> Long story short, I am completely fine with something like this:
>>
>

Hello Mr. Torokhov,

Could you kindly update about this request?
Thanks~

> @Mr. Dmitry Torokhov,
> If you are satisfied with the change then could you please apply
> this patch and another patch? (may together as a single patch)
>
> Another patch: https://patchwork.kernel.org/patch/9362233/
> (fix bug of dropping valid packet after syn_dropped event)
>
> --
> Aniroop Mathur
>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index e9ae3d500a55..28bac2df2982 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -179,15 +179,6 @@
>>         }
>>  }
>>
>> -static void evdev_queue_syn_dropped(struct evdev_client *client)
>> -{
>> -       unsigned long flags;
>> -
>> -       spin_lock_irqsave(&client->buffer_lock, flags);
>> -       __evdev_queue_syn_dropped(client);
>> -       spin_unlock_irqrestore(&client->buffer_lock, flags);
>> -}
>> -
>>  static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
>>  {
>>         unsigned long flags;
>> @@ -938,11 +929,7 @@
>>         spin_unlock_irq(&client->buffer_lock);
>>
>>         ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>> -       if (ret < 0)
>> -               evdev_queue_syn_dropped(client);
>> -
>>         kfree(mem);
>> -
>>         return ret;
>>  }
>>
>>