From c9cd57bf57fd450972a7802b9f09a680dbb4634e Mon Sep 17 00:00:00 2001 From: "Yan.Gao" Date: Thu, 10 Dec 2020 10:25:07 +0800 Subject: tty: Protect disc_data in n_tty_close and n_tty_flush_buffer n_tty_flush_buffer can happen in parallel with n_tty_close that the tty->disc_data will be set to NULL. n_tty_flush_buffer accesses tty->disc_data, so we must prevent n_tty_close clear tty->disc_data while n_tty_flush_buffer has a non-NULL view of tty->disc_data. So we need to make sure that accesses to disc_data are atomic using tty->termios_rwsem. There is an example I meet: When n_tty_flush_buffer accesses tty struct, the disc_data is right. However, then reset_buffer_flags accesses tty->disc_data, disc_data become NULL, So kernel crash when accesses tty->disc_data->real_tail. I guess there could be another thread change tty->disc_data to NULL, and during N_TTY line discipline, n_tty_close will set tty->disc_data to be NULL. So use tty->termios_rwsem to protect disc_data between close and flush_buffer. IP: reset_buffer_flags+0x9/0xf0 PGD 0 P4D 0 Oops: 0002 [#1] SMP CPU: 23 PID: 2087626 Comm: (agetty) Kdump: loaded Tainted: G Hardware name: UNISINSIGHT X3036P-G3/ST01M2C7S, BIOS 2.00.13 01/11/2019 task: ffff9c4e9da71e80 task.stack: ffffb30cfe898000 RIP: 0010:reset_buffer_flags+0x9/0xf0 RSP: 0018:ffffb30cfe89bca8 EFLAGS: 00010246 RAX: ffff9c4e9da71e80 RBX: ffff9c368d1bac00 RCX: 0000000000000000 RDX: 0000000000000000 RSI: ffff9c4ea17b50f0 RDI: 0000000000000000 RBP: ffffb30cfe89bcc8 R08: 0000000000000100 R09: 0000000000000001 R10: 0000000000000001 R11: 0000000000000000 R12: ffff9c368d1bacc0 R13: ffff9c20cfd18428 R14: ffff9c4ea17b50f0 R15: ffff9c368d1bac00 FS: 00007f9fbbe97940(0000) GS:ffff9c375c740000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000002260 CR3: 0000002f72233003 CR4: 00000000007606e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 PKRU: 55555554 Call Trace: ? n_tty_flush_buffer+0x2a/0x60 tty_buffer_flush+0x76/0x90 tty_ldisc_flush+0x22/0x40 vt_ioctl+0x5a7/0x10b0 ? n_tty_ioctl_helper+0x27/0x110 tty_ioctl+0xef/0x8c0 do_vfs_ioctl+0xa7/0x5e0 ? __audit_syscall_entry+0xaf/0x100 ? syscall_trace_enter+0x1d0/0x2b0 SyS_ioctl+0x79/0x90 do_syscall_64+0x6c/0x1b0 entry_SYSCALL64_slow_path+0x25/0x25 n_tty_flush_buffer --->tty->disc_data is OK ->reset_buffer_flags -->tty->disc_data is NULL Signed-off-by: Yan.Gao Reviewed-by: Xianting Tian Link: https://lore.kernel.org/r/20201210022507.30729-1-gao.yanB@h3c.com Signed-off-by: Greg Kroah-Hartman --- drivers/tty/n_tty.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'drivers/tty/n_tty.c') diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 319d68c8a5df..d633ba56cf83 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -1894,8 +1894,10 @@ static void n_tty_close(struct tty_struct *tty) if (tty->link) n_tty_packet_mode_flush(tty); + down_write(&tty->termios_rwsem); vfree(ldata); tty->disc_data = NULL; + up_write(&tty->termios_rwsem); } /** -- cgit v1.2.3 From 3b830a9c34d5897be07176ce4e6f2d75e2c8cfd7 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Mon, 18 Jan 2021 13:31:30 -0800 Subject: tty: convert tty_ldisc_ops 'read()' function to take a kernel pointer The tty line discipline .read() function was passed the final user pointer destination as an argument, which doesn't match the 'write()' function, and makes it very inconvenient to do a splice method for ttys. This is a conversion to use a kernel buffer instead. NOTE! It does this by passing the tty line discipline ->read() function an additional "cookie" to fill in, and an offset into the cookie data. The line discipline can fill in the cookie data with its own private information, and then the reader will repeat the read until either the cookie is cleared or it runs out of data. The only real user of this is N_HDLC, which can use this to handle big packets, even if the kernel buffer is smaller than the whole packet. Cc: Christoph Hellwig Cc: Greg Kroah-Hartman Cc: Al Viro Signed-off-by: Linus Torvalds --- drivers/bluetooth/hci_ldisc.c | 34 +++++++++--------- drivers/input/serio/serport.c | 4 ++- drivers/net/ppp/ppp_async.c | 3 +- drivers/net/ppp/ppp_synctty.c | 3 +- drivers/tty/n_gsm.c | 3 +- drivers/tty/n_hdlc.c | 60 +++++++++++++++++++++---------- drivers/tty/n_null.c | 3 +- drivers/tty/n_r3964.c | 10 +++--- drivers/tty/n_tracerouter.c | 4 ++- drivers/tty/n_tracesink.c | 4 ++- drivers/tty/n_tty.c | 82 ++++++++++++++++++------------------------- drivers/tty/tty_io.c | 64 +++++++++++++++++++++++++++++++-- include/linux/tty_ldisc.h | 3 +- net/nfc/nci/uart.c | 3 +- 14 files changed, 178 insertions(+), 102 deletions(-) (limited to 'drivers/tty/n_tty.c') diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c index f83d67eafc9f..dd92aea15b8b 100644 --- a/drivers/bluetooth/hci_ldisc.c +++ b/drivers/bluetooth/hci_ldisc.c @@ -802,7 +802,8 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file, * We don't provide read/write/poll interface for user space. */ static ssize_t hci_uart_tty_read(struct tty_struct *tty, struct file *file, - unsigned char __user *buf, size_t nr) + unsigned char *buf, size_t nr, + void **cookie, unsigned long offset) { return 0; } @@ -819,29 +820,28 @@ static __poll_t hci_uart_tty_poll(struct tty_struct *tty, return 0; } +static struct tty_ldisc_ops hci_uart_ldisc = { + .owner = THIS_MODULE, + .magic = TTY_LDISC_MAGIC, + .name = "n_hci", + .open = hci_uart_tty_open, + .close = hci_uart_tty_close, + .read = hci_uart_tty_read, + .write = hci_uart_tty_write, + .ioctl = hci_uart_tty_ioctl, + .compat_ioctl = hci_uart_tty_ioctl, + .poll = hci_uart_tty_poll, + .receive_buf = hci_uart_tty_receive, + .write_wakeup = hci_uart_tty_wakeup, +}; + static int __init hci_uart_init(void) { - static struct tty_ldisc_ops hci_uart_ldisc; int err; BT_INFO("HCI UART driver ver %s", VERSION); /* Register the tty discipline */ - - memset(&hci_uart_ldisc, 0, sizeof(hci_uart_ldisc)); - hci_uart_ldisc.magic = TTY_LDISC_MAGIC; - hci_uart_ldisc.name = "n_hci"; - hci_uart_ldisc.open = hci_uart_tty_open; - hci_uart_ldisc.close = hci_uart_tty_close; - hci_uart_ldisc.read = hci_uart_tty_read; - hci_uart_ldisc.write = hci_uart_tty_write; - hci_uart_ldisc.ioctl = hci_uart_tty_ioctl; - hci_uart_ldisc.compat_ioctl = hci_uart_tty_ioctl; - hci_uart_ldisc.poll = hci_uart_tty_poll; - hci_uart_ldisc.receive_buf = hci_uart_tty_receive; - hci_uart_ldisc.write_wakeup = hci_uart_tty_wakeup; - hci_uart_ldisc.owner = THIS_MODULE; - err = tty_register_ldisc(N_HCI, &hci_uart_ldisc); if (err) { BT_ERR("HCI line discipline registration failed. (%d)", err); diff --git a/drivers/input/serio/serport.c b/drivers/input/serio/serport.c index 8ac970a423de..33e9d9bfd036 100644 --- a/drivers/input/serio/serport.c +++ b/drivers/input/serio/serport.c @@ -156,7 +156,9 @@ out: * returning 0 characters. */ -static ssize_t serport_ldisc_read(struct tty_struct * tty, struct file * file, unsigned char __user * buf, size_t nr) +static ssize_t serport_ldisc_read(struct tty_struct * tty, struct file * file, + unsigned char *kbuf, size_t nr, + void **cookie, unsigned long offset) { struct serport *serport = (struct serport*) tty->disc_data; struct serio *serio; diff --git a/drivers/net/ppp/ppp_async.c b/drivers/net/ppp/ppp_async.c index 29a0917a81e6..f14a9d190de9 100644 --- a/drivers/net/ppp/ppp_async.c +++ b/drivers/net/ppp/ppp_async.c @@ -259,7 +259,8 @@ static int ppp_asynctty_hangup(struct tty_struct *tty) */ static ssize_t ppp_asynctty_read(struct tty_struct *tty, struct file *file, - unsigned char __user *buf, size_t count) + unsigned char *buf, size_t count, + void **cookie, unsigned long offset) { return -EAGAIN; } diff --git a/drivers/net/ppp/ppp_synctty.c b/drivers/net/ppp/ppp_synctty.c index 0f338752c38b..f774b7e52da4 100644 --- a/drivers/net/ppp/ppp_synctty.c +++ b/drivers/net/ppp/ppp_synctty.c @@ -257,7 +257,8 @@ static int ppp_sync_hangup(struct tty_struct *tty) */ static ssize_t ppp_sync_read(struct tty_struct *tty, struct file *file, - unsigned char __user *buf, size_t count) + unsigned char *buf, size_t count, + void **cookie, unsigned long offset) { return -EAGAIN; } diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index 25f3152089c2..fea1eeac5b90 100644 --- a/drivers/tty/n_gsm.c +++ b/drivers/tty/n_gsm.c @@ -2557,7 +2557,8 @@ static void gsmld_write_wakeup(struct tty_struct *tty) */ static ssize_t gsmld_read(struct tty_struct *tty, struct file *file, - unsigned char __user *buf, size_t nr) + unsigned char *buf, size_t nr, + void **cookie, unsigned long offset) { return -EOPNOTSUPP; } diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c index 12557ee1edb6..1363e659dc1d 100644 --- a/drivers/tty/n_hdlc.c +++ b/drivers/tty/n_hdlc.c @@ -416,13 +416,19 @@ static void n_hdlc_tty_receive(struct tty_struct *tty, const __u8 *data, * Returns the number of bytes returned or error code. */ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file, - __u8 __user *buf, size_t nr) + __u8 *kbuf, size_t nr, + void **cookie, unsigned long offset) { struct n_hdlc *n_hdlc = tty->disc_data; int ret = 0; struct n_hdlc_buf *rbuf; DECLARE_WAITQUEUE(wait, current); + /* Is this a repeated call for an rbuf we already found earlier? */ + rbuf = *cookie; + if (rbuf) + goto have_rbuf; + add_wait_queue(&tty->read_wait, &wait); for (;;) { @@ -436,25 +442,8 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file, set_current_state(TASK_INTERRUPTIBLE); rbuf = n_hdlc_buf_get(&n_hdlc->rx_buf_list); - if (rbuf) { - if (rbuf->count > nr) { - /* too large for caller's buffer */ - ret = -EOVERFLOW; - } else { - __set_current_state(TASK_RUNNING); - if (copy_to_user(buf, rbuf->buf, rbuf->count)) - ret = -EFAULT; - else - ret = rbuf->count; - } - - if (n_hdlc->rx_free_buf_list.count > - DEFAULT_RX_BUF_COUNT) - kfree(rbuf); - else - n_hdlc_buf_put(&n_hdlc->rx_free_buf_list, rbuf); + if (rbuf) break; - } /* no data */ if (tty_io_nonblock(tty, file)) { @@ -473,6 +462,39 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file, remove_wait_queue(&tty->read_wait, &wait); __set_current_state(TASK_RUNNING); + if (!rbuf) + return ret; + *cookie = rbuf; + +have_rbuf: + /* Have we used it up entirely? */ + if (offset >= rbuf->count) + goto done_with_rbuf; + + /* More data to go, but can't copy any more? EOVERFLOW */ + ret = -EOVERFLOW; + if (!nr) + goto done_with_rbuf; + + /* Copy as much data as possible */ + ret = rbuf->count - offset; + if (ret > nr) + ret = nr; + memcpy(kbuf, rbuf->buf+offset, ret); + offset += ret; + + /* If we still have data left, we leave the rbuf in the cookie */ + if (offset < rbuf->count) + return ret; + +done_with_rbuf: + *cookie = NULL; + + if (n_hdlc->rx_free_buf_list.count > DEFAULT_RX_BUF_COUNT) + kfree(rbuf); + else + n_hdlc_buf_put(&n_hdlc->rx_free_buf_list, rbuf); + return ret; } /* end of n_hdlc_tty_read() */ diff --git a/drivers/tty/n_null.c b/drivers/tty/n_null.c index 96feabae4740..ce03ae78f5c6 100644 --- a/drivers/tty/n_null.c +++ b/drivers/tty/n_null.c @@ -20,7 +20,8 @@ static void n_null_close(struct tty_struct *tty) } static ssize_t n_null_read(struct tty_struct *tty, struct file *file, - unsigned char __user * buf, size_t nr) + unsigned char *buf, size_t nr, + void **cookie, unsigned long offset) { return -EOPNOTSUPP; } diff --git a/drivers/tty/n_r3964.c b/drivers/tty/n_r3964.c index 934dd2fb2ec8..3161f0a535e3 100644 --- a/drivers/tty/n_r3964.c +++ b/drivers/tty/n_r3964.c @@ -129,7 +129,7 @@ static void remove_client_block(struct r3964_info *pInfo, static int r3964_open(struct tty_struct *tty); static void r3964_close(struct tty_struct *tty); static ssize_t r3964_read(struct tty_struct *tty, struct file *file, - unsigned char __user * buf, size_t nr); + void *cookie, unsigned char *buf, size_t nr); static ssize_t r3964_write(struct tty_struct *tty, struct file *file, const unsigned char *buf, size_t nr); static int r3964_ioctl(struct tty_struct *tty, struct file *file, @@ -1058,7 +1058,8 @@ static void r3964_close(struct tty_struct *tty) } static ssize_t r3964_read(struct tty_struct *tty, struct file *file, - unsigned char __user * buf, size_t nr) + unsigned char *kbuf, size_t nr, + void **cookie, unsigned long offset) { struct r3964_info *pInfo = tty->disc_data; struct r3964_client_info *pClient; @@ -1109,10 +1110,7 @@ static ssize_t r3964_read(struct tty_struct *tty, struct file *file, kfree(pMsg); TRACE_M("r3964_read - msg kfree %p", pMsg); - if (copy_to_user(buf, &theMsg, ret)) { - ret = -EFAULT; - goto unlock; - } + memcpy(kbuf, &theMsg, ret); TRACE_PS("read - return %d", ret); goto unlock; diff --git a/drivers/tty/n_tracerouter.c b/drivers/tty/n_tracerouter.c index 4479af4d2fa5..3490ed51b1a3 100644 --- a/drivers/tty/n_tracerouter.c +++ b/drivers/tty/n_tracerouter.c @@ -118,7 +118,9 @@ static void n_tracerouter_close(struct tty_struct *tty) * -EINVAL */ static ssize_t n_tracerouter_read(struct tty_struct *tty, struct file *file, - unsigned char __user *buf, size_t nr) { + unsigned char *buf, size_t nr, + void **cookie, unsigned long offset) +{ return -EINVAL; } diff --git a/drivers/tty/n_tracesink.c b/drivers/tty/n_tracesink.c index d96ba82cc356..1d9931041fd8 100644 --- a/drivers/tty/n_tracesink.c +++ b/drivers/tty/n_tracesink.c @@ -115,7 +115,9 @@ static void n_tracesink_close(struct tty_struct *tty) * -EINVAL */ static ssize_t n_tracesink_read(struct tty_struct *tty, struct file *file, - unsigned char __user *buf, size_t nr) { + unsigned char *buf, size_t nr, + void **cookie, unsigned long offset) +{ return -EINVAL; } diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 7e5e36315260..4a34a9f43b29 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -164,29 +164,24 @@ static void zero_buffer(struct tty_struct *tty, u8 *buffer, int size) memset(buffer, 0x00, size); } -static int tty_copy_to_user(struct tty_struct *tty, void __user *to, - size_t tail, size_t n) +static void tty_copy(struct tty_struct *tty, void *to, size_t tail, size_t n) { struct n_tty_data *ldata = tty->disc_data; size_t size = N_TTY_BUF_SIZE - tail; void *from = read_buf_addr(ldata, tail); - int uncopied; if (n > size) { tty_audit_add_data(tty, from, size); - uncopied = copy_to_user(to, from, size); - zero_buffer(tty, from, size - uncopied); - if (uncopied) - return uncopied; + memcpy(to, from, size); + zero_buffer(tty, from, size); to += size; n -= size; from = ldata->read_buf; } tty_audit_add_data(tty, from, n); - uncopied = copy_to_user(to, from, n); - zero_buffer(tty, from, n - uncopied); - return uncopied; + memcpy(to, from, n); + zero_buffer(tty, from, n); } /** @@ -1942,15 +1937,16 @@ static inline int input_available_p(struct tty_struct *tty, int poll) /** * copy_from_read_buf - copy read data directly * @tty: terminal device - * @b: user data + * @kbp: data * @nr: size of data * * Helper function to speed up n_tty_read. It is only called when - * ICANON is off; it copies characters straight from the tty queue to - * user space directly. It can be profitably called twice; once to - * drain the space from the tail pointer to the (physical) end of the - * buffer, and once to drain the space from the (physical) beginning of - * the buffer to head pointer. + * ICANON is off; it copies characters straight from the tty queue. + * + * It can be profitably called twice; once to drain the space from + * the tail pointer to the (physical) end of the buffer, and once + * to drain the space from the (physical) beginning of the buffer + * to head pointer. * * Called under the ldata->atomic_read_lock sem * @@ -1960,7 +1956,7 @@ static inline int input_available_p(struct tty_struct *tty, int poll) */ static int copy_from_read_buf(struct tty_struct *tty, - unsigned char __user **b, + unsigned char **kbp, size_t *nr) { @@ -1976,8 +1972,7 @@ static int copy_from_read_buf(struct tty_struct *tty, n = min(*nr, n); if (n) { unsigned char *from = read_buf_addr(ldata, tail); - retval = copy_to_user(*b, from, n); - n -= retval; + memcpy(*kbp, from, n); is_eof = n == 1 && *from == EOF_CHAR(tty); tty_audit_add_data(tty, from, n); zero_buffer(tty, from, n); @@ -1986,7 +1981,7 @@ static int copy_from_read_buf(struct tty_struct *tty, if (L_EXTPROC(tty) && ldata->icanon && is_eof && (head == ldata->read_tail)) n = 0; - *b += n; + *kbp += n; *nr -= n; } return retval; @@ -1995,12 +1990,12 @@ static int copy_from_read_buf(struct tty_struct *tty, /** * canon_copy_from_read_buf - copy read data in canonical mode * @tty: terminal device - * @b: user data + * @kbp: data * @nr: size of data * * Helper function for n_tty_read. It is only called when ICANON is on; * it copies one line of input up to and including the line-delimiting - * character into the user-space buffer. + * character into the result buffer. * * NB: When termios is changed from non-canonical to canonical mode and * the read buffer contains data, n_tty_set_termios() simulates an EOF @@ -2016,14 +2011,14 @@ static int copy_from_read_buf(struct tty_struct *tty, */ static int canon_copy_from_read_buf(struct tty_struct *tty, - unsigned char __user **b, + unsigned char **kbp, size_t *nr) { struct n_tty_data *ldata = tty->disc_data; size_t n, size, more, c; size_t eol; size_t tail; - int ret, found = 0; + int found = 0; /* N.B. avoid overrun if nr == 0 */ if (!*nr) @@ -2059,10 +2054,8 @@ static int canon_copy_from_read_buf(struct tty_struct *tty, n_tty_trace("%s: eol:%zu found:%d n:%zu c:%zu tail:%zu more:%zu\n", __func__, eol, found, n, c, tail, more); - ret = tty_copy_to_user(tty, *b, tail, n); - if (ret) - return -EFAULT; - *b += n; + tty_copy(tty, *kbp, tail, n); + *kbp += n; *nr -= n; if (found) @@ -2130,10 +2123,11 @@ static int job_control(struct tty_struct *tty, struct file *file) */ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, - unsigned char __user *buf, size_t nr) + unsigned char *kbuf, size_t nr, + void **cookie, unsigned long offset) { struct n_tty_data *ldata = tty->disc_data; - unsigned char __user *b = buf; + unsigned char *kb = kbuf; DEFINE_WAIT_FUNC(wait, woken_wake_function); int c; int minimum, time; @@ -2179,17 +2173,13 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, /* First test for status change. */ if (packet && tty->link->ctrl_status) { unsigned char cs; - if (b != buf) + if (kb != kbuf) break; spin_lock_irq(&tty->link->ctrl_lock); cs = tty->link->ctrl_status; tty->link->ctrl_status = 0; spin_unlock_irq(&tty->link->ctrl_lock); - if (put_user(cs, b)) { - retval = -EFAULT; - break; - } - b++; + *kb++ = cs; nr--; break; } @@ -2232,24 +2222,20 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, } if (ldata->icanon && !L_EXTPROC(tty)) { - retval = canon_copy_from_read_buf(tty, &b, &nr); + retval = canon_copy_from_read_buf(tty, &kb, &nr); if (retval) break; } else { int uncopied; /* Deal with packet mode. */ - if (packet && b == buf) { - if (put_user(TIOCPKT_DATA, b)) { - retval = -EFAULT; - break; - } - b++; + if (packet && kb == kbuf) { + *kb++ = TIOCPKT_DATA; nr--; } - uncopied = copy_from_read_buf(tty, &b, &nr); - uncopied += copy_from_read_buf(tty, &b, &nr); + uncopied = copy_from_read_buf(tty, &kb, &nr); + uncopied += copy_from_read_buf(tty, &kb, &nr); if (uncopied) { retval = -EFAULT; break; @@ -2258,7 +2244,7 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, n_tty_check_unthrottle(tty); - if (b - buf >= minimum) + if (kb - kbuf >= minimum) break; if (time) timeout = time; @@ -2270,8 +2256,8 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, remove_wait_queue(&tty->read_wait, &wait); mutex_unlock(&ldata->atomic_read_lock); - if (b - buf) - retval = b - buf; + if (kb - kbuf) + retval = kb - kbuf; return retval; } diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index 338bc4ef5549..a34f8bcf875e 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -833,6 +833,65 @@ static void tty_update_time(struct timespec64 *time) time->tv_sec = sec; } +/* + * Iterate on the ldisc ->read() function until we've gotten all + * the data the ldisc has for us. + * + * The "cookie" is something that the ldisc read function can fill + * in to let us know that there is more data to be had. + * + * We promise to continue to call the ldisc until it stops returning + * data or clears the cookie. The cookie may be something that the + * ldisc maintains state for and needs to free. + */ +static int iterate_tty_read(struct tty_ldisc *ld, struct tty_struct *tty, struct file *file, + char __user *buf, size_t count) +{ + int retval = 0; + void *cookie = NULL; + unsigned long offset = 0; + char kernel_buf[64]; + + do { + int size, uncopied; + + size = count > sizeof(kernel_buf) ? sizeof(kernel_buf) : count; + size = ld->ops->read(tty, file, kernel_buf, size, &cookie, offset); + if (!size) + break; + + /* + * A ldisc read error return will override any previously copied + * data (eg -EOVERFLOW from HDLC) + */ + if (size < 0) { + memzero_explicit(kernel_buf, sizeof(kernel_buf)); + return size; + } + + uncopied = copy_to_user(buf+offset, kernel_buf, size); + size -= uncopied; + offset += size; + count -= size; + + /* + * If the user copy failed, we still need to do another ->read() + * call if we had a cookie to let the ldisc clear up. + * + * But make sure size is zeroed. + */ + if (unlikely(uncopied)) { + count = 0; + retval = -EFAULT; + } + } while (cookie); + + /* We always clear tty buffer in case they contained passwords */ + memzero_explicit(kernel_buf, sizeof(kernel_buf)); + return offset ? offset : retval; +} + + /** * tty_read - read method for tty device files * @file: pointer to tty file @@ -866,10 +925,9 @@ static ssize_t tty_read(struct file *file, char __user *buf, size_t count, ld = tty_ldisc_ref_wait(tty); if (!ld) return hung_up_tty_read(file, buf, count, ppos); + i = -EIO; if (ld->ops->read) - i = ld->ops->read(tty, file, buf, count); - else - i = -EIO; + i = iterate_tty_read(ld, tty, file, buf, count); tty_ldisc_deref(ld); if (i > 0) diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h index b1e6043e9917..572a07976116 100644 --- a/include/linux/tty_ldisc.h +++ b/include/linux/tty_ldisc.h @@ -185,7 +185,8 @@ struct tty_ldisc_ops { void (*close)(struct tty_struct *); void (*flush_buffer)(struct tty_struct *tty); ssize_t (*read)(struct tty_struct *tty, struct file *file, - unsigned char __user *buf, size_t nr); + unsigned char *buf, size_t nr, + void **cookie, unsigned long offset); ssize_t (*write)(struct tty_struct *tty, struct file *file, const unsigned char *buf, size_t nr); int (*ioctl)(struct tty_struct *tty, struct file *file, diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c index 11b554ce07ff..1204c438e87d 100644 --- a/net/nfc/nci/uart.c +++ b/net/nfc/nci/uart.c @@ -292,7 +292,8 @@ static int nci_uart_tty_ioctl(struct tty_struct *tty, struct file *file, /* We don't provide read/write/poll interface for user space. */ static ssize_t nci_uart_tty_read(struct tty_struct *tty, struct file *file, - unsigned char __user *buf, size_t nr) + unsigned char *buf, size_t nr, + void **cookie, unsigned long offset) { return 0; } -- cgit v1.2.3 From 64a69892afadd6fffaeadc65427bb7601161139d Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Tue, 19 Jan 2021 13:46:28 -0800 Subject: tty: clean up legacy leftovers from n_tty line discipline Back when the line disciplines did their own direct user accesses, they had to deal with the data copy possibly failing in the middle. Now that the user copy is done by the tty_io.c code, that failure case no longer exists. Remove the left-over error handling code that cannot trigger. Signed-off-by: Linus Torvalds --- drivers/tty/n_tty.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) (limited to 'drivers/tty/n_tty.c') diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 4a34a9f43b29..3a1a79462d16 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -1955,19 +1955,17 @@ static inline int input_available_p(struct tty_struct *tty, int poll) * read_tail published */ -static int copy_from_read_buf(struct tty_struct *tty, +static void copy_from_read_buf(struct tty_struct *tty, unsigned char **kbp, size_t *nr) { struct n_tty_data *ldata = tty->disc_data; - int retval; size_t n; bool is_eof; size_t head = smp_load_acquire(&ldata->commit_head); size_t tail = ldata->read_tail & (N_TTY_BUF_SIZE - 1); - retval = 0; n = min(head - ldata->read_tail, N_TTY_BUF_SIZE - tail); n = min(*nr, n); if (n) { @@ -1984,7 +1982,6 @@ static int copy_from_read_buf(struct tty_struct *tty, *kbp += n; *nr -= n; } - return retval; } /** @@ -2010,9 +2007,9 @@ static int copy_from_read_buf(struct tty_struct *tty, * read_tail published */ -static int canon_copy_from_read_buf(struct tty_struct *tty, - unsigned char **kbp, - size_t *nr) +static void canon_copy_from_read_buf(struct tty_struct *tty, + unsigned char **kbp, + size_t *nr) { struct n_tty_data *ldata = tty->disc_data; size_t n, size, more, c; @@ -2022,7 +2019,7 @@ static int canon_copy_from_read_buf(struct tty_struct *tty, /* N.B. avoid overrun if nr == 0 */ if (!*nr) - return 0; + return; n = min(*nr + 1, smp_load_acquire(&ldata->canon_head) - ldata->read_tail); @@ -2069,7 +2066,6 @@ static int canon_copy_from_read_buf(struct tty_struct *tty, ldata->push = 0; tty_audit_push(); } - return 0; } extern ssize_t redirected_tty_write(struct file *, const char __user *, @@ -2222,24 +2218,17 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, } if (ldata->icanon && !L_EXTPROC(tty)) { - retval = canon_copy_from_read_buf(tty, &kb, &nr); - if (retval) - break; + canon_copy_from_read_buf(tty, &kb, &nr); } else { - int uncopied; - /* Deal with packet mode. */ if (packet && kb == kbuf) { *kb++ = TIOCPKT_DATA; nr--; } - uncopied = copy_from_read_buf(tty, &kb, &nr); - uncopied += copy_from_read_buf(tty, &kb, &nr); - if (uncopied) { - retval = -EFAULT; - break; - } + /* See comment above copy_from_read_buf() why twice */ + copy_from_read_buf(tty, &kb, &nr); + copy_from_read_buf(tty, &kb, &nr); } n_tty_check_unthrottle(tty); -- cgit v1.2.3 From 15ea8ae8e03fdb845ed3ff5d9f11dd5f4f60252c Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Tue, 19 Jan 2021 18:14:20 -0800 Subject: tty: teach n_tty line discipline about the new "cookie continuations" With the conversion to do the tty ldisc read operations in small chunks, the n_tty line discipline became noticeably slower for throughput oriented loads, because rather than read things in up to 2kB chunks, it would return at most 64 bytes per read() system call. The cost is mainly all in the "do system calls over and over", not really in the new "copy to an extra kernel buffer". This can be fixed by teaching the n_tty line discipline about the "cookie continuation" model, which the chunking code supports because things like hdlc need to be able to handle packets up to 64kB in size. Doing that doesn't just get us back to the old performace, but to much better performance: my stupid "copy 10MB of data over a pty" test program is now almost twice as fast as it used to be (going down from 0.1s to 0.054s). This is entirely because it now creates maximal chunks (which happens to be "one byte less than one page" due to how we do the circular tty buffers). NOTE! This case only handles the simpler non-icanon case, which is the one where people may care about throughput. I'm going to do the icanon case later too, because while performance isn't a major issue for that, there may be programs that think they'll always get a full line and don't like the 64-byte chunking for that reason. Such programs are arguably buggy (signals etc can cause random partial results from tty reads anyway), and good programs will handle such partial reads, but expecting everybody to write "good programs" has never been a winning policy for the kernel.. Signed-off-by: Linus Torvalds --- drivers/tty/n_tty.c | 52 ++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 10 deletions(-) (limited to 'drivers/tty/n_tty.c') diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 3a1a79462d16..b89308d52ade 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -1943,19 +1943,17 @@ static inline int input_available_p(struct tty_struct *tty, int poll) * Helper function to speed up n_tty_read. It is only called when * ICANON is off; it copies characters straight from the tty queue. * - * It can be profitably called twice; once to drain the space from - * the tail pointer to the (physical) end of the buffer, and once - * to drain the space from the (physical) beginning of the buffer - * to head pointer. - * * Called under the ldata->atomic_read_lock sem * + * Returns true if it successfully copied data, but there is still + * more data to be had. + * * n_tty_read()/consumer path: * caller holds non-exclusive termios_rwsem * read_tail published */ -static void copy_from_read_buf(struct tty_struct *tty, +static bool copy_from_read_buf(struct tty_struct *tty, unsigned char **kbp, size_t *nr) @@ -1978,10 +1976,14 @@ static void copy_from_read_buf(struct tty_struct *tty, /* Turn single EOF into zero-length read */ if (L_EXTPROC(tty) && ldata->icanon && is_eof && (head == ldata->read_tail)) - n = 0; + return false; *kbp += n; *nr -= n; + + /* If we have more to copy, let the caller know */ + return head != ldata->read_tail; } + return false; } /** @@ -2132,6 +2134,25 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, int packet; size_t tail; + /* + * Is this a continuation of a read started earler? + * + * If so, we still hold the atomic_read_lock and the + * termios_rwsem, and can just continue to copy data. + */ + if (*cookie) { + if (copy_from_read_buf(tty, &kb, &nr)) + return kb - kbuf; + + /* No more data - release locks and stop retries */ + n_tty_kick_worker(tty); + n_tty_check_unthrottle(tty); + up_read(&tty->termios_rwsem); + mutex_unlock(&ldata->atomic_read_lock); + *cookie = NULL; + return kb - kbuf; + } + c = job_control(tty, file); if (c < 0) return c; @@ -2226,9 +2247,20 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, nr--; } - /* See comment above copy_from_read_buf() why twice */ - copy_from_read_buf(tty, &kb, &nr); - copy_from_read_buf(tty, &kb, &nr); + /* + * Copy data, and if there is more to be had + * and we have nothing more to wait for, then + * let's mark us for retries. + * + * NOTE! We return here with both the termios_sem + * and atomic_read_lock still held, the retries + * will release them when done. + */ + if (copy_from_read_buf(tty, &kb, &nr) && kb - kbuf >= minimum) { + remove_wait_queue(&tty->read_wait, &wait); + *cookie = cookie; + return kb - kbuf; + } } n_tty_check_unthrottle(tty); -- cgit v1.2.3 From d7fe75cbc23c7d225eee2ef04def239b6603dce7 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Wed, 20 Jan 2021 15:43:38 -0800 Subject: tty: teach the n_tty ICANON case about the new "cookie continuations" too The ICANON case is a bit messy, since it has to look for the line ending, and has special code to then suppress line ending characters if they match the __DISABLED_CHAR. So it actually looks up the line ending even past the point where it knows it won't copy it to the result buffer. That said, apart from all those odd legacy N_TTY ICANON cases, the actual "should we continue copying" logic isn't really all that complicated or different from the non-canon case. In fact, the lack of "wait for at least N characters" arguably makes the repeat case slightly simpler. It really just boils down to "there's more of the line to be copied". So add the necessarily trivial logic, and now the N_TTY case will give long result lines even when in canon mode. Signed-off-by: Linus Torvalds --- drivers/tty/n_tty.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) (limited to 'drivers/tty/n_tty.c') diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index b89308d52ade..9e546d0cc55c 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -2009,21 +2009,22 @@ static bool copy_from_read_buf(struct tty_struct *tty, * read_tail published */ -static void canon_copy_from_read_buf(struct tty_struct *tty, +static bool canon_copy_from_read_buf(struct tty_struct *tty, unsigned char **kbp, size_t *nr) { struct n_tty_data *ldata = tty->disc_data; size_t n, size, more, c; size_t eol; - size_t tail; + size_t tail, canon_head; int found = 0; /* N.B. avoid overrun if nr == 0 */ if (!*nr) - return; + return false; - n = min(*nr + 1, smp_load_acquire(&ldata->canon_head) - ldata->read_tail); + canon_head = smp_load_acquire(&ldata->canon_head); + n = min(*nr + 1, canon_head - ldata->read_tail); tail = ldata->read_tail & (N_TTY_BUF_SIZE - 1); size = min_t(size_t, tail + n, N_TTY_BUF_SIZE); @@ -2067,7 +2068,11 @@ static void canon_copy_from_read_buf(struct tty_struct *tty, else ldata->push = 0; tty_audit_push(); + return false; } + + /* No EOL found - do a continuation retry if there is more data */ + return ldata->read_tail != canon_head; } extern ssize_t redirected_tty_write(struct file *, const char __user *, @@ -2141,8 +2146,13 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, * termios_rwsem, and can just continue to copy data. */ if (*cookie) { - if (copy_from_read_buf(tty, &kb, &nr)) - return kb - kbuf; + if (ldata->icanon && !L_EXTPROC(tty)) { + if (canon_copy_from_read_buf(tty, &kb, &nr)) + return kb - kbuf; + } else { + if (copy_from_read_buf(tty, &kb, &nr)) + return kb - kbuf; + } /* No more data - release locks and stop retries */ n_tty_kick_worker(tty); @@ -2239,7 +2249,8 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, } if (ldata->icanon && !L_EXTPROC(tty)) { - canon_copy_from_read_buf(tty, &kb, &nr); + if (canon_copy_from_read_buf(tty, &kb, &nr)) + goto more_to_be_read; } else { /* Deal with packet mode. */ if (packet && kb == kbuf) { @@ -2257,6 +2268,7 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, * will release them when done. */ if (copy_from_read_buf(tty, &kb, &nr) && kb - kbuf >= minimum) { +more_to_be_read: remove_wait_queue(&tty->read_wait, &wait); *cookie = cookie; return kb - kbuf; -- cgit v1.2.3