[patch] staging: speakup: fix speakup-r empty line lockup

[Okash Khawaja]

When cursor is at beginning of an empty or whitespace-only line and
speakup-r typed, kernel locks up. This happens because deadlock of in
input_event function over dev->event_lock, as demonstrated by lockdep
logs. The reason for that is speakup simulates a down arrow - because
cursor is at an empty line - while inside key press notifier handler
which is ultimately triggered from input_event function. The simulated
key press leads to input_event being called again, this time under its
own context. So the spinlock is dev->event_lock is acquired while still
being held.

This patch ensures that key press is not simulated from inside key press
notifier handler. Instead it delegates to cursor_timer. It starts the
timer and passes RA_DOWN_ARROW as argument. When timer handler runs and
sees RA_DOWN_ARROW, it will then call kbd_fakekey2(RA_DOWN_ARROW) which
will correctly simulate the keypress inside timer context.

When not inside key press notifier callback, the behaviour will remain
the same as before this patch.

Signed-off-by: Okash Khawaja <***@gmail.com>

---
drivers/staging/speakup/main.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)

--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -1376,6 +1376,8 @@ static void reset_highlight_buffers(stru

static int read_all_key;

+static volatile int in_keyboard_notifier = 0;
+
static void start_read_all_timer(struct vc_data *vc, int command);

enum {
@@ -1408,7 +1410,10 @@ static void read_all_doc(struct vc_data
cursor_track = read_all_mode;
spk_reset_index_count(0);
if (get_sentence_buf(vc, 0) == -1) {
- kbd_fakekey2(vc, RA_DOWN_ARROW);
+ del_timer(&cursor_timer);
+ if (!in_keyboard_notifier)
+ speakup_fake_down_arrow();
+ start_read_all_timer(vc, RA_DOWN_ARROW);
} else {
say_sentence_num(0, 0);
synth_insert_next_index(0);
@@ -2212,8 +2217,10 @@ static int keyboard_notifier_call(struct
int ret = NOTIFY_OK;
static int keycode; /* to hold the current keycode */

+ in_keyboard_notifier = 1;
+
if (vc->vc_mode == KD_GRAPHICS)
- return ret;
+ goto out;

/*
* First, determine whether we are handling a fake keypress on
@@ -2225,7 +2232,7 @@ static int keyboard_notifier_call(struct
*/

if (speakup_fake_key_pressed())
- return ret;
+ goto out;

switch (code) {
case KBD_KEYCODE:
@@ -2266,6 +2273,8 @@ static int keyboard_notifier_call(struct
break;
}
}
+out:
+ in_keyboard_notifier = 0;
return ret;
}

[John Covici]

hmmm, new patch is the same as the old one and so will not work any
better.

On Sat, 26 Aug 2017 13:15:13 -0400,

Post by Okash Khawaja
When cursor is at beginning of an empty or whitespace-only line and
speakup-r typed, kernel locks up. This happens because deadlock of in
input_event function over dev->event_lock, as demonstrated by lockdep
logs. The reason for that is speakup simulates a down arrow - because
cursor is at an empty line - while inside key press notifier handler
which is ultimately triggered from input_event function. The simulated
key press leads to input_event being called again, this time under its
own context. So the spinlock is dev->event_lock is acquired while still
being held.
This patch ensures that key press is not simulated from inside key press
notifier handler. Instead it delegates to cursor_timer. It starts the
timer and passes RA_DOWN_ARROW as argument. When timer handler runs and
sees RA_DOWN_ARROW, it will then call kbd_fakekey2(RA_DOWN_ARROW) which
will correctly simulate the keypress inside timer context.
When not inside key press notifier callback, the behaviour will remain
the same as before this patch.
---
drivers/staging/speakup/main.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -1376,6 +1376,8 @@ static void reset_highlight_buffers(stru
static int read_all_key;
+static volatile int in_keyboard_notifier = 0;
+
static void start_read_all_timer(struct vc_data *vc, int command);
enum {
@@ -1408,7 +1410,10 @@ static void read_all_doc(struct vc_data
cursor_track = read_all_mode;
spk_reset_index_count(0);
if (get_sentence_buf(vc, 0) == -1) {
- kbd_fakekey2(vc, RA_DOWN_ARROW);
+ del_timer(&cursor_timer);
+ if (!in_keyboard_notifier)
+ speakup_fake_down_arrow();
+ start_read_all_timer(vc, RA_DOWN_ARROW);
} else {
say_sentence_num(0, 0);
synth_insert_next_index(0);
@@ -2212,8 +2217,10 @@ static int keyboard_notifier_call(struct
int ret = NOTIFY_OK;
static int keycode; /* to hold the current keycode */
+ in_keyboard_notifier = 1;
+
if (vc->vc_mode == KD_GRAPHICS)
- return ret;
+ goto out;
/*
* First, determine whether we are handling a fake keypress on
@@ -2225,7 +2232,7 @@ static int keyboard_notifier_call(struct
*/
if (speakup_fake_key_pressed())
- return ret;
+ goto out;
switch (code) {
@@ -2266,6 +2273,8 @@ static int keyboard_notifier_call(struct
break;
}
}
+ in_keyboard_notifier = 0;
return ret;
}

[Okash Khawaja]

Hi John,

Yes it is the same. But it is something we need otherwise the code will
have a path which causes deadlock. My suggestion is we include this patch
in speakup code and deal with incorrect cursor position when control key is
hit, as a separate bug.

Previous email explains the deadlock.

Thanks,
Okash

Post by John Covici
hmmm, new patch is the same as the old one and so will not work any
better.
On Sat, 26 Aug 2017 13:15:13 -0400,

Post by Okash Khawaja
When cursor is at beginning of an empty or whitespace-only line and
speakup-r typed, kernel locks up. This happens because deadlock of in
input_event function over dev->event_lock, as demonstrated by lockdep
logs. The reason for that is speakup simulates a down arrow - because
cursor is at an empty line - while inside key press notifier handler
which is ultimately triggered from input_event function. The simulated
key press leads to input_event being called again, this time under its
own context. So the spinlock is dev->event_lock is acquired while still
being held.
This patch ensures that key press is not simulated from inside key press
notifier handler. Instead it delegates to cursor_timer. It starts the
timer and passes RA_DOWN_ARROW as argument. When timer handler runs and
sees RA_DOWN_ARROW, it will then call kbd_fakekey2(RA_DOWN_ARROW) which
will correctly simulate the keypress inside timer context.
When not inside key press notifier callback, the behaviour will remain
the same as before this patch.
---
drivers/staging/speakup/main.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -1376,6 +1376,8 @@ static void reset_highlight_buffers(stru
static int read_all_key;
+static volatile int in_keyboard_notifier = 0;
+
static void start_read_all_timer(struct vc_data *vc, int command);
enum {
@@ -1408,7 +1410,10 @@ static void read_all_doc(struct vc_data
cursor_track = read_all_mode;
spk_reset_index_count(0);
if (get_sentence_buf(vc, 0) == -1) {
- kbd_fakekey2(vc, RA_DOWN_ARROW);
+ del_timer(&cursor_timer);
+ if (!in_keyboard_notifier)
+ speakup_fake_down_arrow();
+ start_read_all_timer(vc, RA_DOWN_ARROW);
} else {
say_sentence_num(0, 0);
synth_insert_next_index(0);
@@ -2212,8 +2217,10 @@ static int keyboard_notifier_call(struct
int ret = NOTIFY_OK;
static int keycode; /* to hold the current keycode */
+ in_keyboard_notifier = 1;
+
if (vc->vc_mode == KD_GRAPHICS)
- return ret;
+ goto out;
/*
* First, determine whether we are handling a fake keypress on
@@ -2225,7 +2232,7 @@ static int keyboard_notifier_call(struct
*/
if (speakup_fake_key_pressed())
- return ret;
+ goto out;
switch (code) {
@@ -2266,6 +2273,8 @@ static int keyboard_notifier_call(struct
break;
}
}
+ in_keyboard_notifier = 0;
return ret;
}

--
How do
you spend it?
John Covici

[Samuel Thibault]

Hello,

Post by Okash Khawaja
+static volatile int in_keyboard_notifier = 0;

Why "volatile"? Is the read from read_all_doc really done by another
thread?

The presence of volatile is usually very bad sign: either there is
proper synchronization, and volatile is useless, or there isn't, and
volatile just makes code work by luck sometimes only (even if that means
"most of the time", that's not "all the time" :) )

Samuel

[Okash Khawaja]

On 28 Aug 2017 9:17 pm, "Samuel Thibault" <***@ens-lyon.org>
wrote:

Hello,

Post by Okash Khawaja
+static volatile int in_keyboard_notifier = 0;

Why "volatile"? Is the read from read_all_doc really done by another
thread?

The presence of volatile is usually very bad sign: either there is
proper synchronization, and volatile is useless, or there isn't, and
volatile just makes code work by luck sometimes only (even if that means
"most of the time", that's not "all the time" :) )


Thanks for explaining! And indeed it's not read from a different thread so
it's not needed there at all. I'll fix that.

[Okash Khawaja]

When cursor is at beginning of an empty or whitespace-only line and
speakup-r typed, kernel locks up. This happens because deadlock of in
input_event function over dev->event_lock, as demonstrated by lockdep
logs. The reason for that is speakup simulates a down arrow - because
cursor is at an empty line - while inside key press notifier handler
which is ultimately triggered from input_event function. The simulated
key press leads to input_event being called again, this time under its
own context. So the spinlock is dev->event_lock is acquired while still
being held.

This patch ensures that key press is not simulated from inside key press
notifier handler. Instead it delegates to cursor_timer. It starts the
timer and passes RA_DOWN_ARROW as argument. When timer handler runs and
sees RA_DOWN_ARROW, it will then call kbd_fakekey2(RA_DOWN_ARROW) which
will correctly simulate the keypress inside timer context.

When not inside key press notifier callback, the behaviour will remain
the same as before this patch.

Signed-off-by: Okash Khawaja <***@gmail.com>

---
drivers/staging/speakup/main.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)

--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -1376,6 +1376,8 @@ static void reset_highlight_buffers(stru

static int read_all_key;

+static int in_keyboard_notifier = 0;
+
static void start_read_all_timer(struct vc_data *vc, int command);

enum {
@@ -1408,7 +1410,15 @@ static void read_all_doc(struct vc_data
cursor_track = read_all_mode;
spk_reset_index_count(0);
if (get_sentence_buf(vc, 0) == -1) {
- kbd_fakekey2(vc, RA_DOWN_ARROW);
+ del_timer(&cursor_timer);
+ if (!in_keyboard_notifier)
+ speakup_fake_down_arrow();
+ else {
+ pr_warn(">>> read_all_doc: in_interrupt()=%ld\n",
+ in_interrupt());
+ dump_stack();
+ }
+ start_read_all_timer(vc, RA_DOWN_ARROW);
} else {
say_sentence_num(0, 0);
synth_insert_next_index(0);
@@ -2212,8 +2222,10 @@ static int keyboard_notifier_call(struct
int ret = NOTIFY_OK;
static int keycode; /* to hold the current keycode */

+ in_keyboard_notifier = 1;
+
if (vc->vc_mode == KD_GRAPHICS)
- return ret;
+ goto out;

/*
* First, determine whether we are handling a fake keypress on
@@ -2225,7 +2237,7 @@ static int keyboard_notifier_call(struct
*/

if (speakup_fake_key_pressed())
- return ret;
+ goto out;

switch (code) {
case KBD_KEYCODE:
@@ -2266,6 +2278,8 @@ static int keyboard_notifier_call(struct
break;
}
}
+out:
+ in_keyboard_notifier = 0;
return ret;
}

[Okash Khawaja]

(Please ignore patch v2 last sent)

When cursor is at beginning of an empty or whitespace-only line and
speakup-r typed, kernel locks up. This happens because deadlock of in
input_event function over dev->event_lock, as demonstrated by lockdep
logs. The reason for that is speakup simulates a down arrow - because
cursor is at an empty line - while inside key press notifier handler
which is ultimately triggered from input_event function. The simulated
key press leads to input_event being called again, this time under its
own context. So the spinlock is dev->event_lock is acquired while still
being held.

This patch ensures that key press is not simulated from inside key press
notifier handler. Instead it delegates to cursor_timer. It starts the
timer and passes RA_DOWN_ARROW as argument. When timer handler runs and
sees RA_DOWN_ARROW, it will then call kbd_fakekey2(RA_DOWN_ARROW) which
will correctly simulate the keypress inside timer context.

When not inside key press notifier callback, the behaviour will remain
the same as before this patch.

Signed-off-by: Okash Khawaja <***@gmail.com>

---
drivers/staging/speakup/main.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)

--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -1376,6 +1376,8 @@ static void reset_highlight_buffers(stru

static int read_all_key;

+static int in_keyboard_notifier = 0;
+
static void start_read_all_timer(struct vc_data *vc, int command);

enum {
@@ -1408,7 +1410,10 @@ static void read_all_doc(struct vc_data
cursor_track = read_all_mode;
spk_reset_index_count(0);
if (get_sentence_buf(vc, 0) == -1) {
- kbd_fakekey2(vc, RA_DOWN_ARROW);
+ del_timer(&cursor_timer);
+ if (!in_keyboard_notifier)
+ speakup_fake_down_arrow();
+ start_read_all_timer(vc, RA_DOWN_ARROW);
} else {
say_sentence_num(0, 0);
synth_insert_next_index(0);
@@ -2212,8 +2217,10 @@ static int keyboard_notifier_call(struct
int ret = NOTIFY_OK;
static int keycode; /* to hold the current keycode */

+ in_keyboard_notifier = 1;
+
if (vc->vc_mode == KD_GRAPHICS)
- return ret;
+ goto out;

/*
* First, determine whether we are handling a fake keypress on
@@ -2225,7 +2232,7 @@ static int keyboard_notifier_call(struct
*/

if (speakup_fake_key_pressed())
- return ret;
+ goto out;

switch (code) {
case KBD_KEYCODE:
@@ -2266,6 +2273,8 @@ static int keyboard_notifier_call(struct
break;
}
}
+out:
+ in_keyboard_notifier = 0;
return ret;
}

[Samuel Thibault]

Post by Okash Khawaja
(Please ignore patch v2 last sent)
When cursor is at beginning of an empty or whitespace-only line and
speakup-r typed, kernel locks up. This happens because deadlock of in
input_event function over dev->event_lock, as demonstrated by lockdep
logs. The reason for that is speakup simulates a down arrow - because
cursor is at an empty line - while inside key press notifier handler
which is ultimately triggered from input_event function. The simulated
key press leads to input_event being called again, this time under its
own context. So the spinlock is dev->event_lock is acquired while still
being held.
This patch ensures that key press is not simulated from inside key press
notifier handler. Instead it delegates to cursor_timer. It starts the
timer and passes RA_DOWN_ARROW as argument. When timer handler runs and
sees RA_DOWN_ARROW, it will then call kbd_fakekey2(RA_DOWN_ARROW) which
will correctly simulate the keypress inside timer context.
When not inside key press notifier callback, the behaviour will remain
the same as before this patch.
---
drivers/staging/speakup/main.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -1376,6 +1376,8 @@ static void reset_highlight_buffers(stru
static int read_all_key;
+static int in_keyboard_notifier = 0;
+
static void start_read_all_timer(struct vc_data *vc, int command);
enum {
@@ -1408,7 +1410,10 @@ static void read_all_doc(struct vc_data
cursor_track = read_all_mode;
spk_reset_index_count(0);
if (get_sentence_buf(vc, 0) == -1) {
- kbd_fakekey2(vc, RA_DOWN_ARROW);
+ del_timer(&cursor_timer);
+ if (!in_keyboard_notifier)
+ speakup_fake_down_arrow();
+ start_read_all_timer(vc, RA_DOWN_ARROW);
} else {
say_sentence_num(0, 0);
synth_insert_next_index(0);
@@ -2212,8 +2217,10 @@ static int keyboard_notifier_call(struct
int ret = NOTIFY_OK;
static int keycode; /* to hold the current keycode */
+ in_keyboard_notifier = 1;
+
if (vc->vc_mode == KD_GRAPHICS)
- return ret;
+ goto out;
/*
* First, determine whether we are handling a fake keypress on
@@ -2225,7 +2232,7 @@ static int keyboard_notifier_call(struct
*/
if (speakup_fake_key_pressed())
- return ret;
+ goto out;
switch (code) {
@@ -2266,6 +2273,8 @@ static int keyboard_notifier_call(struct
break;
}
}
+ in_keyboard_notifier = 0;
return ret;
}