[PATCH 0/2] input: evdev: Fix how SYN_DROPPED is generated

[PATCH 0/2] input: evdev: Fix how SYN_DROPPED is generated

[Kornel Dulęba]

This series fixes two issues discovered while working with a gpio-keys
backed device on Android. In short:

1. When a gpio-keys backed device is opened, its initial state is
   immediately reported.
2. Android issues EVIOCSCLOCKID right after, before reading any data.
3. evdev flushes the queue, enqueues SYN_DROPPED, and caches the current
   time as the device timestamp. Notably, it does not enqueue a
   SYN_REPORT.
4. When the next real event is reported by hardware, it is enqueued with
   the stale cached timestamp and a prepended SYN_DROPPED.
5. The whole packet, up to the first legitimate SYN_REPORT, is then
   ignored by userspace.

SYN_DROPPED needs to be issued after EVIOCSCLOCKID to ensure that any
partially read packet is dropped. At the same time, evdev only defines
the batched events() callback, not the single-event event() one. All
events in a packet up to SYN_REPORT are written to a client's queue at
once while holding client->buffer_lock. The EVIOCSCLOCKID handler takes
the same lock when flushing the queue. Therefore, EVIOCSCLOCKID cannot
inject SYN_DROPPED in the middle of a packet while it is being written
to the queue.

Based on the above, I believe that the best fix here is to emit
SYN_REPORT right after SYN_DROPPED.

Patch 1: Always follow SYN_DROPPED with a SYN_REPORT.
Patch 2: Stamp the SYN_DROPPED+SYN_REPORT events with a timestamp read
         directly from the clock, bypassing the cache.

Signed-off-by: Kornel Dulęba <korneld@google.com>
---
Kornel Dulęba (2):
      input: evdev: Always terminate SYN_DROPPED with SYN_REPORT
      input: evdev: Read clock directly in __evdev_queue_syn_dropped()

 drivers/input/evdev.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)
---
base-commit: 7be4bc52fd9e0e55dc9a2e0232b89427d2c4b614
change-id: 20261022-evdev_syndropped_fix-76352587616b

Best regards,
-- 
Kornel Dulęba <korneld@google.com>

[PATCH 1/2] input: evdev: Always terminate SYN_DROPPED with SYN_REPORT

[Kornel Dulęba]

Per Documentation/input/event-codes.rst, on SYN_DROPPED:
"Client should ignore all events up to and including next SYN_REPORT
event and query the device (using EVIOCG* ioctls) to obtain its
current state."

Currently, __evdev_queue_syn_dropped() queues a standalone SYN_DROPPED
event without a terminating SYN_REPORT. This causes two issues:

1. The queued SYN_DROPPED event remains unreadable until the next
   hardware event frame arrives and finishes with SYN_REPORT.
2. When the next hardware event frame arrives, SYN_DROPPED is prepended
   to it in the same packet. Compliant userspace clients then discard
   the entire hardware event frame up to its SYN_REPORT.

This was discovered on an Android system with a gpio-keys backed device:
1. gpio_keys_open() reports the initial state on open().
2. Android follows open() with EVIOCSCLOCKID.
3. This creates an unterminated SYN_DROPPED, causing the subsequent
   event to be discarded by userspace.

Fix this by appending SYN_REPORT after SYN_DROPPED in
__evdev_queue_syn_dropped() and updating client->packet_head
accordingly.

Signed-off-by: Kornel Dulęba <korneld@google.com>
---
 drivers/input/evdev.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 8bfaaa45e0b9..c6da2601480f 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -147,6 +147,7 @@ static void __evdev_queue_syn_dropped(struct evdev_client *client)
 {
 	ktime_t *ev_time = input_get_timestamp(client->evdev->handle.dev);
 	struct timespec64 ts = ktime_to_timespec64(ev_time[client->clk_type]);
+	unsigned int buf_mask = client->bufsize - 1;
 	struct input_event ev;
 
 	memset(&ev, 0, sizeof(ev));
@@ -156,13 +157,24 @@ static void __evdev_queue_syn_dropped(struct evdev_client *client)
 	ev.code = SYN_DROPPED;
 
 	client->buffer[client->head++] = ev;
-	client->head &= client->bufsize - 1;
+	client->head &= buf_mask;
 
 	if (unlikely(client->head == client->tail)) {
 		/* drop queue but keep our SYN_DROPPED event */
-		client->tail = (client->head - 1) & (client->bufsize - 1);
-		client->packet_head = client->tail;
+		client->tail = (client->head - 1) & buf_mask;
 	}
+
+	ev.code = SYN_REPORT;
+
+	client->buffer[client->head++] = ev;
+	client->head &= buf_mask;
+
+	if (unlikely(client->head == client->tail)) {
+		/* drop queue but keep our SYN_DROPPED and SYN_REPORT events */
+		client->tail = (client->head - 2) & buf_mask;
+	}
+
+	client->packet_head = client->head;
 }
 
 static void evdev_queue_syn_dropped(struct evdev_client *client)

-- 
2.56.0.307.g5fe7bd680c-goog

[PATCH 2/2] input: evdev: Read clock directly in __evdev_queue_syn_dropped()

[Kornel Dulęba]

Currently, __evdev_queue_syn_dropped() calls input_get_timestamp() on
the underlying input_dev to obtain the event timestamp. If no timestamp
is set, input_get_timestamp() caches the current time and returns it;
otherwise, it returns the cached value.

There are two problems with this approach:

1. Locking. The timestamp is obtained in the same way in
   evdev_events(), which is called with dev->event_lock held. Calls to
   __evdev_queue_syn_dropped() are only serialized with
   client->buffer_lock. This makes both accesses likely racy.
2. If no timestamp is cached yet, the subsequent hardware event
   generated after __evdev_queue_syn_dropped() is called will receive
   the stale cached timestamp.

Fix this by reading the current time directly based on the value of
client->clk_type. Open-coding this is not the prettiest solution, but I
couldn't figure out anything better.

I initially thought about fixing this by resetting the input timestamp
in this function instead, similarly to how INPUT_FLUSH handles it.
However, I decided against it, as that would require non-trivial
locking changes.

Signed-off-by: Kornel Dulęba <korneld@google.com>
---
 drivers/input/evdev.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index c6da2601480f..da7396c19188 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -145,11 +145,18 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
 
 static void __evdev_queue_syn_dropped(struct evdev_client *client)
 {
-	ktime_t *ev_time = input_get_timestamp(client->evdev->handle.dev);
-	struct timespec64 ts = ktime_to_timespec64(ev_time[client->clk_type]);
 	unsigned int buf_mask = client->bufsize - 1;
 	struct input_event ev;
+	ktime_t time;
+	struct timespec64 ts;
+
+	time = client->clk_type == INPUT_CLK_REAL ?
+			ktime_get_real() :
+			client->clk_type == INPUT_CLK_MONO ?
+				ktime_get() :
+				ktime_get_boottime();
 
+	ts = ktime_to_timespec64(time);
 	memset(&ev, 0, sizeof(ev));
 	ev.input_event_sec = ts.tv_sec;
 	ev.input_event_usec = ts.tv_nsec / NSEC_PER_USEC;

-- 
2.56.0.307.g5fe7bd680c-goog