linux-kernel - Re: [PATCH v2 4/4] input: evdev: Make device readable only when it contains a complete packet.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BANLkTinLcXsZpHm3_uzRUEdrJQsbozgghA@mail.gmail.com>
Date:	Mon, 4 Apr 2011 17:34:09 -0700
From:	Jeffrey Brown <jeffbrown@...roid.com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	rydberg@...omail.se, djkurtz@...gle.com,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] input: evdev: Make device readable only when it
 contains a complete packet.

Dmitry,

On Mon, Apr 4, 2011 at 3:46 PM, Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
> On Mon, Apr 04, 2011 at 03:16:13PM -0700, Jeffrey Brown wrote:
>> bool full_sync = (type == EV_SYN && code == SYN_REPORT);
>
> I am not sure what is cheaper - 2 conditionals or stack manipulation
> needed to push another argument if we happed to be register-starved.

Not a question of the computational cost.  It was mostly done to avoid
repeating the same predicate in multiple places.  This was one of the
suggested improvements to my earlier patch.

>> This comment for last_syn is not quite right.  We need last_syn to
>> refer to the position just beyond the last sync.  Otherwise the device
>> will not become readable until another event is written there.  The
>> invariants for last_syn should be similar to those for head.
>
> Hm, yes, comment is incorrect. Given this fact I do not like the name
> anymore either (nor do I like 'end'). Need to think about something
> better.

Heh, I faced this very same dilemma.  I tried 'last_sync',
'readable_tail', 'read_end', and others before settling on 'end' and a
descriptive comment.

>> Should use client->head here so that the SYN_DROPPED is readable.
>
> It is readable, but we do not want to signal on it.

I think we do want to signal on it.  We should signal whenever the
device becomes readable.

Signaling on dropped is useful in the case where a misbehaving device
driver fails to ever call input_sync.  If that happens, we might
enqueue a dropped event and then never wake up the client which makes
the issue hard to diagnose.

>> I don't think it's safe to modify last_syn outside of the spin lock.
>> This should be done above.
>
> This is the only writer, plus we are running under event_lock with
> interrupts off, so it is safe.

The value will be read concurrently by evdev_fetch_next_event.  So if
this were safe, then we wouldn't need the spin lock at all.

At the very least for the sake of consistency, I think we should keep
the buffer manipulations within the guarded region.

Jeff.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/