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

[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 <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;
 }
 

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

[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,
Okash Khawaja wrote:
> 
> 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 <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;
>  }
>  

-- 
Your life is like a penny.  You're going to lose it.  The question is:
How do
you spend it?

         John Covici
         covici@ccs.covici.com

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

[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


On 27 Aug 2017 5:46 am, "John Covici" <covici@ccs.covici.com> wrote:

> 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,
> Okash Khawaja wrote:
> >
> > 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 <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;
> >  }
> >
>
> --
> Your life is like a penny.  You're going to lose it.  The question is:
> How do
> you spend it?
>
>          John Covici
>          covici@ccs.covici.com
>

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

[Samuel Thibault]

Hello,

Okash Khawaja, on sam. 26 août 2017 18:15:13 +0100, wrote:
> +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

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

[Okash Khawaja]

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

Hello,

Okash Khawaja, on sam. 26 août 2017 18:15:13 +0100, wrote:
> +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.

[patch v2] 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 <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;
 }
 

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

[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 <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;
 }
 

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

[Samuel Thibault]

Okash Khawaja, on lun. 28 août 2017 23:13:13 +0100, wrote:
> (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 <okash.khawaja@gmail.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  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
The nice thing about Windows is - It does not just crash, it displays a
dialog box and lets you press 'OK' first.
(Arno Schaefer's .sig)