Re: [PATCH v2 3/5] tty: sc16is7xx: convert to kthread_run_worker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The explicitly allocated kthread worker is destroyed before the devm-managed threaded IRQ handler is freed, causing a Use-After-Free.

Pre-existing issues:
- [High] Closing any single port cancels the shared polling timer, starving all other active ports on the same multi-port chip.
--

commit 3bc3e4920cc0fac18e8f989a3296c6b4222d065c
Author: Bradley Morgan <brads@mainlining.org>

tty: sc16is7xx: convert to kthread_run_worker
    
Convert sc16is7xx to use kthread_run_worker() instead of the
deprecated kthread_run(kthread_worker_fn) pattern. The new API sets
worker->task before the worker starts.

> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 7107a0fb1e7bb..2cfc1f2900091 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1186,7 +1185,7 @@ static void sc16is7xx_shutdown(struct uart_port *port)
>  	if (s->polling)
>  		kthread_cancel_delayed_work_sync(&s->poll_work);
>  
> -	kthread_flush_worker(&s->kworker);
> +	kthread_flush_worker(s->kworker);
>  }

[Severity: High]
This is a pre-existing issue, but does closing any single port cancel the
shared polling timer for all ports?

In polling mode, s->poll_work is shared across all ports on the chip. If
one port is closed, sc16is7xx_shutdown() unconditionally cancels s->poll_work,
which seems to starve any other active ports on the same chip.

> @@ -1705,8 +1702,8 @@ void sc16is7xx_remove(struct device *dev)
>  	if (s->polling)
>  		kthread_cancel_delayed_work_sync(&s->poll_work);
>  
> -	kthread_flush_worker(&s->kworker);
> -	kthread_stop(s->kworker_task);
> +	kthread_flush_worker(s->kworker);
> +	kthread_destroy_worker(s->kworker);
>  
>  	clk_disable_unprepare(s->clk);
>  }

[Severity: High]
Can kthread_destroy_worker() lead to a use-after-free if the threaded IRQ
handler fires concurrently?

The driver uses devm_request_threaded_irq() in sc16is7xx_probe() to manage its
interrupt, meaning the IRQ handler remains active until after
sc16is7xx_remove() returns and devres cleans it up.

If an interrupt fires or an in-flight threaded IRQ executes in parallel right
after kthread_destroy_worker() frees s->kworker, the IRQ handler could call
functions like sc16is7xx_ier_clear() which enqueue work:

drivers/tty/serial/sc16is7xx.c:sc16is7xx_ier_clear() {
    ...
    kthread_queue_work(s->kworker, &one->reg_work);
}

This would dereference the freed s->kworker pointer. Previously kworker was
embedded in the sc16is7xx_port struct, so stopping the task was safe from a
use-after-free, but now kthread_destroy_worker() frees the dynamically
allocated worker before the IRQ handler is disabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788513591.git.brads@mainlining.org?part=3