| 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
| ||
|
Message-ID: <58239.81.207.0.53.1185578895.squirrel@secure.samage.net>
Date: Sat, 28 Jul 2007 01:28:15 +0200 (CEST)
From: "Indan Zupancic" <indan@....nu>
To: "Dmitry Torokhov" <dtor@...ightbb.com>
Cc: linux-input@...ey.karlin.mff.cuni.cz, linux-kernel@...r.kernel.org
Subject: Re: [RFC/RFT 1/5] Input: implement proper locking in input core
Hi,
Not real feedback, just some nitpicks.
On Tue, July 24, 2007 06:45, Dmitry Torokhov wrote:
> +static int input_defuzz_abs_event(int value, int old_val, int fuzz)
> +{
> + if (fuzz) {
> + if (value > old_val - fuzz / 2 && value < old_val + fuzz / 2)
> + return value;
>
> - add_input_randomness(type, code, value);
> + if (value > old_val - fuzz && value < old_val + fuzz)
> + return (old_val * 3 + value) / 4;
>
> - switch (type) {
> + if (value > old_val - fuzz * 2 && value < old_val + fuzz * 2)
> + return (old_val + value) / 2;
> + }
Shouldn't the return values of the second and third case be reversed?
In the 2nd check the new values is weighted for 1/4, while in the 3rd
case it counts for 1/2, which breaks the "account new value more when
it is closer to the old one" logic that I thought I saw here. So to sum up,
should the second return be "return (old_val + value * 3) / 4"?
> +/*
> + * Generate software autorepeat event. Note that we take
> + * dev->event_lock here to avoid racing with input_event
> + * which may cause keys get "stuck".
> + */
Hurray. :-)
> - if (code > SW_MAX || !test_bit(code, dev->swbit) || !!test_bit(code, dev->sw) == value)
> - return;
> + if (dev->rep[REP_PERIOD])
> + mod_timer(&dev->timer, jiffies +
> + msecs_to_jiffies(dev->rep[REP_PERIOD]));
> + }
Perhaps use a local var for the "msecs_to_jiffies(dev->rep[REP_PERIOD])" part.
> +static void input_start_autorepeat(struct input_dev *dev, int code)
> +{
> + if (test_bit(EV_REP, dev->evbit) &&
> + dev->rep[REP_PERIOD] && dev->rep[REP_DELAY] &&
> + dev->timer.data) {
> + dev->repeat_key = code;
> + mod_timer(&dev->timer,
> + jiffies + msecs_to_jiffies(dev->rep[REP_DELAY]));
> + }
> +}
Same here.
> + case EV_KEY:
> + if (is_event_supported(code, dev->keybit, KEY_MAX) &&
> + !!test_bit(code, dev->key) != value) {
A bit confusing, test_bit(0 only returns 0 or 1 anyway, doesn't it?
So "test_bit(code, dev->key) != value" should be all right.
I noticed that the old code did it too, but still.
> - case EV_MSC:
> + case EV_SW:
> + if (is_event_supported(code, dev->swbit, SW_MAX) &&
> + !!test_bit(code, dev->sw) != value) {
Same.
> - break;
> + case EV_LED:
> + if (is_event_supported(code, dev->ledbit, LED_MAX) &&
> + !!test_bit(code, dev->led) != value) {
And here.
> +void input_inject_event(struct input_handle *handle,
> + unsigned int type, unsigned int code, int value)
> {
> - struct input_dev *dev = (void *) data;
> + struct input_dev *dev = handle->dev;
> + struct input_handle *grab;
>
> - if (!test_bit(dev->repeat_key, dev->key))
> - return;
> + if (is_event_supported(type, dev->evbit, EV_MAX)) {
> + spin_lock_irq(&dev->event_lock);
>
> - input_event(dev, EV_KEY, dev->repeat_key, 2);
> - input_sync(dev);
> + grab = rcu_dereference(dev->grab);
> + if (!grab || grab == handle)
> + input_handle_event(dev, type, code, value);
'handle' can't be NULL, so can drop the "!grab" check, as checking
"grab == handle" should be sufficient.
> +/**
> + * input_open_device - open input device
> + * @handle: handle through which device is being accessed
> + *
> + * This function should be called by input handlers when they
> + * want to start receive events from given input device.
> + */
> int input_open_device(struct input_handle *handle)
> {
> struct input_dev *dev = handle->dev;
> - int err;
> + int retval;
>
> - err = mutex_lock_interruptible(&dev->mutex);
> - if (err)
> - return err;
> + retval = mutex_lock_interruptible(&dev->mutex);
> + if (retval)
> + return retval;
> +
> + if (dev->going_away) {
> + retval = -ENODEV;
> + goto out;
> + }
>
> handle->open++;
>
> if (!dev->users++ && dev->open)
Ugh, not your code, and perhaps it's me, but that looks weird.
The ++ hidden inthe if check is ugly, and would mean that "users"
can be negative, which is strange.
> - err = dev->open(dev);
> + retval = dev->open(dev);
>
> - if (err)
> - handle->open--;
> + if (retval && !--handle->open) {
Eek! That -- is hidden well there. Would it hurt to call synchronize_sched()
unconditionally? Something like:
if (retval) {
handle->open--;
It's a rare case anyway.
> + /*
> + * Make sure we are not delivering any more events
> + * through this handle
> + */
> + synchronize_sched();
> + }
>
> +/**
> + * input_close_device - close input device
> + * @handle: handle through which device is being accessed
> + *
> + * This function should be called by input handlers when they
> + * want to stop receive events from given input device.
> + */
> void input_close_device(struct input_handle *handle)
> {
> struct input_dev *dev = handle->dev;
>
> - input_release_device(handle);
> -
> mutex_lock(&dev->mutex);
>
> + __input_release_device(handle);
> +
> if (!--dev->users && dev->close)
> dev->close(dev);
> - handle->open--;
> +
> + if (!--handle->open) {
> + /*
> + * synchronize_sched() makes sure that input_pass_event()
> + * completed and that no more input events are delivered
> + * through this handle
> + */
> + synchronize_sched();
> + }
Same here, though just leaving the original "handle->open--;" there and
merely adding the if check would be better too I think. Or just get rid of
the whole if thing.
> static void input_seq_print_bitmap(struct seq_file *seq, const char *name,
> @@ -569,7 +765,9 @@ static const struct file_operations inpu
>
> static void *input_handlers_seq_start(struct seq_file *seq, loff_t *pos)
> {
> - /* acquire lock here ... Yes, we do need locking, I knowi, I know... */
;-)
> + if (mutex_lock_interruptible(&input_mutex))
> + return NULL;
> +
> seq->private = (void *)(unsigned long)*pos;
> return seq_list_start(&input_handler_list, *pos);
> }
Greetings,
Indan
-
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/
Powered by blists - more mailing lists