linux-kernel - Re: [PATCH v4] tty: resolve loopback wait problem for serial port

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Mon, 4 Jan 2021 13:51:50 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     zhangqiumiao1@...wei.com
Cc:     jirislaby@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] tty: resolve loopback wait problem for serial port

On Mon, Jan 04, 2021 at 08:41:26PM +0800, zhangqiumiao1@...wei.com wrote:
> From: Qiumiao Zhang <zhangqiumiao1@...wei.com>
> 
> Serial port writing will be suspended when the buffer is continuously full. When
> the serial port's TX and RX are short circuited, there is a certain probability
> that this will happed. The concrete representation is n_tty_write blocking in
> wait_woken and the process is trapped in a loop waiting. After testing, when the
> the serial port runs well, wait_woken wait time will not exceed 60 seconds. So
> for serial port, set the timeout value of wait_woken function to 60s. Wake up and
> flush the buffer after timeout.
> 

Why would we ever want to wait longer than 60 seconds?  What problems
does this cause if we just always set the timeout to 60 seconds?

> Signed-off-by: Qiumiao Zhang <zhangqiumiao1@...wei.com>
> ---
> v4:
>  fix wrong expression in path description
>  remove unnecessary macro definition and debugging code
> v3:
>  add changes from v1 to v2
> 
> v2:
>  change to use "real name"
>  fix wrong expression in path description
>  remove config option
>  use driver type to judge tty device type
> 
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 319d68c..0e6f202 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -2375,7 +2375,17 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
>  		}
>  		up_read(&tty->termios_rwsem);
> 
> -		wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
> +		/* Resolve the problem of loopback waiting for serial port*/

Extra space please before the "*/"

> +		if (tty->driver->type == TTY_DRIVER_TYPE_SERIAL) {

Why is serial unique here?

> +			if (wait_woken(&wait, TASK_INTERRUPTIBLE, 60 * HZ) == 0)
> +				if (tty && tty->ops->flush_buffer) {
> +					tty->ops->flush_buffer(tty);
> +				} else {
> +					tty_err(tty, "tty related structure not registered\n");

How is this an error now?  What can a user do about it?  Do you need to
fix up some drivers because of this change?

Did you run this through checkpatch?  Are you sure that it does not
complain about the { } use here?



> +				}
> +		} else {
> +			wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
> +		}

This whole thing still feels really wrong.  If things are stuck, why
will this fix them?  Shouldn't the serial driver properly recover no
matter what?

What hardware and workload are you seeing this problem on?

thanks,

greg k-h