Hi Dmitry

On Thu, Jun 6, 2013 at 9:15 PM, Dmitry Torokhov
[off-list ref] wrote:

Hi David,

On Thu, Mar 28, 2013 at 12:59:40PM +0100, David Herrmann wrote:

quoted

If userspace requests current KEY-state, they very likely assume that no
such events are pending in the output queue of the evdev device.
Otherwise, they will parse events which they already handled via
EVIOCGKEY(). For XKB applications this can cause irreversible keyboard
states if a modifier is locked multiple times because a CTRL-DOWN event is
handled once via EVIOCGKEY() and once from the queue via read(), even
though it should handle it only once.

Therefore, lets do the only logical thing and flush the evdev queue
atomically during this ioctl. We only flush events that are affected by
the given ioctl.

This only affects boolean events like KEY, SND, SW and LED. ABS, REL and
others are not affected as duplicate events can be handled gracefully by
user-space.

Note: This actually breaks semantics of the evdev ABI. However,
investigations showed that userspace already expects the new semantics and
we end up fixing at least all XKB applications.
All applications that are aware of this race-condition mirror the KEY
state for each open-file and detect/drop duplicate events. Hence, they do
not care whether duplicates are posted or not and work fine with this fix.

Also note that we need proper locking to guarantee atomicity and avoid
dead-locks. event_lock must be locked before queue_lock (see input-core).
However, we can safely release event_lock while flushing the queue. This
allows the input-core to proceed with pending events and only stop if it
needs our queue_lock to post new events.
This should guarantee that we don't block event-dispatching for too long
while flushing a single event queue.

Could you please tell me again why we need to take event_lock in
addition to buffer_lock? We only affect the state of one client's buffer
and it seems to be that event_lock alone should be enough. At least it
is enough in read() when we fetching events form the client's queue.

We need the buffer_lock to protect against evdev_fetch_next_event() as
we might modify any part of the buffer in __flush_queue(). We don't
want userspace to get inconsistent data. And we need event_lock to
protect:
  memcpy(mem, bits, sizeof(unsigned long) * max);
Otherwise, input-core might modify the event-bits while we copy them
to our buffer. But we want the function to be atomic, so we cannot
allow modifications during the function-call.

Regards
David