> Hi
>
> On Tue, Jan 31, 2017 at 5:09 PM, Aniroop Mathur <a.mathur@xxxxxxxxxxx> wrote:
>> On Sun, Jan 22, 2017 at 6:45 PM, David Herrmann <dh.herrmann@xxxxxxxxx> wrote:
>>> On Thu, Nov 24, 2016 at 9:11 PM, Aniroop Mathur <a.mathur@xxxxxxxxxxx> 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;
> }
>
>
