linux-kernel - Re: [PATCH v2 1/3] tty: Add functions for handling flow control chars

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d79c4c20-1cfd-a3a1-516b-b6b51360527c@linux.intel.com>
Date:   Fri, 8 Apr 2022 15:20:07 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
cc:     linux-serial <linux-serial@...r.kernel.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Gilles Buloz <gilles.buloz@...tron.com>,
        Johan Hovold <johan@...nel.org>
Subject: Re: [PATCH v2 1/3] tty: Add functions for handling flow control
 chars

On Fri, 8 Apr 2022, Andy Shevchenko wrote:

> On Fri, Apr 08, 2022 at 02:39:52PM +0300, Ilpo Järvinen wrote:
> > Move receive path flow control character handling to own function
> > and a helper.
> > 
> > This seems cleanest approach especially once skipping due to lookahead
> > is added. Its downside is the duplicated START_CHAR and STOP_CHAR
> > checks.
> > 
> > No functional changes.
> 
> But it seems the change. See below.
> 
> ...
> 
> > +static bool n_tty_is_char_flow_ctrl(struct tty_struct *tty, unsigned char c)
> > +{
> > +	return c == START_CHAR(tty) || c == STOP_CHAR(tty);
> > +}
> > +
> > +/* Returns true if c is consumed as flow-control character */
> > +static bool n_tty_receive_char_flow_ctrl(struct tty_struct *tty, unsigned char c)
> > +{
> > +	if (!n_tty_is_char_flow_ctrl(tty, c))
> > +		return false;
> > +
> > +	if (c == START_CHAR(tty)) {
> > +		start_tty(tty);
> > +		process_echoes(tty);
> 
> > +	} else if (c == STOP_CHAR(tty)) {
> 
> In the original code no 'else' was present.
>
> Perhaps it's not a functional change, but this detail has to be explained.

Correct that the previous code didn't have else, however, there was return 
with the same effect. Adding this else here was no accident from my part 
but it is intentionally there to have no functional change for the 
START_CHAR == STOP_CHAR case!

> > +		stop_tty(tty);
> > +	}
> > +
> > +	return true;
> > +}
> 
> ...
> 
> > -	if (I_IXON(tty)) {
> > -		if (c == START_CHAR(tty)) {
> > -			start_tty(tty);
> > -			process_echoes(tty);
> > -			return;
> > -		}
> > -		if (c == STOP_CHAR(tty)) {
> > -			stop_tty(tty);
> > -			return;
> > -		}
> > -	}
> > +	if (I_IXON(tty) && n_tty_receive_char_flow_ctrl(tty, c))
> > +		return;
> 
> 

-- 
 i.