Re: [PATCH v3 1/4] tty: serdev: Add mutex lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tty: serdev: ABBA deadlock between serport->lock and TTY workqueue
- [Medium] pm: serdev: cancelled async pm_runtime_put leaks power state
--

commit f8d69b3702358f814be05474626c0ebf27b17939
Author: Markus Probst <markus.probst@posteo.de>

    tty: serdev: Add mutex lock
    
    Besides more predictable behaviour, this allows for several hardened
    behaviour changes:
    
    Return -EALREADY in serdev_device_open if the device is already open
    instead of causing undefined behaviour.

> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 7500efcdfc218..77e8e1d4d2a66 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -142,6 +142,11 @@ void serdev_device_remove(struct serdev_device *serdev)
>  	struct serdev_controller *ctrl = serdev->ctrl;
>  
>  	device_unregister(&serdev->dev);
> +
> +	/* Warn if driver did not close the serial device. */
> +	if (ctrl->ops->close && WARN_ON(ctrl->ops->close(ctrl)))
> +		pm_runtime_put(&ctrl->dev);
> +

[Severity: Medium]
When a driver fails to close the device, this calls an asynchronous
pm_runtime_put(). If this removal occurs during controller teardown,
serdev_controller_remove() will immediately call
pm_runtime_disable(&ctrl->dev).

Disabling runtime PM explicitly cancels any pending asynchronous idle/suspend
work. Does this leave the device permanently powered on? Would a synchronous
pm_runtime_put_sync() prevent this leak?

>  	ctrl->serdev = NULL;
>  }
>  EXPORT_SYMBOL_GPL(serdev_device_remove);
[ ... ]
> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index bab1b143b8a68..c11908f5e1ce3 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c
[ ... ]
> @@ -29,6 +30,8 @@ static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp,
>  	struct serport *serport = serdev_controller_get_drvdata(ctrl);
>  	size_t ret;
>  
> +	guard(mutex)(&serport->lock);
> +

[Severity: High]
This is a pre-existing issue, but does attempting to acquire the lock here
trigger a use-after-free if the device is actively receiving data when
unregistered?

When serdev_tty_port_unregister() drops the reference to ctrl, it frees both
ctrl and serport without synchronously cancelling the flush_to_ldisc TTY
workqueue. If that workqueue is executing concurrently, it will call
ttyport_receive_buf() and try to acquire serport->lock on the freed memory.

Also, does this lock attempt cause an ABBA deadlock?

ttyport_close() acquires serport->lock, then calls tty_release_struct().
tty_release_struct() synchronously waits for the flush_to_ldisc workqueue
to drain via cancel_work_sync(). If that workqueue is running concurrently,
it calls ttyport_receive_buf() and blocks here trying to acquire the same
serport->lock, deadlocking the system.

>  	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
>  		return 0;
>  
[ ... ]
> @@ -130,23 +142,30 @@ static int ttyport_open(struct serdev_controller *ctrl)
>  
>  	set_bit(SERPORT_ACTIVE, &serport->flags);
>  
> +	mutex_unlock(&serport->lock);
> +
>  	return 0;
>  
>  err_close:
>  	tty->ops->close(tty, NULL);
> -err_unlock:
> +err_tty_unlock:
>  	tty_unlock(tty);
>  	tty_release_struct(tty, serport->tty_idx);
> +err_flags_unlock:
> +	mutex_unlock(&serport->lock);

[Severity: High]
Does this error path trigger the same deadlock described above? It calls
tty_release_struct() while holding serport->lock.

>  
>  	return ret;
>  }
>  
> -static void ttyport_close(struct serdev_controller *ctrl)
> +static bool ttyport_close(struct serdev_controller *ctrl)
>  {
>  	struct serport *serport = serdev_controller_get_drvdata(ctrl);
>  	struct tty_struct *tty = serport->tty;
>  
> -	clear_bit(SERPORT_ACTIVE, &serport->flags);
> +	guard(mutex)(&serport->lock);
> +

[Severity: High]
This is a pre-existing issue, but does reading serport->tty into a local
variable before acquiring the lock bypass the lock's protection?

If ttyport_close() runs concurrently with ttyport_open(), it may read a stale
or NULL tty pointer before acquiring the lock. After ttyport_open() finishes
and releases the lock, ttyport_close() acquires it, passes the
SERPORT_ACTIVE check, and then dereferences the stale tty pointer.

> +	if (!__test_and_clear_bit(SERPORT_ACTIVE, &serport->flags))
> +		return false;
>  
>  	tty_lock(tty);
>  	if (tty->ops->close)
> @@ -154,6 +173,8 @@ static void ttyport_close(struct serdev_controller *ctrl)
>  	tty_unlock(tty);
>  
>  	tty_release_struct(tty, serport->tty_idx);
> +
> +	return true;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-rust_serdev_probe_refactor-v3-0-d1b6e77135e0@posteo.de?part=1