From: Samuel Thibault <samuel.thibault@ens-lyon.org>
To: Hongyu Xie <xiehongyu1@kylinos.cn>
Cc: speakup@linux-speakup.org, linux-kernel@vger.kernel.org,
gregkh@linuxfoundation.org, chris@the-brannons.com,
kirk@reisers.ca, w.d.hubbs@gmail.com, stable@vger.kernel.org
Subject: Re: [PATCH 2/4] speakup: Do not hold spk_mutex across catch_up()
Date: Sun, 18 Oct 2026 20:26:54 +0200 [thread overview]
Message-ID: <atUPbjRxDtzBZ4Vc@end> (raw)
In-Reply-To: <20261014071713.321914-3-xiehongyu1@kylinos.cn>
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 -+-
next prev parent reply other threads:[~2026-10-18 18:26 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-14 7:17 [PATCH 0/4] speakup: fix hang and use-after-free around synth removal Hongyu Xie
2026-10-14 7:17 ` [PATCH 1/4] speakup: Stop catch_up when the synth is not alive Hongyu Xie
2026-10-18 18:24 ` Samuel Thibault
2026-10-14 7:17 ` [PATCH 2/4] speakup: Do not hold spk_mutex across catch_up() Hongyu Xie
2026-10-18 18:26 ` Samuel Thibault [this message]
2026-10-14 7:17 ` [PATCH 3/4] speakup: Guard late sysfs operations on speakup_kobj Hongyu Xie
2026-10-18 18:27 ` Samuel Thibault
2026-10-14 7:17 ` [PATCH 4/4] speakup: Serialize synth clearing against locked readers Hongyu Xie
2026-10-18 19:40 ` Samuel Thibault
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=atUPbjRxDtzBZ4Vc@end \
--to=samuel.thibault@ens-lyon.org \
--cc=chris@the-brannons.com \
--cc=gregkh@linuxfoundation.org \
--cc=kirk@reisers.ca \
--cc=linux-kernel@vger.kernel.org \
--cc=speakup@linux-speakup.org \
--cc=stable@vger.kernel.org \
--cc=w.d.hubbs@gmail.com \
--cc=xiehongyu1@kylinos.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®