Re: [PATCH v2] greybus/uart: Fix atomicity violation in get_serial_info()

[Alex Elder <elder@ieee.org>]

On 11/6/24 3:58 AM, Qiu-ji Chen wrote:
> Our static checker found a bug where set_serial_info() uses a mutex, but
> get_serial_info() does not. Fortunately, the impact of this is relatively
> minor. It doesn't cause a crash or any other serious issues. However, if a
> race condition occurs between set_serial_info() and get_serial_info(),
> there is a chance that the data returned by get_serial_info() will be
> meaningless.
> 
> Signed-off-by: Qiu-ji Chen <chenqiuji666@gmail.com>
> Fixes: 0aad5ad563c8 ("greybus/uart: switch to ->[sg]et_serial()")

Looks good.

Reviewed-by: Alex Elder <elder@riscstar.com>

PS  I was going to suggest computing the close delay and closing wait
     outside the mutex, as get_serial_info() above it does.  But it's
     minor and there's no reason to hold up your patch for that.

> ---
> V2:
> Modified the patch description to make it more concise and easier to understand.
> Changed the fix code to ensure the logic is correct.
> Thanks to Johan Hovold and Dan Carpenter for helpful suggestion.
> ---
>   drivers/staging/greybus/uart.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
> index cdf4ebb93b10..8eab94cb06fa 100644
> --- a/drivers/staging/greybus/uart.c
> +++ b/drivers/staging/greybus/uart.c
> @@ -596,11 +596,13 @@ static int get_serial_info(struct tty_struct *tty,
>   	struct gb_tty *gb_tty = tty->driver_data;
>   
>   	ss->line = gb_tty->minor;
> +	mutex_lock(&gb_tty->port.mutex);
>   	ss->close_delay = jiffies_to_msecs(gb_tty->port.close_delay) / 10;
>   	ss->closing_wait =
>   		gb_tty->port.closing_wait == ASYNC_CLOSING_WAIT_NONE ?
>   		ASYNC_CLOSING_WAIT_NONE :
>   		jiffies_to_msecs(gb_tty->port.closing_wait) / 10;
> +	mutex_unlock(&gb_tty->port.mutex);
>   
>   	return 0;
>   }