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 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 still pending for the old synth instead of waiting for it to be output. 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: CPU0 (speakup_thread) CPU1 (synth_release) ------------------- ------------------- atomic_inc(&synth_catch_up_count) mutex_unlock(&spk_mutex) synth->catch_up(synth) spin_lock(&speakup_info.spinlock) /* reads synth->alive, synth->alive = 0 synth->io_ops, the tty */ spin_unlock(...) wake the thread out of its sleep /* returns once ->alive wait_event(!synth_catch_up_count) is 0 (loop exit of the /* resources torn down only previous patch) */ after the count hits zero */ atomic_dec_and_test(...) wake_up(&synth_catch_up_waitq) synth->release(synth) The SPEECH_KILL key handler can revive the synth at any time: the is_alive callbacks set synth->alive back to 1, so a revival landing after synth_release() cleared it keeps the loop running and the drain blocks forever. Mark the synth with ->releasing under speakup_info.spinlock while it is being released, and have spk_synth_is_alive_restart() and spk_synth_is_alive_nop() skip the revival then, so the drain always terminates. The wake_up_process(speakup_task) sites (spk_do_flush(), do_synth_init(), synth_release()) get the same treatment: the keyboard notifier is registered before the kthread is created, so a key press in that window would pass NULL (or the kthread_create() error) to wake_up_process(). They now take the task pointer under rcu_read_lock() with READ_ONCE() and skip IS_ERR_OR_NULL() values; 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 Reviewed-by: Samuel Thibault Signed-off-by: Hongyu Xie --- 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 9da57ead17cb..967862cd0440 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 a5762330e249..efb931db1850 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 6a49ff475c16..40870949b3c6 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 2fc75e60fbac..5279e112248b 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.32.0