Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756426AbbDOS0h (ORCPT ); Wed, 15 Apr 2015 14:26:37 -0400 Received: from mail-qk0-f175.google.com ([209.85.220.175]:36523 "EHLO mail-qk0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756144AbbDOS0e (ORCPT ); Wed, 15 Apr 2015 14:26:34 -0400 Message-ID: <552EAD37.1020600@hurleysoftware.com> Date: Wed, 15 Apr 2015 14:25:59 -0400 From: Peter Hurley User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Joseph Salisbury , gregkh@linuxfoundation.org, luis.henriques@canonical.com, sasha.levin@oracle.com, kamal.mostafa@canonical.com, jslaby@suse.cz CC: linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: Re: [3.14.y][3.16.y-ckt][3.18.y][3.19.y][PATCH 1/1] n_tty: Fix read buffer overwrite when no newline References: <8317c7c803d45bc4606a09145d02c499456ace37.1428325696.git.joseph.salisbury@canonical.com> In-Reply-To: <8317c7c803d45bc4606a09145d02c499456ace37.1428325696.git.joseph.salisbury@canonical.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9487 Lines: 261 Hi Joseph, On 04/15/2015 01:39 PM, Joseph Salisbury wrote: > From: Peter Hurley > > BugLink: http://bugs.launchpad.net/bugs/1381005 > > In canon mode, the read buffer head will advance over the buffer tail > if the input > 4095 bytes without receiving a line termination char. > > Discard additional input until a line termination is received. > Before evaluating for overflow, the 'room' value is normalized for > I_PARMRK and 1 byte is reserved for line termination (even in !icanon > mode, in case the mode is switched). The following table shows the > transform: > > actual buffer | 'room' value before overflow calc > space avail | !I_PARMRK | I_PARMRK > -------------------------------------------------- > 0 | -1 | -1 > 1 | 0 | 0 > 2 | 1 | 0 > 3 | 2 | 0 > 4+ | 3 | 1 > > When !icanon or when icanon and the read buffer contains newlines, > normalized 'room' values of -1 and 0 are clamped to 0, and > 'overflow' is 0, so read_head is not adjusted and the input i/o loop > exits (setting no_room if called from flush_to_ldisc()). No input > is discarded since the reader does have input available to read > which ensures forward progress. > > When icanon and the read buffer does not contain newlines and the > normalized 'room' value is 0, then overflow and room are reset to 1, > so that the i/o loop will process the next input char normally > (except for parity errors which are ignored). Thus, erasures, signalling > chars, 7-bit mode, etc. will continue to be handled properly. > > If the input char processed was not a line termination char, then > the canon_head index will not have advanced, so the normalized 'room' > value will now be -1 and 'overflow' will be set, which indicates the > read_head can safely be reset, effectively erasing the last char > processed. > > If the input char processed was a line termination, then the > canon_head index will have advanced, so 'overflow' is cleared to 0, > the read_head is not reset, and 'room' is cleared to 0, which exits > the i/o loop (because the reader now have input available to read > which ensures forward progress). > > Note that it is possible for a line termination to be received, and > for the reader to copy the line to the user buffer before the > input i/o loop is ready to process the next input char. This is > why the i/o loop recomputes the room/overflow state with every > input char while handling overflow. > > Finally, if the input data was processed without receiving > a line termination (so that overflow is still set), the pty > driver must receive a write wakeup. A pty writer may be waiting > to write more data in n_tty_write() but without unthrottling > here that wakeup will not arrive, and forward progress will halt. > (Normally, the pty writer is woken when the reader reads data out > of the buffer and more space become available). Thanks for doing this! (I can now cross off the 1st item on my TODO list) comments below. > Signed-off-by: Peter Hurley > Signed-off-by: Greg Kroah-Hartman > (backported from commit fb5ef9e7da39968fec6d6f37f20a23d23740c75e) Please note this is essentially also a backport of commit 06c49f9fa31f ("n_tty: Fix PARMRK over-throttling") as well, since it incorporates the results. > Signed-off-by: Joseph Salisbury > --- > drivers/tty/n_tty.c | 108 +++++++++++++++++++++++++++++++++++++++------------- > 1 file changed, 81 insertions(+), 27 deletions(-) > > diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c > index 4ddfa60..f190136 100644 > --- a/drivers/tty/n_tty.c > +++ b/drivers/tty/n_tty.c > @@ -247,8 +247,8 @@ static void n_tty_write_wakeup(struct tty_struct *tty) > > static void n_tty_check_throttle(struct tty_struct *tty) > { > - if (tty->driver->type == TTY_DRIVER_TYPE_PTY) > - return; Ok. > + struct n_tty_data *ldata = tty->disc_data; > + Drop this. Nothing in n_tty_check_throttle() uses 'ldata' as a result of this commit. > /* > * Check the remaining room for the input canonicalization > * mode. We don't want to throttle the driver if we're in > @@ -1512,23 +1512,6 @@ n_tty_receive_char_lnext(struct tty_struct *tty, unsigned char c, char flag) > n_tty_receive_char_flagged(tty, c, flag); > } > > -/** > - * n_tty_receive_buf - data receive > - * @tty: terminal device > - * @cp: buffer > - * @fp: flag buffer > - * @count: characters > - * > - * Called by the terminal driver when a block of characters has > - * been received. This function must be called from soft contexts > - * not from interrupt context. The driver is responsible for making > - * calls one at a time and in order (or using flush_to_ldisc) > - * > - * n_tty_receive_buf()/producer path: > - * claims non-exclusive termios_rwsem > - * publishes read_head and canon_head > - */ > - > static void > n_tty_receive_buf_real_raw(struct tty_struct *tty, const unsigned char *cp, > char *fp, int count) > @@ -1684,24 +1667,85 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp, > } > } > > +/** > + * n_tty_receive_buf_common - process input > + * @tty: device to receive input > + * @cp: input chars > + * @fp: flags for each char (if NULL, all chars are TTY_NORMAL) > + * @count: number of input chars in @cp > + * > + * Called by the terminal driver when a block of characters has > + * been received. This function must be called from soft contexts > + * not from interrupt context. The driver is responsible for making > + * calls one at a time and in order (or using flush_to_ldisc) > + * > + * Returns the # of input chars from @cp which were processed. > + * > + * In canonical mode, the maximum line length is 4096 chars (including > + * the line termination char); lines longer than 4096 chars are > + * truncated. After 4095 chars, input data is still processed but > + * not stored. Overflow processing ensures the tty can always > + * receive more input until at least one line can be read. > + * > + * In non-canonical mode, the read buffer will only accept 4095 chars; > + * this provides the necessary space for a newline char if the input > + * mode is switched to canonical. > + * > + * Note it is possible for the read buffer to _contain_ 4096 chars > + * in non-canonical mode: the read buffer could already contain the > + * maximum canon line of 4096 chars when the mode is switched to > + * non-canonical. > + * > + * n_tty_receive_buf()/producer path: > + * claims non-exclusive termios_rwsem > + * publishes commit_head or canon_head > + */ > static int > n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp, > char *fp, int count, int flow) > { > struct n_tty_data *ldata = tty->disc_data; > - int room, n, rcvd = 0; > + int room, n, rcvd = 0, overflow; > > down_read(&tty->termios_rwsem); > > while (1) { > - room = receive_room(tty); > + /* > + * When PARMRK is set, each input char may take up to 3 chars > + * in the read buf; reduce the buffer space avail by 3x > + * > + * If we are doing input canonicalization, and there are no > + * pending newlines, let characters through without limit, so > + * that erase characters will be handled. Other excess > + * characters will be beeped. > + * > + * paired with store in *_copy_from_read_buf() -- guarantees > + * the consumer has loaded the data in read_buf up to the new > + * read_tail (so this producer will not overwrite unread data) > + */ > + size_t tail = smp_load_acquire(&ldata->read_tail); smp_load_acquire() is part of another fix not associated with this problem. This line should simply be size_t tail = ldata->read_tail; Then this fix can be applied across 3.12~3.19, inclusive. Thanks again. Regards, Peter Hurley > + > + room = N_TTY_BUF_SIZE - (ldata->read_head - tail); > + if (I_PARMRK(tty)) > + room = (room + 2) / 3; > + room--; > + if (room <= 0) { > + overflow = ldata->icanon && ldata->canon_head == tail; > + if (overflow && room < 0) > + ldata->read_head--; > + room = overflow; > + ldata->no_room = flow && !room; > + } else > + overflow = 0; > + > n = min(count, room); > - if (!n) { > - if (flow && !room) > - ldata->no_room = 1; > + if (!n) > break; > - } > - __receive_buf(tty, cp, fp, n); > + > + /* ignore parity errors if handling overflow */ > + if (!overflow || !fp || *fp != TTY_PARITY) > + __receive_buf(tty, cp, fp, n); > + > cp += n; > if (fp) > fp += n; > @@ -1710,7 +1754,17 @@ n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp, > } > > tty->receive_room = room; > - n_tty_check_throttle(tty); > + > + /* Unthrottle if handling overflow on pty */ > + if (tty->driver->type == TTY_DRIVER_TYPE_PTY) { > + if (overflow) { > + tty_set_flow_change(tty, TTY_UNTHROTTLE_SAFE); > + tty_unthrottle_safe(tty); > + __tty_set_flow_change(tty, 0); > + } > + } else > + n_tty_check_throttle(tty); > + > up_read(&tty->termios_rwsem); > > return rcvd; > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/