[PATCH 0/4] speakup: fix hang and use-after-free around synth removal

[PATCH 0/4] speakup: fix hang and use-after-free around synth removal

[Hongyu Xie]

This series fixes a speakup hang reported when a speakup synth module
is unloaded while its output cannot make progress (e.g. a serial
synth whose port has no consumer on the other end, so the tty write
keeps returning 0).  rmmod then stays stuck in D state forever, and
while the loop holds spk_mutex, any synth module load or sysfs synth
switch hangs the same way.

Patch 1 stops the catch-up loops when the synth is not alive anymore.
Patch 2 takes spk_mutex off the catch_up() call path, and makes
synth_release() drain any in-flight catch_up() before releasing the
synth resources, so loading, unloading and switching are no longer
blocked by speech output, while the synth structures stay valid for
the whole call.  It also guards the wake_up_process(speakup_task)
sites against the kthread not being up (or having failed to be
created) yet, pinning the task_struct with rcu_read_lock() so a
concurrent module exit cannot free it mid-wakeup, and closes the
SPEECH_KILL revival hole: spk_synth_is_alive_restart() could set
synth->alive back to 1 from the unguarded key handler after
synth_release() cleared it, which would make the drain wait forever;
the revival is now serialized against the clearing under
speakup_info.spinlock and skipped for a synth being released.

Patch 3 clears the global speakup_kobj after kobject_put() in both
speakup_kobj_init()'s error path and speakup_kobj_exit(): after a
late failure of a built-in speakup_init() the dangling pointer used
to stay behind, and a later insmod of a synth driver would feed it
to sysfs_create_group(); it now fails cleanly on the NULL check.
The removal side gets the same guard: a synth left set by a failed
do_synth_init() makes a later load go through synth_release(), whose
sysfs_remove_group() would now meet a NULL speakup_kobj and crash;
the group removal is skipped when the kobject is gone.

Patch 4 closes a related hole in the keyboard path:
speakup_key() checks the global synth before taking
speakup_info.spinlock, but do_spkup() and its handlers dereference
it inside the lock while synth_release() clears it under spk_mutex;
a speakup key pressed while the synth is being removed could pass
the unlocked check and oops.  The clear is now serialized against
the locked readers: both NULL clears take speakup_info.spinlock,
and every reader that dereferences synth inside that lock now
rechecks it after acquiring it - do_spkup() at entry, the console
notifier paths after their trylock, do_handle_shift(),
do_handle_spec(), the speakup_key() special-handler dispatch, the
synth_direct sysfs store, and the /dev/synth and /dev/synthu write
paths - so a clear can no longer land between a reader's check
and its dereference.

The races the series covers, and the patch that closes each (the
patches show the CPU0/CPU1 interleavings, before and after):

  R1  SPEECH_KILL revival vs. the release drain          patch 2
  R2  wakeup of a not-yet-created or gone kthread        patch 2
  R3  sysfs_create_group() on a freed kobject            patch 3
  R3' sysfs_remove_group() on a NULL kobject             patch 3
  R4  key path check vs. synth clearing                  patch 4
  R5  console path check vs. synth clearing              patch 4
  R6  catch_up() lifetime during unload/switch           patch 2

Patches 1 and 2 are both needed for the no-room hang: patch 1
provides the loop exit but nothing clears synth->alive in that case,
patch 2 provides the deactivation and the drain.  The drain of
patch 2 relies on the loop exit of patch 1 to terminate, so the
order is required, not merely convenient: without patch 1 first,
patch 2 would trade the spk_mutex hang for a new one inside
wait_event().  Patches 3 and 4 are independent fixes found while
reviewing the init/exit and key paths.

Tested on x86_64 (qemu, speakup + speakup_acntsa on a backpressured
serial port): unload, second synth load and sysfs switch all hang on
the unpatched kernel and complete on the patched one; soft synth
operation and a 30-round insmod/write/rmmod stress loop with
interleaved switches show no regression.  The four-patch version
re-ran the same matrix with identical results, and so did the
version extending the in-lock rechecks to the six remaining
reader sites.

While testing the series I noticed a few preexisting issues that are
NOT addressed here (kept out to keep the series focused, can be
follow-ups if wanted):

- the cursor_done timer callback still dereferences synth inside
  speakup_info.spinlock with no synth check at all; closing it
  needs the cursor_timer lifetime handled against synth teardown
  and is left as a follow-up.

- the serial read buffer handler (synth_readbuf_handler) also
  dereferences synth inside the lock unchecked, but cannot run
  in this tree: its only registration chain goes through
  spk_serial_synth_probe(), which has no callers, and any
  out-of-tree registration is serialized by the free_irq() in
  the serial synth release path, which runs before the in-lock
  clear.  Patch 4 discloses this in its commit message.

- speakup_dtlk.c: synth_flush() and synth_read_tts() spin on
  synth_writable()/synth_readable() with no timeout, so the catch-up
  drain of patch 2 could still wait unboundedly if the DoubleTalk
  hardware stops responding mid-flush.

- do_synth_init() does not roll back on a sysfs_create_group()
  failure (the global synth pointer stays set).  Patch 3 keeps that
  dangling synth from crashing on the kobject side, but the synth is
  still not cleaned up; the follow-up should make that error path
  unwind like the probe failure path does.


Hongyu Xie (4):
  speakup: Stop catch_up when the synth is not alive
  speakup: Do not hold spk_mutex across catch_up()
  speakup: Guard late sysfs operations on speakup_kobj
  speakup: Serialize synth clearing against locked readers

 drivers/accessibility/speakup/devsynth.c      |  8 +++
 drivers/accessibility/speakup/kobjects.c      |  7 ++
 drivers/accessibility/speakup/main.c          | 20 ++++++
 .../accessibility/speakup/speakup_acntpc.c    |  4 ++
 .../accessibility/speakup/speakup_apollo.c    |  4 ++
 .../accessibility/speakup/speakup_decext.c    |  4 ++
 drivers/accessibility/speakup/speakup_decpc.c |  4 ++
 .../accessibility/speakup/speakup_dectlk.c    |  4 ++
 drivers/accessibility/speakup/speakup_dtlk.c  |  4 ++
 drivers/accessibility/speakup/speakup_keypc.c |  4 ++
 drivers/accessibility/speakup/spk_priv.h      |  3 +
 drivers/accessibility/speakup/spk_types.h     |  1 +
 drivers/accessibility/speakup/synth.c         | 69 +++++++++++++++++--
 drivers/accessibility/speakup/thread.c        | 11 ++-
 14 files changed, 142 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH 1/4] speakup: Stop catch_up when the synth is not alive

[Hongyu Xie]

When the synth output cannot make progress, the catch-up loop in
_spk_do_catch_up() (and in its local copies in the serial synth
drivers) can spin forever: nothing tells it that the synth is dead.
This happens

- when the tty write keeps returning 0 (no room) because nothing
  consumes the data, e.g. a serial port with no device attached that
  keeps flow control asserted, or
- when an I/O error deactivates the synth: synth_out() then keeps
  returning 0 and the loop never notices that synth->alive became 0.

While speakup_thread() holds spk_mutex across the whole loop, any
synth_add(), synth_remove() or synth_init() then blocks forever,
which gets reported as a hung task on module unload:

 INFO: task modprobe:2999919 blocked for more than 120 seconds.
  mutex_lock+0x54/0x68
  synth_remove+0x28/0xb8 [speakup]
 INFO: task modprobe:2999919 is blocked on a mutex likely owned
  by task speakup:
  schedule_timeout+...
  _spk_do_catch_up+...
  speakup_thread+...

Check synth->alive in the loops as well, so that catch_up() stops as
soon as the synth is dead.  synth_release() writes it under
speakup_info.spinlock, while the I/O error paths write it without the
lock; that is fine, since the loops only need to eventually observe
the change.

On its own this fixes the I/O error case, where the synth is already
deactivated when the loop retries.  In the no-room case nothing
clears synth->alive, and speakup_thread() still holds spk_mutex
across the loop, so nothing can get in to deactivate the synth; the
follow-up patch completes the fix by clearing synth->alive from
synth_release() and by dropping spk_mutex from the catch_up() call.

Fixes: 2067fd92d75b6 ("staging/speakup: Move out of staging")
Cc: stable@vger.kernel.org
Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
---
 drivers/accessibility/speakup/speakup_acntpc.c | 4 ++++
 drivers/accessibility/speakup/speakup_apollo.c | 4 ++++
 drivers/accessibility/speakup/speakup_decext.c | 4 ++++
 drivers/accessibility/speakup/speakup_decpc.c  | 4 ++++
 drivers/accessibility/speakup/speakup_dectlk.c | 4 ++++
 drivers/accessibility/speakup/speakup_dtlk.c   | 4 ++++
 drivers/accessibility/speakup/speakup_keypc.c  | 4 ++++
 drivers/accessibility/speakup/synth.c          | 4 ++++
 8 files changed, 32 insertions(+)

diff --git a/drivers/accessibility/speakup/speakup_acntpc.c b/drivers/accessibility/speakup/speakup_acntpc.c
index a27e6bbf05da3..fe19ba3f00053 100644
--- a/drivers/accessibility/speakup/speakup_acntpc.c
+++ b/drivers/accessibility/speakup/speakup_acntpc.c
@@ -192,6 +192,10 @@ static void do_catch_up(struct spk_synth *synth)
 	jiff_max = jiffies + jiffy_delta_val;
 	while (!kthread_should_stop()) {
 		spin_lock_irqsave(&speakup_info.spinlock, flags);
+		if (!synth->alive) {
+			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+			break;
+		}
 		if (speakup_info.flushing) {
 			speakup_info.flushing = 0;
 			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
diff --git a/drivers/accessibility/speakup/speakup_apollo.c b/drivers/accessibility/speakup/speakup_apollo.c
index d2fbb3f572219..b62696e050fc0 100644
--- a/drivers/accessibility/speakup/speakup_apollo.c
+++ b/drivers/accessibility/speakup/speakup_apollo.c
@@ -157,6 +157,10 @@ static void do_catch_up(struct spk_synth *synth)
 
 	while (!kthread_should_stop()) {
 		spin_lock_irqsave(&speakup_info.spinlock, flags);
+		if (!synth->alive) {
+			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+			break;
+		}
 		jiffy_delta_val = jiffy_delta->u.n.value;
 		full_time_val = full_time->u.n.value;
 		delay_time_val = delay_time->u.n.value;
diff --git a/drivers/accessibility/speakup/speakup_decext.c b/drivers/accessibility/speakup/speakup_decext.c
index 271bcf279bf9b..e6be5a99670bd 100644
--- a/drivers/accessibility/speakup/speakup_decext.c
+++ b/drivers/accessibility/speakup/speakup_decext.c
@@ -171,6 +171,10 @@ static void do_catch_up(struct spk_synth *synth)
 
 	while (!kthread_should_stop()) {
 		spin_lock_irqsave(&speakup_info.spinlock, flags);
+		if (!synth->alive) {
+			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+			break;
+		}
 		if (speakup_info.flushing) {
 			speakup_info.flushing = 0;
 			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
diff --git a/drivers/accessibility/speakup/speakup_decpc.c b/drivers/accessibility/speakup/speakup_decpc.c
index 1bf36d1e54773..cfafd94759639 100644
--- a/drivers/accessibility/speakup/speakup_decpc.c
+++ b/drivers/accessibility/speakup/speakup_decpc.c
@@ -391,6 +391,10 @@ static void do_catch_up(struct spk_synth *synth)
 
 	while (!kthread_should_stop()) {
 		spin_lock_irqsave(&speakup_info.spinlock, flags);
+		if (!synth->alive) {
+			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+			break;
+		}
 		if (speakup_info.flushing) {
 			speakup_info.flushing = 0;
 			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
diff --git a/drivers/accessibility/speakup/speakup_dectlk.c b/drivers/accessibility/speakup/speakup_dectlk.c
index 56334405d865c..d9247c53d3275 100644
--- a/drivers/accessibility/speakup/speakup_dectlk.c
+++ b/drivers/accessibility/speakup/speakup_dectlk.c
@@ -246,6 +246,10 @@ static void do_catch_up(struct spk_synth *synth)
 		spin_unlock_irqrestore(&flush_lock, flags);
 
 		spin_lock_irqsave(&speakup_info.spinlock, flags);
+		if (!synth->alive) {
+			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+			break;
+		}
 		if (speakup_info.flushing) {
 			speakup_info.flushing = 0;
 			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
diff --git a/drivers/accessibility/speakup/speakup_dtlk.c b/drivers/accessibility/speakup/speakup_dtlk.c
index fa826568937bf..73cd39e781b86 100644
--- a/drivers/accessibility/speakup/speakup_dtlk.c
+++ b/drivers/accessibility/speakup/speakup_dtlk.c
@@ -206,6 +206,10 @@ static void do_catch_up(struct spk_synth *synth)
 	jiff_max = jiffies + jiffy_delta_val;
 	while (!kthread_should_stop()) {
 		spin_lock_irqsave(&speakup_info.spinlock, flags);
+		if (!synth->alive) {
+			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+			break;
+		}
 		if (speakup_info.flushing) {
 			speakup_info.flushing = 0;
 			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
diff --git a/drivers/accessibility/speakup/speakup_keypc.c b/drivers/accessibility/speakup/speakup_keypc.c
index 9356f63795603..e229009107f75 100644
--- a/drivers/accessibility/speakup/speakup_keypc.c
+++ b/drivers/accessibility/speakup/speakup_keypc.c
@@ -193,6 +193,10 @@ static void do_catch_up(struct spk_synth *synth)
 	jiff_max = jiffies + jiffy_delta_val;
 	while (!kthread_should_stop()) {
 		spin_lock_irqsave(&speakup_info.spinlock, flags);
+		if (!synth->alive) {
+			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+			break;
+		}
 		if (speakup_info.flushing) {
 			speakup_info.flushing = 0;
 			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
diff --git a/drivers/accessibility/speakup/synth.c b/drivers/accessibility/speakup/synth.c
index 11cba1fd8715d..6a49ff475c16a 100644
--- a/drivers/accessibility/speakup/synth.c
+++ b/drivers/accessibility/speakup/synth.c
@@ -75,6 +75,10 @@ static void _spk_do_catch_up(struct spk_synth *synth, int unicode)
 	jiff_max = jiffies + jiffy_delta_val;
 	while (!kthread_should_stop()) {
 		spin_lock_irqsave(&speakup_info.spinlock, flags);
+		if (!synth->alive) {
+			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+			break;
+		}
 		if (speakup_info.flushing) {
 			speakup_info.flushing = 0;
 			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
-- 
2.25.1

[PATCH 2/4] speakup: Do not hold spk_mutex across catch_up()

[Hongyu Xie]

speakup_thread() holds spk_mutex while calling synth->catch_up().
Apart from the indefinite retry loops fixed by the previous patch, a
synth that digests its buffer slowly (e.g. a real speech synthesizer
receiving a large chunk of text) keeps the mutex held for the whole
drain, so loading, unloading or switching any synth module blocks
until all pending speech has been output, which may take arbitrarily
long.

Take spk_mutex off the catch_up() call path and keep the synth alive
for the duration of the call instead:

- speakup_thread() raises a catch-up count and releases spk_mutex
  before calling catch_up(), re-taking it afterwards.

- synth_release() first clears synth->alive (which makes catch_up()
  return) and wakes the thread out of its sleep, then waits for the
  catch-up count to drop to zero before releasing the synth resources
  (sysfs attributes, variables, tty).  The synth buffer is cleared
  there as well, so a switch discards the text that was still pending
  for the old synth; previously a switch would first wait for that
  text to be output by the old synth, which is the unbounded wait
  this patch removes.  As synth_remove() only returns after the
  drain, a synth module cannot be unloaded while catch_up() still
  runs, so the synth structure and its tty stay valid for the whole
  call without any module reference counting.

The count/wait handoff between the thread and the release looks
like this (without it, the resources could be freed while
catch_up() still runs):

    CPU0 (speakup_thread)          CPU1 (synth_release)
    -------------------            -------------------
    atomic_inc(&synth_catch_up_count)
    mutex_unlock(&spk_mutex)
    synth->catch_up(synth)
      /* reads synth->alive,       spin_lock(&speakup_info.spinlock)
         synth->io_ops, the tty */  synth->alive = 0
                                   spin_unlock(...)
                                   wake the thread out of its sleep
                                   wait_event(!synth_catch_up_count)
                                   /* resources stay valid: sysfs,
                                      vars, tty are torn down only
                                      after the count hits zero */
      /* returns once ->alive
         is 0 (loop exit of the
         previous patch) */
    atomic_dec_and_test(...)
      wake_up(&synth_catch_up_waitq)
                                   sysfs_remove_group(...)
                                   speakup_unregister_var(...)
                                   synth->release(synth)
                                   /* patch 4 clears synth itself
                                      under the spinlock */

The SPEECH_KILL key handler can revive the synth at any time: it
runs under speakup_info.spinlock (speakup_key() holds it across
do_spkup()), and the is_alive callbacks set synth->alive back to 1
and queue the init string again.  If that lands after
synth_release() cleared synth->alive, an in-flight catch_up()
draining to a stuck synth never sees ->alive cleared, and
synth_release() blocks forever on the catch-up count:

    CPU0 (SPEECH_KILL key)         CPU1 (synth_release)
    ----------------------         -------------------
                                   spin_lock(&speakup_info.spinlock)
                                   synth->alive = 0
                                   spin_unlock(...)
    spk_synth_is_alive_restart():
      /* under the same spinlock
         via speakup_key() */
      synth->alive = 1  /* revival */
      /* catch_up() rechecks         wait_event(!synth_catch_up_count)
         ->alive, sees 1 again */    /* never: the revival keeps the
                                         loop running */

Mark the synth with ->releasing under speakup_info.spinlock while it
is being released, and have both spk_synth_is_alive_restart() and
spk_synth_is_alive_nop() skip the revival then, so the drain always
terminates:

    CPU0 (SPEECH_KILL key)         CPU1 (synth_release)
    ----------------------         -------------------
                                   spin_lock(&speakup_info.spinlock)
                                   synth->alive = 0
                                   synth->releasing = true
                                   spin_unlock(...)
    spk_synth_is_alive_restart():
      if (synth->releasing)
        return 0  /* no revival */  wait_event(!synth_catch_up_count)
      /* the catch_up() loop          /* terminates: the loop
         exits on ->alive == 0           observes ->alive == 0 */

The other wake_up_process(speakup_task) sites (spk_do_flush(),
do_synth_init()) get the same guard: the speakup keyboard notifier is
registered before the kthread is created, so a speakup key press or a
sysfs write in that window would otherwise pass NULL (or the
kthread_create() error) to wake_up_process():

    CPU0 (speakup key / sysfs)     CPU1 (module exit)
    --------------------------     -----------------
    spk_do_flush():
      wake_up_process(
        speakup_task)              speakup_exit():
      /* speakup_task is NULL         ... kthread_stop(speakup_task)
         before kthread_create(),        /* task_struct freed */
         ERR_PTR after a failure */   speakup_task = NULL;
      /* wakeup on NULL / ERR_PTR,
         or on freed memory */

The wakeup sites take the task pointer under rcu_read_lock(), so
the task_struct cannot be freed underneath them by a concurrent
module exit either:

    spk_do_flush():
      rcu_read_lock();
      task = READ_ONCE(speakup_task);
      if (!IS_ERR_OR_NULL(task))
        wake_up_process(task);
      rcu_read_unlock();
      /* the exit path unregisters the notifiers first, and
         atomic_notifier_chain_unregister() synchronizes with
         the RCU readers, so by the time speakup_task is
         cleared no wakeup site is still running */

After this change spk_mutex no longer serializes synth registration,
unregistration and switching against speech output.

This depends on the previous patch: the catch-up drain introduced
here assumes that catch_up() eventually returns once synth->alive has
been cleared, including in the local catch_up() copies of the serial
synth drivers.

Fixes: 2067fd92d75b6 ("staging/speakup: Move out of staging")
Cc: stable@vger.kernel.org
Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
---
 drivers/accessibility/speakup/spk_priv.h  |  3 ++
 drivers/accessibility/speakup/spk_types.h |  1 +
 drivers/accessibility/speakup/synth.c     | 59 +++++++++++++++++++++--
 drivers/accessibility/speakup/thread.c    | 11 ++++-
 4 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/drivers/accessibility/speakup/spk_priv.h b/drivers/accessibility/speakup/spk_priv.h
index 9da57ead17cbe..967862cd04403 100644
--- a/drivers/accessibility/speakup/spk_priv.h
+++ b/drivers/accessibility/speakup/spk_priv.h
@@ -75,6 +75,9 @@ struct spk_synth *synth_current(void);
 
 extern struct speakup_info_t speakup_info;
 
+extern atomic_t synth_catch_up_count;
+extern wait_queue_head_t synth_catch_up_waitq;
+
 extern struct var_t synth_time_vars[];
 
 extern struct spk_io_ops spk_serial_io_ops;
diff --git a/drivers/accessibility/speakup/spk_types.h b/drivers/accessibility/speakup/spk_types.h
index a5762330e2495..efb931db18506 100644
--- a/drivers/accessibility/speakup/spk_types.h
+++ b/drivers/accessibility/speakup/spk_types.h
@@ -200,6 +200,7 @@ struct spk_synth {
 	unsigned char (*get_index)(struct spk_synth *synth);
 	struct synth_indexing indexing;
 	int alive;
+	bool releasing;	/* set while synth_release() drains this synth */
 	struct attribute_group attributes;
 
 	void *dev;
diff --git a/drivers/accessibility/speakup/synth.c b/drivers/accessibility/speakup/synth.c
index 6a49ff475c16a..40870949b3c6b 100644
--- a/drivers/accessibility/speakup/synth.c
+++ b/drivers/accessibility/speakup/synth.c
@@ -24,6 +24,14 @@ char spk_pitch_buff[32] = "";
 static int module_status;
 bool spk_quiet_boot;
 
+/*
+ * Number of catch_up() calls currently in flight, and waitqueue used by
+ * synth_release() to wait for them to finish before tearing down the
+ * synth resources they may still be using.
+ */
+atomic_t synth_catch_up_count = ATOMIC_INIT(0);
+DECLARE_WAIT_QUEUE_HEAD(synth_catch_up_waitq);
+
 struct speakup_info_t speakup_info = {
 	/*
 	 * This spinlock is used to protect the entire speakup machinery, and
@@ -154,6 +162,8 @@ EXPORT_SYMBOL_GPL(spk_synth_get_index);
 
 int spk_synth_is_alive_nop(struct spk_synth *synth)
 {
+	if (synth->releasing)
+		return 0;
 	synth->alive = 1;
 	return 1;
 }
@@ -163,6 +173,18 @@ int spk_synth_is_alive_restart(struct spk_synth *synth)
 {
 	if (synth->alive)
 		return 1;
+	/*
+	 * The callers run under speakup_info.spinlock (speakup_key()
+	 * holds it across do_spkup()), which is also the lock
+	 * synth_release() takes to clear ->alive, so checking
+	 * ->releasing here cannot race it: a synth being released is
+	 * not revived, or an in-flight catch_up() draining to a stuck
+	 * synth would never see ->alive cleared and synth_release()
+	 * would block forever on the catch-up count while holding
+	 * spk_mutex.
+	 */
+	if (synth->releasing)
+		return 0;
 	if (synth->io_ops->wait_for_xmitr(synth) > 0) {
 		/* restart */
 		synth->alive = 1;
@@ -197,6 +219,8 @@ void synth_start(void)
 
 void spk_do_flush(void)
 {
+	struct task_struct *task;
+
 	if (!synth)
 		return;
 
@@ -209,7 +233,11 @@ void spk_do_flush(void)
 		}
 	}
 	wake_up_interruptible_all(&speakup_event);
-	wake_up_process(speakup_task);
+	rcu_read_lock();
+	task = READ_ONCE(speakup_task);
+	if (!IS_ERR_OR_NULL(task))
+		wake_up_process(task);
+	rcu_read_unlock();
 }
 
 void synth_write(const char *_buf, size_t count)
@@ -476,13 +504,18 @@ int synth_init(char *synth_name)
 /* called by: synth_add() */
 static int do_synth_init(struct spk_synth *in_synth)
 {
+	struct task_struct *task;
 	struct var_t *var;
+	unsigned long flags;
 
 	synth_release();
 	if (in_synth->checkval != SYNTH_CHECK)
 		return -EINVAL;
 	synth = in_synth;
 	synth->alive = 0;
+	spin_lock_irqsave(&speakup_info.spinlock, flags);
+	synth->releasing = false;
+	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
 	pr_warn("synth probe\n");
 	if (synth->probe(synth) < 0) {
 		pr_warn("%s: device probe failed\n", in_synth->name);
@@ -510,13 +543,17 @@ static int do_synth_init(struct spk_synth *in_synth)
 		return -ENOMEM;
 	synth_flags = synth->flags;
 	wake_up_interruptible_all(&speakup_event);
-	if (speakup_task)
-		wake_up_process(speakup_task);
+	rcu_read_lock();
+	task = READ_ONCE(speakup_task);
+	if (!IS_ERR_OR_NULL(task))
+		wake_up_process(task);
+	rcu_read_unlock();
 	return 0;
 }
 
 void synth_release(void)
 {
+	struct task_struct *task;
 	struct var_t *var;
 	unsigned long flags;
 
@@ -525,8 +562,24 @@ void synth_release(void)
 	spin_lock_irqsave(&speakup_info.spinlock, flags);
 	pr_info("releasing synth %s\n", synth->name);
 	synth->alive = 0;
+	synth->releasing = true;
 	timer_delete(&thread_timer);
+	synth_buffer_clear();
 	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+	/*
+	 * catch_up() may still be running without spk_mutex held (it gets
+	 * taken off the mutex by speakup_thread()).  Wake it out of its
+	 * sleep and wait until it is done before releasing the resources
+	 * (sysfs, variables, tty) it may be accessing.  speakup_task can
+	 * be an ERR_PTR here, when speakup_init() failed after the
+	 * command-line synth_init().
+	 */
+	rcu_read_lock();
+	task = READ_ONCE(speakup_task);
+	if (!IS_ERR_OR_NULL(task))
+		wake_up_process(task);
+	rcu_read_unlock();
+	wait_event(synth_catch_up_waitq, !atomic_read(&synth_catch_up_count));
 	if (synth->attributes.name)
 		sysfs_remove_group(speakup_kobj, &synth->attributes);
 	for (var = synth->vars; var->var_id != MAXVARS; var++)
diff --git a/drivers/accessibility/speakup/thread.c b/drivers/accessibility/speakup/thread.c
index 2fc75e60fbac2..5279e112248b4 100644
--- a/drivers/accessibility/speakup/thread.c
+++ b/drivers/accessibility/speakup/thread.c
@@ -50,9 +50,18 @@ int speakup_thread(void *data)
 		if (synth && synth->catch_up && synth->alive) {
 			/*
 			 * It is up to the callee to take the lock, so that it
-			 * can sleep whenever it likes
+			 * can sleep whenever it likes.  It may indeed sleep for
+			 * a long while, e.g. while draining to a slow synth, so
+			 * run it without spk_mutex: synth_release() waits for
+			 * the catch-up count to drop before freeing the synth,
+			 * hence the synth stays alive for the whole call.
 			 */
+			atomic_inc(&synth_catch_up_count);
+			mutex_unlock(&spk_mutex);
 			synth->catch_up(synth);
+			if (atomic_dec_and_test(&synth_catch_up_count))
+				wake_up(&synth_catch_up_waitq);
+			mutex_lock(&spk_mutex);
 		}
 
 		speakup_start_ttys();
-- 
2.25.1

[PATCH 3/4] speakup: Guard late sysfs operations on speakup_kobj

[Hongyu Xie]

speakup_kobj_init() drops the kobject on its error path and
speakup_kobj_exit() drops it on removal, but neither clears the
global pointer.  When built-in speakup fails late in speakup_init()
(e.g. a kthread_create() failure, whose error path runs
speakup_kobj_exit()), or when the kobject setup itself fails, the
dangling speakup_kobj stays behind: a later insmod of a synth driver
then reaches sysfs_create_group() through synth_add()->
do_synth_init() and dereferences the freed kobject:

    boot (built-in speakup)         later insmod (synth driver)
    --------------------------      ---------------------------
    speakup_init() fails late:
      err_speakup:
        kobject_put(speakup_kobj);
        /* speakup_kobj stays set */

                                    synth_add() -> do_synth_init()
                                      sysfs_create_group(speakup_kobj,
                                                         &attributes)
                                      /* freed kobject: UAF */

Clear the pointer right after kobject_put() so the late
sysfs_create_group() fails cleanly on the NULL check instead of
touching freed memory:

      err_speakup:
        kobject_put(speakup_kobj);
        speakup_kobj = NULL;

                                    sysfs_create_group(NULL, ...)
                                      /* fails cleanly: the NULL
                                         check is sysfs's own */

The removal side needs the same guard: when the kobject setup
failed, a first insmod of a synth driver fails in do_synth_init()
(synth stays set because that error path does not roll it back),
and a second insmod then goes through synth_release(), whose
sysfs_remove_group() would now meet speakup_kobj == NULL and crash
on the NULL kobject:

    first insmod                    second insmod
    -----------                     ------------
    do_synth_init():
      synth = in_synth;
      probe() fails                 synth_release():
      /* no rollback:                sysfs_remove_group(
         synth stays set */            speakup_kobj == NULL, ...)
                                      /* NULL kobject: the crash
                                         the NULL clear above would
                                         itself introduce without
                                         the guard below */

Skip the group removal when the kobject is gone, mirroring the
NULL check that sysfs_create_group() already performs on its own:

                                    if (synth->attributes.name &&
                                        speakup_kobj)
                                      sysfs_remove_group(...)
                                      /* skipped: the kobject is
                                         gone */

Fixes: 2067fd92d75b6 ("staging/speakup: Move out of staging")
Cc: stable@vger.kernel.org
Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
---
 drivers/accessibility/speakup/kobjects.c | 2 ++
 drivers/accessibility/speakup/synth.c    | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/accessibility/speakup/kobjects.c b/drivers/accessibility/speakup/kobjects.c
index a73b478e06de0..e1860f8d9d422 100644
--- a/drivers/accessibility/speakup/kobjects.c
+++ b/drivers/accessibility/speakup/kobjects.c
@@ -1041,6 +1041,7 @@ int speakup_kobj_init(void)
 	sysfs_remove_group(speakup_kobj, &main_attr_group);
 err_speakup:
 	kobject_put(speakup_kobj);
+	speakup_kobj = NULL;
 err_acc:
 	kobject_put(accessibility_kobj);
 out:
@@ -1052,5 +1053,6 @@ void speakup_kobj_exit(void)
 	sysfs_remove_group(speakup_kobj, &i18n_attr_group);
 	sysfs_remove_group(speakup_kobj, &main_attr_group);
 	kobject_put(speakup_kobj);
+	speakup_kobj = NULL;
 	kobject_put(accessibility_kobj);
 }
diff --git a/drivers/accessibility/speakup/synth.c b/drivers/accessibility/speakup/synth.c
index 40870949b3c6b..1af80eb116af6 100644
--- a/drivers/accessibility/speakup/synth.c
+++ b/drivers/accessibility/speakup/synth.c
@@ -580,7 +580,7 @@ void synth_release(void)
 		wake_up_process(task);
 	rcu_read_unlock();
 	wait_event(synth_catch_up_waitq, !atomic_read(&synth_catch_up_count));
-	if (synth->attributes.name)
+	if (synth->attributes.name && speakup_kobj)
 		sysfs_remove_group(speakup_kobj, &synth->attributes);
 	for (var = synth->vars; var->var_id != MAXVARS; var++)
 		speakup_unregister_var(var->var_id);
-- 
2.25.1

[PATCH 4/4] speakup: Serialize synth clearing against locked readers

[Hongyu Xie]

speakup_key() validates the global synth pointer before taking
speakup_info.spinlock, but do_spkup() and its handlers run inside
that lock and dereference synth: speech_kill() calls
synth->is_alive(), and the flush path reaches synth_start()
reading synth->alive.  synth_release() clears synth under
spk_mutex, which does not serialize against the key path, so a
speakup key pressed while the synth is being removed can pass the
unlocked check and oops on the stale pointer:

    CPU0 (speakup key)             CPU1 (rmmod -> synth_release)
    ------------------             ----------------------------
    speakup_key():
      if (!synth)                  mutex_lock(&spk_mutex)
        return 0  /* passes */     synth_release():
    spin_lock(&speakup_info.         synth->alive = 0   (spinlock)
        spinlock)                   ... drain in-flight catch_up() ...
                                    synth->release(synth)
    do_spkup():                     synth = NULL       /* unlocked */
      speech_kill():
        synth->is_alive()
        /* dereferences a synth whose release() has
           already torn down its I/O */

Closing this window needs both sides of the race:

- Recheck synth at do_spkup() entry, inside the lock, so the whole
  handler table runs only while a synth is still registered.
  speakup_con_write() and speakup_con_update() gate on synth
  before their trylock; recheck it after acquiring the lock too,
  so these readers no longer validate outside the critical
  section that dereferences.

- Clear synth under speakup_info.spinlock in synth_release() and
  in the do_synth_init() probe-failure path, so a clear can never
  land between a locked reader's check and its dereference.

With the check and the clear under the same lock they are
mutually exclusive for these paths, instead of merely close
together:

    CPU0 (speakup key)             CPU1 (rmmod -> synth_release)
    ------------------             ----------------------------
    spin_lock(&speakup_info.       ... drain in-flight catch_up() ...
        spinlock)                  spin_lock(&speakup_info.spinlock)
    do_spkup():                    synth = NULL
      if (!synth)                  spin_unlock(...)
        return  /* in-lock recheck */
      speech_kill():
        synth->is_alive()  /* cannot interleave with a clear */

The console notifier paths had the same window between the
pre-trylock check and the in-lock use:

    CPU0 (vt notifier)             CPU1 (rmmod -> synth_release)
    ------------------             ----------------------------
    speakup_con_write():
      if (!synth)                  (the clear lands between
        return  /* pre-trylock */    the pre-trylock check
    spin_trylock(&speakup_info.      and the in-lock use)
        spinlock)
    ... uses synth ...  /* stale */

and now recheck synth after the trylock, in the lock, exactly
like do_spkup().

The remaining locked readers had the same window between an
unlocked check and the in-lock dereference, and gain the same
recheck:

- do_handle_shift() and do_handle_spec() gate on synth before
  taking speakup_info.spinlock and dereference it via
  synth_printf() inside the lock.
- speakup_key()'s special-handler dispatch dereferences synth
  inside the lock speakup_key() holds (handle_goto(),
  edit_bits(), the help handler), past the unlocked entry
  check.
- synth_direct_store() validates synth before the lock while
  its synth_write() call runs inside it.
- the /dev/synth and /dev/synthu write paths validate synth
  once before their copy_from_user() loop while every round
  calls synth_write() (or synth_buffer_add() plus
  synth_start()) inside the lock.

With these rechecks the check and the clear are mutually
exclusive for every reader that dereferences synth inside
speakup_info.spinlock.

The speakup thread keeps its existing fence: synth_release()
drains the catch-up count before clearing.

Two sites of this shape remain, preexisting.  The cursor_done()
timer callback dereferences synth inside the lock with no synth
check at all, and closing it needs the cursor_timer lifetime
handled against synth teardown, which is beyond this series.
The serial read buffer handler (synth_readbuf_handler()) also
dereferences synth inside the lock with no synth check.  It
cannot run in this tree: the only registration chain goes
through spk_serial_synth_probe(), which has no callers, and
any registration from outside the tree is still serialized by
the free_irq() in the serial synth release path, which runs
before the in-lock clear.

Fixes: 2067fd92d75b6 ("staging/speakup: Move out of staging")
Cc: stable@vger.kernel.org
Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
---
 drivers/accessibility/speakup/devsynth.c |  8 ++++++++
 drivers/accessibility/speakup/kobjects.c |  5 +++++
 drivers/accessibility/speakup/main.c     | 20 ++++++++++++++++++++
 drivers/accessibility/speakup/synth.c    |  4 ++++
 4 files changed, 37 insertions(+)

diff --git a/drivers/accessibility/speakup/devsynth.c b/drivers/accessibility/speakup/devsynth.c
index e3d909bd04804..19799a362948d 100644
--- a/drivers/accessibility/speakup/devsynth.c
+++ b/drivers/accessibility/speakup/devsynth.c
@@ -29,6 +29,10 @@ static ssize_t speakup_file_write(struct file *fp, const char __user *buffer,
 		count -= bytes;
 		ptr += bytes;
 		spin_lock_irqsave(&speakup_info.spinlock, flags);
+		if (!synth) {
+			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+			return -ENODEV;
+		}
 		synth_write(buf, bytes);
 		spin_unlock_irqrestore(&speakup_info.spinlock, flags);
 	}
@@ -84,6 +88,10 @@ static ssize_t speakup_file_writeu(struct file *fp, const char __user *buffer,
 		/* And speak this up */
 		if (out) {
 			spin_lock_irqsave(&speakup_info.spinlock, flags);
+			if (!synth) {
+				spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+				return -ENODEV;
+			}
 			for (in = 0; in < out; in++)
 				synth_buffer_add(ubuf[in]);
 			synth_start();
diff --git a/drivers/accessibility/speakup/kobjects.c b/drivers/accessibility/speakup/kobjects.c
index e1860f8d9d422..00e92efa872ca 100644
--- a/drivers/accessibility/speakup/kobjects.c
+++ b/drivers/accessibility/speakup/kobjects.c
@@ -426,6 +426,11 @@ static ssize_t synth_direct_store(struct kobject *kobj,
 	string_unescape_any_inplace(unescaped);
 
 	spin_lock_irqsave(&speakup_info.spinlock, flags);
+	if (!synth) {
+		spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+		kfree(unescaped);
+		return -EPERM;
+	}
 	synth_write(unescaped, strlen(unescaped));
 	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
 
diff --git a/drivers/accessibility/speakup/main.c b/drivers/accessibility/speakup/main.c
index e9b7c2761f6f9..a88b6d66eeb76 100644
--- a/drivers/accessibility/speakup/main.c
+++ b/drivers/accessibility/speakup/main.c
@@ -1169,6 +1169,10 @@ static void do_handle_shift(struct vc_data *vc, u_char value, char up_flag)
 	if (!synth || up_flag || spk_killed)
 		return;
 	spin_lock_irqsave(&speakup_info.spinlock, flags);
+	if (!synth) {
+		spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+		return;
+	}
 	if (cursor_track == read_all_mode) {
 		switch (value) {
 		case KVAL(K_SHIFT):
@@ -1768,6 +1772,10 @@ static void speakup_con_write(struct vc_data *vc, u16 *str, int len)
 	if (!spin_trylock_irqsave(&speakup_info.spinlock, flags))
 		/* Speakup output, discard */
 		return;
+	if (!synth) {
+		spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+		return;
+	}
 	if (spk_bell_pos && spk_keydown && (vc->state.x == spk_bell_pos - 1))
 		bleep(3);
 	if ((is_cursor) || (cursor_track == read_all_mode)) {
@@ -1797,6 +1805,10 @@ static void speakup_con_update(struct vc_data *vc)
 	if (!spin_trylock_irqsave(&speakup_info.spinlock, flags))
 		/* Speakup output, discard */
 		return;
+	if (!synth) {
+		spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+		return;
+	}
 	speakup_date(vc);
 	if (vc->vc_mode == KD_GRAPHICS && !spk_paused && spk_str_pause[0]) {
 		synth_printf("%s", spk_str_pause);
@@ -1814,6 +1826,10 @@ static void do_handle_spec(struct vc_data *vc, u_char value, char up_flag)
 	if (!synth || up_flag || spk_killed)
 		return;
 	spin_lock_irqsave(&speakup_info.spinlock, flags);
+	if (!synth) {
+		spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+		return;
+	}
 	spk_shut_up &= 0xfe;
 	if (spk_no_intr)
 		spk_do_flush();
@@ -2087,6 +2103,8 @@ static spkup_hand spkup_handler[] = {
 
 static void do_spkup(struct vc_data *vc, u_char value)
 {
+	if (!synth)
+		return;
 	if (spk_killed && value != SPEECH_KILL)
 		return;
 	spk_keydown = 0;
@@ -2119,6 +2137,8 @@ speakup_key(struct vc_data *vc, int shift_state, int keycode, u16 keysym,
 		return 0;
 
 	spin_lock_irqsave(&speakup_info.spinlock, flags);
+	if (!synth)
+		goto out;
 	tty = vc->port.tty;
 	if (type >= 0xf0)
 		type -= 0xf0;
diff --git a/drivers/accessibility/speakup/synth.c b/drivers/accessibility/speakup/synth.c
index 1af80eb116af6..aef482f8b14e8 100644
--- a/drivers/accessibility/speakup/synth.c
+++ b/drivers/accessibility/speakup/synth.c
@@ -519,7 +519,9 @@ static int do_synth_init(struct spk_synth *in_synth)
 	pr_warn("synth probe\n");
 	if (synth->probe(synth) < 0) {
 		pr_warn("%s: device probe failed\n", in_synth->name);
+		spin_lock_irqsave(&speakup_info.spinlock, flags);
 		synth = NULL;
+		spin_unlock_irqrestore(&speakup_info.spinlock, flags);
 		return -ENODEV;
 	}
 	synth_time_vars[0].u.n.value =
@@ -585,7 +587,9 @@ void synth_release(void)
 	for (var = synth->vars; var->var_id != MAXVARS; var++)
 		speakup_unregister_var(var->var_id);
 	synth->release(synth);
+	spin_lock_irqsave(&speakup_info.spinlock, flags);
 	synth = NULL;
+	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
 }
 
 /* called by: all_driver_init() */
-- 
2.25.1

Re: [PATCH 1/4] speakup: Stop catch_up when the synth is not alive

[Samuel Thibault]

Hongyu Xie, le mer. 14 oct. 2026 15:17:10 +0800, a ecrit:
> When the synth output cannot make progress, the catch-up loop in
> _spk_do_catch_up() (and in its local copies in the serial synth
> drivers) can spin forever: nothing tells it that the synth is dead.
> This happens
> 
> - when the tty write keeps returning 0 (no room) because nothing
>   consumes the data, e.g. a serial port with no device attached that
>   keeps flow control asserted, or
> - when an I/O error deactivates the synth: synth_out() then keeps
>   returning 0 and the loop never notices that synth->alive became 0.
> 
> While speakup_thread() holds spk_mutex across the whole loop, any
> synth_add(), synth_remove() or synth_init() then blocks forever,
> which gets reported as a hung task on module unload:
> 
>  INFO: task modprobe:2999919 blocked for more than 120 seconds.
>   mutex_lock+0x54/0x68
>   synth_remove+0x28/0xb8 [speakup]
>  INFO: task modprobe:2999919 is blocked on a mutex likely owned
>   by task speakup:
>   schedule_timeout+...
>   _spk_do_catch_up+...
>   speakup_thread+...
> 
> Check synth->alive in the loops as well, so that catch_up() stops as
> soon as the synth is dead.  synth_release() writes it under
> speakup_info.spinlock, while the I/O error paths write it without the
> lock; that is fine, since the loops only need to eventually observe
> the change.
> 
> On its own this fixes the I/O error case, where the synth is already
> deactivated when the loop retries.  In the no-room case nothing
> clears synth->alive, and speakup_thread() still holds spk_mutex
> across the loop, so nothing can get in to deactivate the synth; the
> follow-up patch completes the fix by clearing synth->alive from
> synth_release() and by dropping spk_mutex from the catch_up() call.
> 
> Fixes: 2067fd92d75b6 ("staging/speakup: Move out of staging")
> Cc: stable@vger.kernel.org
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>

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

Thanks!

Samuel

> Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
> ---
>  drivers/accessibility/speakup/speakup_acntpc.c | 4 ++++
>  drivers/accessibility/speakup/speakup_apollo.c | 4 ++++
>  drivers/accessibility/speakup/speakup_decext.c | 4 ++++
>  drivers/accessibility/speakup/speakup_decpc.c  | 4 ++++
>  drivers/accessibility/speakup/speakup_dectlk.c | 4 ++++
>  drivers/accessibility/speakup/speakup_dtlk.c   | 4 ++++
>  drivers/accessibility/speakup/speakup_keypc.c  | 4 ++++
>  drivers/accessibility/speakup/synth.c          | 4 ++++
>  8 files changed, 32 insertions(+)
> 
> diff --git a/drivers/accessibility/speakup/speakup_acntpc.c b/drivers/accessibility/speakup/speakup_acntpc.c
> index a27e6bbf05da3..fe19ba3f00053 100644
> --- a/drivers/accessibility/speakup/speakup_acntpc.c
> +++ b/drivers/accessibility/speakup/speakup_acntpc.c
> @@ -192,6 +192,10 @@ static void do_catch_up(struct spk_synth *synth)
>  	jiff_max = jiffies + jiffy_delta_val;
>  	while (!kthread_should_stop()) {
>  		spin_lock_irqsave(&speakup_info.spinlock, flags);
> +		if (!synth->alive) {
> +			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +			break;
> +		}
>  		if (speakup_info.flushing) {
>  			speakup_info.flushing = 0;
>  			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> diff --git a/drivers/accessibility/speakup/speakup_apollo.c b/drivers/accessibility/speakup/speakup_apollo.c
> index d2fbb3f572219..b62696e050fc0 100644
> --- a/drivers/accessibility/speakup/speakup_apollo.c
> +++ b/drivers/accessibility/speakup/speakup_apollo.c
> @@ -157,6 +157,10 @@ static void do_catch_up(struct spk_synth *synth)
>  
>  	while (!kthread_should_stop()) {
>  		spin_lock_irqsave(&speakup_info.spinlock, flags);
> +		if (!synth->alive) {
> +			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +			break;
> +		}
>  		jiffy_delta_val = jiffy_delta->u.n.value;
>  		full_time_val = full_time->u.n.value;
>  		delay_time_val = delay_time->u.n.value;
> diff --git a/drivers/accessibility/speakup/speakup_decext.c b/drivers/accessibility/speakup/speakup_decext.c
> index 271bcf279bf9b..e6be5a99670bd 100644
> --- a/drivers/accessibility/speakup/speakup_decext.c
> +++ b/drivers/accessibility/speakup/speakup_decext.c
> @@ -171,6 +171,10 @@ static void do_catch_up(struct spk_synth *synth)
>  
>  	while (!kthread_should_stop()) {
>  		spin_lock_irqsave(&speakup_info.spinlock, flags);
> +		if (!synth->alive) {
> +			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +			break;
> +		}
>  		if (speakup_info.flushing) {
>  			speakup_info.flushing = 0;
>  			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> diff --git a/drivers/accessibility/speakup/speakup_decpc.c b/drivers/accessibility/speakup/speakup_decpc.c
> index 1bf36d1e54773..cfafd94759639 100644
> --- a/drivers/accessibility/speakup/speakup_decpc.c
> +++ b/drivers/accessibility/speakup/speakup_decpc.c
> @@ -391,6 +391,10 @@ static void do_catch_up(struct spk_synth *synth)
>  
>  	while (!kthread_should_stop()) {
>  		spin_lock_irqsave(&speakup_info.spinlock, flags);
> +		if (!synth->alive) {
> +			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +			break;
> +		}
>  		if (speakup_info.flushing) {
>  			speakup_info.flushing = 0;
>  			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> diff --git a/drivers/accessibility/speakup/speakup_dectlk.c b/drivers/accessibility/speakup/speakup_dectlk.c
> index 56334405d865c..d9247c53d3275 100644
> --- a/drivers/accessibility/speakup/speakup_dectlk.c
> +++ b/drivers/accessibility/speakup/speakup_dectlk.c
> @@ -246,6 +246,10 @@ static void do_catch_up(struct spk_synth *synth)
>  		spin_unlock_irqrestore(&flush_lock, flags);
>  
>  		spin_lock_irqsave(&speakup_info.spinlock, flags);
> +		if (!synth->alive) {
> +			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +			break;
> +		}
>  		if (speakup_info.flushing) {
>  			speakup_info.flushing = 0;
>  			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> diff --git a/drivers/accessibility/speakup/speakup_dtlk.c b/drivers/accessibility/speakup/speakup_dtlk.c
> index fa826568937bf..73cd39e781b86 100644
> --- a/drivers/accessibility/speakup/speakup_dtlk.c
> +++ b/drivers/accessibility/speakup/speakup_dtlk.c
> @@ -206,6 +206,10 @@ static void do_catch_up(struct spk_synth *synth)
>  	jiff_max = jiffies + jiffy_delta_val;
>  	while (!kthread_should_stop()) {
>  		spin_lock_irqsave(&speakup_info.spinlock, flags);
> +		if (!synth->alive) {
> +			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +			break;
> +		}
>  		if (speakup_info.flushing) {
>  			speakup_info.flushing = 0;
>  			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> diff --git a/drivers/accessibility/speakup/speakup_keypc.c b/drivers/accessibility/speakup/speakup_keypc.c
> index 9356f63795603..e229009107f75 100644
> --- a/drivers/accessibility/speakup/speakup_keypc.c
> +++ b/drivers/accessibility/speakup/speakup_keypc.c
> @@ -193,6 +193,10 @@ static void do_catch_up(struct spk_synth *synth)
>  	jiff_max = jiffies + jiffy_delta_val;
>  	while (!kthread_should_stop()) {
>  		spin_lock_irqsave(&speakup_info.spinlock, flags);
> +		if (!synth->alive) {
> +			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +			break;
> +		}
>  		if (speakup_info.flushing) {
>  			speakup_info.flushing = 0;
>  			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> diff --git a/drivers/accessibility/speakup/synth.c b/drivers/accessibility/speakup/synth.c
> index 11cba1fd8715d..6a49ff475c16a 100644
> --- a/drivers/accessibility/speakup/synth.c
> +++ b/drivers/accessibility/speakup/synth.c
> @@ -75,6 +75,10 @@ static void _spk_do_catch_up(struct spk_synth *synth, int unicode)
>  	jiff_max = jiffies + jiffy_delta_val;
>  	while (!kthread_should_stop()) {
>  		spin_lock_irqsave(&speakup_info.spinlock, flags);
> +		if (!synth->alive) {
> +			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +			break;
> +		}
>  		if (speakup_info.flushing) {
>  			speakup_info.flushing = 0;
>  			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> -- 
> 2.25.1
> 

-- 
Samuel
<D> m'enfin, le 5 juillet, le mec vient visiter le labo...
* D a marque d'une croix rouge le 5 juillet sur son agenda
<y> niarc niarc niarc
<D> cet homme va souffrir
<B> c'est donc le 5 juillet qu'il meurt d'un accident de la route écrasé par un truck muni d'un pare buffle
 -+- #ens-mim - repaire de terroristes -+-

Re: [PATCH 2/4] speakup: Do not hold spk_mutex across catch_up()

[Samuel Thibault]

Hello,

Hongyu Xie, le mer. 14 oct. 2026 15:17:11 +0800, a ecrit:
> speakup_thread() holds spk_mutex while calling synth->catch_up().
> Apart from the indefinite retry loops fixed by the previous patch, a
> synth that digests its buffer slowly (e.g. a real speech synthesizer
> receiving a large chunk of text) keeps the mutex held for the whole
> drain, so loading, unloading or switching any synth module blocks
> until all pending speech has been output, which may take arbitrarily
> long.

Is that really a problem?

If the human operator is working on loading/unloading/switching, they
surely are aware that there is speech ongoing, and if needed, just stop
it so the load/unload/switch can happen?

That seems safer than making the locking even more complex.

Samuel

> Take spk_mutex off the catch_up() call path and keep the synth alive
> for the duration of the call instead:
> 
> - speakup_thread() raises a catch-up count and releases spk_mutex
>   before calling catch_up(), re-taking it afterwards.
> 
> - synth_release() first clears synth->alive (which makes catch_up()
>   return) and wakes the thread out of its sleep, then waits for the
>   catch-up count to drop to zero before releasing the synth resources
>   (sysfs attributes, variables, tty).  The synth buffer is cleared
>   there as well, so a switch discards the text that was still pending
>   for the old synth; previously a switch would first wait for that
>   text to be output by the old synth, which is the unbounded wait
>   this patch removes.  As synth_remove() only returns after the
>   drain, a synth module cannot be unloaded while catch_up() still
>   runs, so the synth structure and its tty stay valid for the whole
>   call without any module reference counting.
> 
> The count/wait handoff between the thread and the release looks
> like this (without it, the resources could be freed while
> catch_up() still runs):
> 
>     CPU0 (speakup_thread)          CPU1 (synth_release)
>     -------------------            -------------------
>     atomic_inc(&synth_catch_up_count)
>     mutex_unlock(&spk_mutex)
>     synth->catch_up(synth)
>       /* reads synth->alive,       spin_lock(&speakup_info.spinlock)
>          synth->io_ops, the tty */  synth->alive = 0
>                                    spin_unlock(...)
>                                    wake the thread out of its sleep
>                                    wait_event(!synth_catch_up_count)
>                                    /* resources stay valid: sysfs,
>                                       vars, tty are torn down only
>                                       after the count hits zero */
>       /* returns once ->alive
>          is 0 (loop exit of the
>          previous patch) */
>     atomic_dec_and_test(...)
>       wake_up(&synth_catch_up_waitq)
>                                    sysfs_remove_group(...)
>                                    speakup_unregister_var(...)
>                                    synth->release(synth)
>                                    /* patch 4 clears synth itself
>                                       under the spinlock */
> 
> The SPEECH_KILL key handler can revive the synth at any time: it
> runs under speakup_info.spinlock (speakup_key() holds it across
> do_spkup()), and the is_alive callbacks set synth->alive back to 1
> and queue the init string again.  If that lands after
> synth_release() cleared synth->alive, an in-flight catch_up()
> draining to a stuck synth never sees ->alive cleared, and
> synth_release() blocks forever on the catch-up count:
> 
>     CPU0 (SPEECH_KILL key)         CPU1 (synth_release)
>     ----------------------         -------------------
>                                    spin_lock(&speakup_info.spinlock)
>                                    synth->alive = 0
>                                    spin_unlock(...)
>     spk_synth_is_alive_restart():
>       /* under the same spinlock
>          via speakup_key() */
>       synth->alive = 1  /* revival */
>       /* catch_up() rechecks         wait_event(!synth_catch_up_count)
>          ->alive, sees 1 again */    /* never: the revival keeps the
>                                          loop running */
> 
> Mark the synth with ->releasing under speakup_info.spinlock while it
> is being released, and have both spk_synth_is_alive_restart() and
> spk_synth_is_alive_nop() skip the revival then, so the drain always
> terminates:
> 
>     CPU0 (SPEECH_KILL key)         CPU1 (synth_release)
>     ----------------------         -------------------
>                                    spin_lock(&speakup_info.spinlock)
>                                    synth->alive = 0
>                                    synth->releasing = true
>                                    spin_unlock(...)
>     spk_synth_is_alive_restart():
>       if (synth->releasing)
>         return 0  /* no revival */  wait_event(!synth_catch_up_count)
>       /* the catch_up() loop          /* terminates: the loop
>          exits on ->alive == 0           observes ->alive == 0 */
> 
> The other wake_up_process(speakup_task) sites (spk_do_flush(),
> do_synth_init()) get the same guard: the speakup keyboard notifier is
> registered before the kthread is created, so a speakup key press or a
> sysfs write in that window would otherwise pass NULL (or the
> kthread_create() error) to wake_up_process():
> 
>     CPU0 (speakup key / sysfs)     CPU1 (module exit)
>     --------------------------     -----------------
>     spk_do_flush():
>       wake_up_process(
>         speakup_task)              speakup_exit():
>       /* speakup_task is NULL         ... kthread_stop(speakup_task)
>          before kthread_create(),        /* task_struct freed */
>          ERR_PTR after a failure */   speakup_task = NULL;
>       /* wakeup on NULL / ERR_PTR,
>          or on freed memory */
> 
> The wakeup sites take the task pointer under rcu_read_lock(), so
> the task_struct cannot be freed underneath them by a concurrent
> module exit either:
> 
>     spk_do_flush():
>       rcu_read_lock();
>       task = READ_ONCE(speakup_task);
>       if (!IS_ERR_OR_NULL(task))
>         wake_up_process(task);
>       rcu_read_unlock();
>       /* the exit path unregisters the notifiers first, and
>          atomic_notifier_chain_unregister() synchronizes with
>          the RCU readers, so by the time speakup_task is
>          cleared no wakeup site is still running */
> 
> After this change spk_mutex no longer serializes synth registration,
> unregistration and switching against speech output.
> 
> This depends on the previous patch: the catch-up drain introduced
> here assumes that catch_up() eventually returns once synth->alive has
> been cleared, including in the local catch_up() copies of the serial
> synth drivers.
> 
> Fixes: 2067fd92d75b6 ("staging/speakup: Move out of staging")
> Cc: stable@vger.kernel.org
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
> Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
> ---
>  drivers/accessibility/speakup/spk_priv.h  |  3 ++
>  drivers/accessibility/speakup/spk_types.h |  1 +
>  drivers/accessibility/speakup/synth.c     | 59 +++++++++++++++++++++--
>  drivers/accessibility/speakup/thread.c    | 11 ++++-
>  4 files changed, 70 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/accessibility/speakup/spk_priv.h b/drivers/accessibility/speakup/spk_priv.h
> index 9da57ead17cbe..967862cd04403 100644
> --- a/drivers/accessibility/speakup/spk_priv.h
> +++ b/drivers/accessibility/speakup/spk_priv.h
> @@ -75,6 +75,9 @@ struct spk_synth *synth_current(void);
>  
>  extern struct speakup_info_t speakup_info;
>  
> +extern atomic_t synth_catch_up_count;
> +extern wait_queue_head_t synth_catch_up_waitq;
> +
>  extern struct var_t synth_time_vars[];
>  
>  extern struct spk_io_ops spk_serial_io_ops;
> diff --git a/drivers/accessibility/speakup/spk_types.h b/drivers/accessibility/speakup/spk_types.h
> index a5762330e2495..efb931db18506 100644
> --- a/drivers/accessibility/speakup/spk_types.h
> +++ b/drivers/accessibility/speakup/spk_types.h
> @@ -200,6 +200,7 @@ struct spk_synth {
>  	unsigned char (*get_index)(struct spk_synth *synth);
>  	struct synth_indexing indexing;
>  	int alive;
> +	bool releasing;	/* set while synth_release() drains this synth */
>  	struct attribute_group attributes;
>  
>  	void *dev;
> diff --git a/drivers/accessibility/speakup/synth.c b/drivers/accessibility/speakup/synth.c
> index 6a49ff475c16a..40870949b3c6b 100644
> --- a/drivers/accessibility/speakup/synth.c
> +++ b/drivers/accessibility/speakup/synth.c
> @@ -24,6 +24,14 @@ char spk_pitch_buff[32] = "";
>  static int module_status;
>  bool spk_quiet_boot;
>  
> +/*
> + * Number of catch_up() calls currently in flight, and waitqueue used by
> + * synth_release() to wait for them to finish before tearing down the
> + * synth resources they may still be using.
> + */
> +atomic_t synth_catch_up_count = ATOMIC_INIT(0);
> +DECLARE_WAIT_QUEUE_HEAD(synth_catch_up_waitq);
> +
>  struct speakup_info_t speakup_info = {
>  	/*
>  	 * This spinlock is used to protect the entire speakup machinery, and
> @@ -154,6 +162,8 @@ EXPORT_SYMBOL_GPL(spk_synth_get_index);
>  
>  int spk_synth_is_alive_nop(struct spk_synth *synth)
>  {
> +	if (synth->releasing)
> +		return 0;
>  	synth->alive = 1;
>  	return 1;
>  }
> @@ -163,6 +173,18 @@ int spk_synth_is_alive_restart(struct spk_synth *synth)
>  {
>  	if (synth->alive)
>  		return 1;
> +	/*
> +	 * The callers run under speakup_info.spinlock (speakup_key()
> +	 * holds it across do_spkup()), which is also the lock
> +	 * synth_release() takes to clear ->alive, so checking
> +	 * ->releasing here cannot race it: a synth being released is
> +	 * not revived, or an in-flight catch_up() draining to a stuck
> +	 * synth would never see ->alive cleared and synth_release()
> +	 * would block forever on the catch-up count while holding
> +	 * spk_mutex.
> +	 */
> +	if (synth->releasing)
> +		return 0;
>  	if (synth->io_ops->wait_for_xmitr(synth) > 0) {
>  		/* restart */
>  		synth->alive = 1;
> @@ -197,6 +219,8 @@ void synth_start(void)
>  
>  void spk_do_flush(void)
>  {
> +	struct task_struct *task;
> +
>  	if (!synth)
>  		return;
>  
> @@ -209,7 +233,11 @@ void spk_do_flush(void)
>  		}
>  	}
>  	wake_up_interruptible_all(&speakup_event);
> -	wake_up_process(speakup_task);
> +	rcu_read_lock();
> +	task = READ_ONCE(speakup_task);
> +	if (!IS_ERR_OR_NULL(task))
> +		wake_up_process(task);
> +	rcu_read_unlock();
>  }
>  
>  void synth_write(const char *_buf, size_t count)
> @@ -476,13 +504,18 @@ int synth_init(char *synth_name)
>  /* called by: synth_add() */
>  static int do_synth_init(struct spk_synth *in_synth)
>  {
> +	struct task_struct *task;
>  	struct var_t *var;
> +	unsigned long flags;
>  
>  	synth_release();
>  	if (in_synth->checkval != SYNTH_CHECK)
>  		return -EINVAL;
>  	synth = in_synth;
>  	synth->alive = 0;
> +	spin_lock_irqsave(&speakup_info.spinlock, flags);
> +	synth->releasing = false;
> +	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
>  	pr_warn("synth probe\n");
>  	if (synth->probe(synth) < 0) {
>  		pr_warn("%s: device probe failed\n", in_synth->name);
> @@ -510,13 +543,17 @@ static int do_synth_init(struct spk_synth *in_synth)
>  		return -ENOMEM;
>  	synth_flags = synth->flags;
>  	wake_up_interruptible_all(&speakup_event);
> -	if (speakup_task)
> -		wake_up_process(speakup_task);
> +	rcu_read_lock();
> +	task = READ_ONCE(speakup_task);
> +	if (!IS_ERR_OR_NULL(task))
> +		wake_up_process(task);
> +	rcu_read_unlock();
>  	return 0;
>  }
>  
>  void synth_release(void)
>  {
> +	struct task_struct *task;
>  	struct var_t *var;
>  	unsigned long flags;
>  
> @@ -525,8 +562,24 @@ void synth_release(void)
>  	spin_lock_irqsave(&speakup_info.spinlock, flags);
>  	pr_info("releasing synth %s\n", synth->name);
>  	synth->alive = 0;
> +	synth->releasing = true;
>  	timer_delete(&thread_timer);
> +	synth_buffer_clear();
>  	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +	/*
> +	 * catch_up() may still be running without spk_mutex held (it gets
> +	 * taken off the mutex by speakup_thread()).  Wake it out of its
> +	 * sleep and wait until it is done before releasing the resources
> +	 * (sysfs, variables, tty) it may be accessing.  speakup_task can
> +	 * be an ERR_PTR here, when speakup_init() failed after the
> +	 * command-line synth_init().
> +	 */
> +	rcu_read_lock();
> +	task = READ_ONCE(speakup_task);
> +	if (!IS_ERR_OR_NULL(task))
> +		wake_up_process(task);
> +	rcu_read_unlock();
> +	wait_event(synth_catch_up_waitq, !atomic_read(&synth_catch_up_count));
>  	if (synth->attributes.name)
>  		sysfs_remove_group(speakup_kobj, &synth->attributes);
>  	for (var = synth->vars; var->var_id != MAXVARS; var++)
> diff --git a/drivers/accessibility/speakup/thread.c b/drivers/accessibility/speakup/thread.c
> index 2fc75e60fbac2..5279e112248b4 100644
> --- a/drivers/accessibility/speakup/thread.c
> +++ b/drivers/accessibility/speakup/thread.c
> @@ -50,9 +50,18 @@ int speakup_thread(void *data)
>  		if (synth && synth->catch_up && synth->alive) {
>  			/*
>  			 * It is up to the callee to take the lock, so that it
> -			 * can sleep whenever it likes
> +			 * can sleep whenever it likes.  It may indeed sleep for
> +			 * a long while, e.g. while draining to a slow synth, so
> +			 * run it without spk_mutex: synth_release() waits for
> +			 * the catch-up count to drop before freeing the synth,
> +			 * hence the synth stays alive for the whole call.
>  			 */
> +			atomic_inc(&synth_catch_up_count);
> +			mutex_unlock(&spk_mutex);
>  			synth->catch_up(synth);
> +			if (atomic_dec_and_test(&synth_catch_up_count))
> +				wake_up(&synth_catch_up_waitq);
> +			mutex_lock(&spk_mutex);
>  		}
>  
>  		speakup_start_ttys();
> -- 
> 2.25.1
> 

-- 
Samuel
<D> m'enfin, le 5 juillet, le mec vient visiter le labo...
* D a marque d'une croix rouge le 5 juillet sur son agenda
<y> niarc niarc niarc
<D> cet homme va souffrir
<B> c'est donc le 5 juillet qu'il meurt d'un accident de la route écrasé par un truck muni d'un pare buffle
 -+- #ens-mim - repaire de terroristes -+-

Re: [PATCH 3/4] speakup: Guard late sysfs operations on speakup_kobj

[Samuel Thibault]

Hongyu Xie, le mer. 14 oct. 2026 15:17:12 +0800, a ecrit:
> speakup_kobj_init() drops the kobject on its error path and
> speakup_kobj_exit() drops it on removal, but neither clears the
> global pointer.  When built-in speakup fails late in speakup_init()
> (e.g. a kthread_create() failure, whose error path runs
> speakup_kobj_exit()), or when the kobject setup itself fails, the
> dangling speakup_kobj stays behind: a later insmod of a synth driver
> then reaches sysfs_create_group() through synth_add()->
> do_synth_init() and dereferences the freed kobject:
> 
>     boot (built-in speakup)         later insmod (synth driver)
>     --------------------------      ---------------------------
>     speakup_init() fails late:
>       err_speakup:
>         kobject_put(speakup_kobj);
>         /* speakup_kobj stays set */
> 
>                                     synth_add() -> do_synth_init()
>                                       sysfs_create_group(speakup_kobj,
>                                                          &attributes)
>                                       /* freed kobject: UAF */
> 
> Clear the pointer right after kobject_put() so the late
> sysfs_create_group() fails cleanly on the NULL check instead of
> touching freed memory:
> 
>       err_speakup:
>         kobject_put(speakup_kobj);
>         speakup_kobj = NULL;
> 
>                                     sysfs_create_group(NULL, ...)
>                                       /* fails cleanly: the NULL
>                                          check is sysfs's own */
> 
> The removal side needs the same guard: when the kobject setup
> failed, a first insmod of a synth driver fails in do_synth_init()
> (synth stays set because that error path does not roll it back),
> and a second insmod then goes through synth_release(), whose
> sysfs_remove_group() would now meet speakup_kobj == NULL and crash
> on the NULL kobject:
> 
>     first insmod                    second insmod
>     -----------                     ------------
>     do_synth_init():
>       synth = in_synth;
>       probe() fails                 synth_release():
>       /* no rollback:                sysfs_remove_group(
>          synth stays set */            speakup_kobj == NULL, ...)
>                                       /* NULL kobject: the crash
>                                          the NULL clear above would
>                                          itself introduce without
>                                          the guard below */
> 
> Skip the group removal when the kobject is gone, mirroring the
> NULL check that sysfs_create_group() already performs on its own:
> 
>                                     if (synth->attributes.name &&
>                                         speakup_kobj)
>                                       sysfs_remove_group(...)
>                                       /* skipped: the kobject is
>                                          gone */
> 
> Fixes: 2067fd92d75b6 ("staging/speakup: Move out of staging")
> Cc: stable@vger.kernel.org
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>

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

Thanks!

Samuel

> Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
> ---
>  drivers/accessibility/speakup/kobjects.c | 2 ++
>  drivers/accessibility/speakup/synth.c    | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/accessibility/speakup/kobjects.c b/drivers/accessibility/speakup/kobjects.c
> index a73b478e06de0..e1860f8d9d422 100644
> --- a/drivers/accessibility/speakup/kobjects.c
> +++ b/drivers/accessibility/speakup/kobjects.c
> @@ -1041,6 +1041,7 @@ int speakup_kobj_init(void)
>  	sysfs_remove_group(speakup_kobj, &main_attr_group);
>  err_speakup:
>  	kobject_put(speakup_kobj);
> +	speakup_kobj = NULL;
>  err_acc:
>  	kobject_put(accessibility_kobj);
>  out:
> @@ -1052,5 +1053,6 @@ void speakup_kobj_exit(void)
>  	sysfs_remove_group(speakup_kobj, &i18n_attr_group);
>  	sysfs_remove_group(speakup_kobj, &main_attr_group);
>  	kobject_put(speakup_kobj);
> +	speakup_kobj = NULL;
>  	kobject_put(accessibility_kobj);
>  }
> diff --git a/drivers/accessibility/speakup/synth.c b/drivers/accessibility/speakup/synth.c
> index 40870949b3c6b..1af80eb116af6 100644
> --- a/drivers/accessibility/speakup/synth.c
> +++ b/drivers/accessibility/speakup/synth.c
> @@ -580,7 +580,7 @@ void synth_release(void)
>  		wake_up_process(task);
>  	rcu_read_unlock();
>  	wait_event(synth_catch_up_waitq, !atomic_read(&synth_catch_up_count));
> -	if (synth->attributes.name)
> +	if (synth->attributes.name && speakup_kobj)
>  		sysfs_remove_group(speakup_kobj, &synth->attributes);
>  	for (var = synth->vars; var->var_id != MAXVARS; var++)
>  		speakup_unregister_var(var->var_id);
> -- 
> 2.25.1
> 

-- 
Samuel
<D> m'enfin, le 5 juillet, le mec vient visiter le labo...
* D a marque d'une croix rouge le 5 juillet sur son agenda
<y> niarc niarc niarc
<D> cet homme va souffrir
<B> c'est donc le 5 juillet qu'il meurt d'un accident de la route écrasé par un truck muni d'un pare buffle
 -+- #ens-mim - repaire de terroristes -+-

Re: [PATCH 4/4] speakup: Serialize synth clearing against locked readers

[Samuel Thibault]

Hello,

Indeed, the check for the synth pointer should be guarded, so it doesn't
disappear suddenly.

Your patch makes sense, but while at it:

> diff --git a/drivers/accessibility/speakup/devsynth.c b/drivers/accessibility/speakup/devsynth.c
> index e3d909bd04804..19799a362948d 100644
> --- a/drivers/accessibility/speakup/devsynth.c
> +++ b/drivers/accessibility/speakup/devsynth.c
> @@ -29,6 +29,10 @@ static ssize_t speakup_file_write(struct file *fp, const char __user *buffer,
>  		count -= bytes;
>  		ptr += bytes;
>  		spin_lock_irqsave(&speakup_info.spinlock, flags);
> +		if (!synth) {
> +			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +			return -ENODEV;
> +		}
>  		synth_write(buf, bytes);
>  		spin_unlock_irqrestore(&speakup_info.spinlock, flags);
>  	}

You can as well remove the check above for simplicity, since it becomes duplicate.

That is the same in mostly all other places of the patch.

Thanks,
Samuel