All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/6] tty: implement write_iter
@ 2021-01-21  9:00 Greg Kroah-Hartman
  2021-01-21  9:00 ` [PATCH 2/6] tty: convert tty_ldisc_ops 'read()' function to take a kernel pointer Greg Kroah-Hartman
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-21  9:00 UTC (permalink / raw)
  To: jirislaby, linux-serial
  Cc: gregkh, hch, viro, linux-kernel, ohw.giles, r.karszniewicz,
	Linus Torvalds

From: Linus Torvalds <torvalds@linux-foundation.org>

This makes the tty layer use the .write_iter() function instead of the
traditional .write() functionality.

That allows writev(), but more importantly also makes it possible to
enable .splice_write() for ttys, reinstating the "splice to tty"
functionality that was lost in commit 36e2c7421f02 ("fs: don't allow
splice read/write without explicit ops").

Fixes: 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
Reported-by: Oliver Giles <ohw.giles@gmail.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 drivers/tty/tty_io.c | 48 ++++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 56ade99ef99f..338bc4ef5549 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -143,9 +143,8 @@ LIST_HEAD(tty_drivers);			/* linked list of tty drivers */
 DEFINE_MUTEX(tty_mutex);
 
 static ssize_t tty_read(struct file *, char __user *, size_t, loff_t *);
-static ssize_t tty_write(struct file *, const char __user *, size_t, loff_t *);
-ssize_t redirected_tty_write(struct file *, const char __user *,
-							size_t, loff_t *);
+static ssize_t tty_write(struct kiocb *, struct iov_iter *);
+ssize_t redirected_tty_write(struct kiocb *, struct iov_iter *);
 static __poll_t tty_poll(struct file *, poll_table *);
 static int tty_open(struct inode *, struct file *);
 long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
@@ -478,7 +477,8 @@ static void tty_show_fdinfo(struct seq_file *m, struct file *file)
 static const struct file_operations tty_fops = {
 	.llseek		= no_llseek,
 	.read		= tty_read,
-	.write		= tty_write,
+	.write_iter	= tty_write,
+	.splice_write	= iter_file_splice_write,
 	.poll		= tty_poll,
 	.unlocked_ioctl	= tty_ioctl,
 	.compat_ioctl	= tty_compat_ioctl,
@@ -491,7 +491,8 @@ static const struct file_operations tty_fops = {
 static const struct file_operations console_fops = {
 	.llseek		= no_llseek,
 	.read		= tty_read,
-	.write		= redirected_tty_write,
+	.write_iter	= redirected_tty_write,
+	.splice_write	= iter_file_splice_write,
 	.poll		= tty_poll,
 	.unlocked_ioctl	= tty_ioctl,
 	.compat_ioctl	= tty_compat_ioctl,
@@ -607,9 +608,9 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
 	/* This breaks for file handles being sent over AF_UNIX sockets ? */
 	list_for_each_entry(priv, &tty->tty_files, list) {
 		filp = priv->file;
-		if (filp->f_op->write == redirected_tty_write)
+		if (filp->f_op->write_iter == redirected_tty_write)
 			cons_filp = filp;
-		if (filp->f_op->write != tty_write)
+		if (filp->f_op->write_iter != tty_write)
 			continue;
 		closecount++;
 		__tty_fasync(-1, filp, 0);	/* can't block */
@@ -902,9 +903,9 @@ static inline ssize_t do_tty_write(
 	ssize_t (*write)(struct tty_struct *, struct file *, const unsigned char *, size_t),
 	struct tty_struct *tty,
 	struct file *file,
-	const char __user *buf,
-	size_t count)
+	struct iov_iter *from)
 {
+	size_t count = iov_iter_count(from);
 	ssize_t ret, written = 0;
 	unsigned int chunk;
 
@@ -956,14 +957,20 @@ static inline ssize_t do_tty_write(
 		size_t size = count;
 		if (size > chunk)
 			size = chunk;
+
 		ret = -EFAULT;
-		if (copy_from_user(tty->write_buf, buf, size))
+		if (copy_from_iter(tty->write_buf, size, from) != size)
 			break;
+
 		ret = write(tty, file, tty->write_buf, size);
 		if (ret <= 0)
 			break;
+
+		/* FIXME! Have Al check this! */
+		if (ret != size)
+			iov_iter_revert(from, size-ret);
+
 		written += ret;
-		buf += ret;
 		count -= ret;
 		if (!count)
 			break;
@@ -1023,9 +1030,9 @@ void tty_write_message(struct tty_struct *tty, char *msg)
  *	write method will not be invoked in parallel for each device.
  */
 
-static ssize_t tty_write(struct file *file, const char __user *buf,
-						size_t count, loff_t *ppos)
+static ssize_t tty_write(struct kiocb *iocb, struct iov_iter *from)
 {
+	struct file *file = iocb->ki_filp;
 	struct tty_struct *tty = file_tty(file);
  	struct tty_ldisc *ld;
 	ssize_t ret;
@@ -1038,18 +1045,15 @@ static ssize_t tty_write(struct file *file, const char __user *buf,
 	if (tty->ops->write_room == NULL)
 		tty_err(tty, "missing write_room method\n");
 	ld = tty_ldisc_ref_wait(tty);
-	if (!ld)
-		return hung_up_tty_write(file, buf, count, ppos);
-	if (!ld->ops->write)
+	if (!ld || !ld->ops->write)
 		ret = -EIO;
 	else
-		ret = do_tty_write(ld->ops->write, tty, file, buf, count);
+		ret = do_tty_write(ld->ops->write, tty, file, from);
 	tty_ldisc_deref(ld);
 	return ret;
 }
 
-ssize_t redirected_tty_write(struct file *file, const char __user *buf,
-						size_t count, loff_t *ppos)
+ssize_t redirected_tty_write(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct file *p = NULL;
 
@@ -1060,11 +1064,11 @@ ssize_t redirected_tty_write(struct file *file, const char __user *buf,
 
 	if (p) {
 		ssize_t res;
-		res = vfs_write(p, buf, count, &p->f_pos);
+		res = vfs_iocb_iter_write(p, iocb, iter);
 		fput(p);
 		return res;
 	}
-	return tty_write(file, buf, count, ppos);
+	return tty_write(iocb, iter);
 }
 
 /**
@@ -2293,7 +2297,7 @@ static int tioccons(struct file *file)
 {
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
-	if (file->f_op->write == redirected_tty_write) {
+	if (file->f_op->write_iter == redirected_tty_write) {
 		struct file *f;
 		spin_lock(&redirect_lock);
 		f = redirect;
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/6] tty: convert tty_ldisc_ops 'read()' function to take a kernel pointer
  2021-01-21  9:00 [PATCH 1/6] tty: implement write_iter Greg Kroah-Hartman
@ 2021-01-21  9:00 ` Greg Kroah-Hartman
  2021-01-21 11:02   ` Jiri Slaby
  2021-01-21  9:00 ` [PATCH 3/6] tty: implement read_iter Greg Kroah-Hartman
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-21  9:00 UTC (permalink / raw)
  To: jirislaby, linux-serial
  Cc: gregkh, hch, viro, linux-kernel, ohw.giles, r.karszniewicz,
	Linus Torvalds

From: Linus Torvalds <torvalds@linux-foundation.org>

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 <hch@lst.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 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(-)

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 @@ static void serport_ldisc_receive(struct tty_struct *tty, const unsigned char *c
  * 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;
 }
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/6] tty: implement read_iter
  2021-01-21  9:00 [PATCH 1/6] tty: implement write_iter Greg Kroah-Hartman
  2021-01-21  9:00 ` [PATCH 2/6] tty: convert tty_ldisc_ops 'read()' function to take a kernel pointer Greg Kroah-Hartman
@ 2021-01-21  9:00 ` Greg Kroah-Hartman
  2021-01-21 10:44   ` Jiri Slaby
  2021-01-21  9:00 ` [PATCH 4/6] tty: clean up legacy leftovers from n_tty line discipline Greg Kroah-Hartman
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-21  9:00 UTC (permalink / raw)
  To: jirislaby, linux-serial
  Cc: gregkh, hch, viro, linux-kernel, ohw.giles, r.karszniewicz,
	Linus Torvalds

From: Linus Torvalds <torvalds@linux-foundation.org>

Now that the ldisc read() function takes kernel pointers, it's fairly
straightforward to make the tty file operations use .read_iter() instead
of .read().

That automatically gives us vread() and friends, and also makes it
possible to do .splice_read() on ttys again.

Fixes: 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
Reported-by: Oliver Giles <ohw.giles@gmail.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 drivers/tty/tty_io.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index a34f8bcf875e..8846d3b99845 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -142,7 +142,7 @@ LIST_HEAD(tty_drivers);			/* linked list of tty drivers */
 /* Mutex to protect creating and releasing a tty */
 DEFINE_MUTEX(tty_mutex);
 
-static ssize_t tty_read(struct file *, char __user *, size_t, loff_t *);
+static ssize_t tty_read(struct kiocb *, struct iov_iter *);
 static ssize_t tty_write(struct kiocb *, struct iov_iter *);
 ssize_t redirected_tty_write(struct kiocb *, struct iov_iter *);
 static __poll_t tty_poll(struct file *, poll_table *);
@@ -476,8 +476,9 @@ static void tty_show_fdinfo(struct seq_file *m, struct file *file)
 
 static const struct file_operations tty_fops = {
 	.llseek		= no_llseek,
-	.read		= tty_read,
+	.read_iter	= tty_read,
 	.write_iter	= tty_write,
+	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.poll		= tty_poll,
 	.unlocked_ioctl	= tty_ioctl,
@@ -490,8 +491,9 @@ static const struct file_operations tty_fops = {
 
 static const struct file_operations console_fops = {
 	.llseek		= no_llseek,
-	.read		= tty_read,
+	.read_iter	= tty_read,
 	.write_iter	= redirected_tty_write,
+	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.poll		= tty_poll,
 	.unlocked_ioctl	= tty_ioctl,
@@ -844,16 +846,17 @@ static void tty_update_time(struct timespec64 *time)
  * 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)
+static int iterate_tty_read(struct tty_ldisc *ld, struct tty_struct *tty,
+		struct file *file, struct iov_iter *to)
 {
 	int retval = 0;
 	void *cookie = NULL;
 	unsigned long offset = 0;
 	char kernel_buf[64];
+	size_t count = iov_iter_count(to);
 
 	do {
-		int size, uncopied;
+		int size, copied;
 
 		size = count > sizeof(kernel_buf) ? sizeof(kernel_buf) : count;
 		size = ld->ops->read(tty, file, kernel_buf, size, &cookie, offset);
@@ -869,10 +872,9 @@ static int iterate_tty_read(struct tty_ldisc *ld, struct tty_struct *tty, struct
 			return size;
 		}
 
-		uncopied = copy_to_user(buf+offset, kernel_buf, size);
-		size -= uncopied;
-		offset += size;
-		count -= size;
+		copied = copy_to_iter(kernel_buf, size, to);
+		offset += copied;
+		count -= copied;
 
 		/*
 		 * If the user copy failed, we still need to do another ->read()
@@ -880,7 +882,7 @@ static int iterate_tty_read(struct tty_ldisc *ld, struct tty_struct *tty, struct
 		 *
 		 * But make sure size is zeroed.
 		 */
-		if (unlikely(uncopied)) {
+		if (unlikely(copied != size)) {
 			count = 0;
 			retval = -EFAULT;
 		}
@@ -907,10 +909,10 @@ static int iterate_tty_read(struct tty_ldisc *ld, struct tty_struct *tty, struct
  *	read calls may be outstanding in parallel.
  */
 
-static ssize_t tty_read(struct file *file, char __user *buf, size_t count,
-			loff_t *ppos)
+static ssize_t tty_read(struct kiocb *iocb, struct iov_iter *to)
 {
 	int i;
+	struct file *file = iocb->ki_filp;
 	struct inode *inode = file_inode(file);
 	struct tty_struct *tty = file_tty(file);
 	struct tty_ldisc *ld;
@@ -923,11 +925,9 @@ static ssize_t tty_read(struct file *file, char __user *buf, size_t count,
 	/* We want to wait for the line discipline to sort out in this
 	   situation */
 	ld = tty_ldisc_ref_wait(tty);
-	if (!ld)
-		return hung_up_tty_read(file, buf, count, ppos);
 	i = -EIO;
-	if (ld->ops->read)
-		i = iterate_tty_read(ld, tty, file, buf, count);
+	if (ld && ld->ops->read)
+		i = iterate_tty_read(ld, tty, file, to);
 	tty_ldisc_deref(ld);
 
 	if (i > 0)
@@ -2927,7 +2927,7 @@ static long tty_compat_ioctl(struct file *file, unsigned int cmd,
 
 static int this_tty(const void *t, struct file *file, unsigned fd)
 {
-	if (likely(file->f_op->read != tty_read))
+	if (likely(file->f_op->read_iter != tty_read))
 		return 0;
 	return file_tty(file) != t ? 0 : fd + 1;
 }
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 4/6] tty: clean up legacy leftovers from n_tty line discipline
  2021-01-21  9:00 [PATCH 1/6] tty: implement write_iter Greg Kroah-Hartman
  2021-01-21  9:00 ` [PATCH 2/6] tty: convert tty_ldisc_ops 'read()' function to take a kernel pointer Greg Kroah-Hartman
  2021-01-21  9:00 ` [PATCH 3/6] tty: implement read_iter Greg Kroah-Hartman
@ 2021-01-21  9:00 ` Greg Kroah-Hartman
  2021-01-21  9:00 ` [PATCH 5/6] tty: teach n_tty line discipline about the new "cookie continuations" Greg Kroah-Hartman
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-21  9:00 UTC (permalink / raw)
  To: jirislaby, linux-serial
  Cc: gregkh, hch, viro, linux-kernel, ohw.giles, r.karszniewicz,
	Linus Torvalds

From: Linus Torvalds <torvalds@linux-foundation.org>

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 <torvalds@linux-foundation.org>
---
 drivers/tty/n_tty.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

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);
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 5/6] tty: teach n_tty line discipline about the new "cookie continuations"
  2021-01-21  9:00 [PATCH 1/6] tty: implement write_iter Greg Kroah-Hartman
                   ` (2 preceding siblings ...)
  2021-01-21  9:00 ` [PATCH 4/6] tty: clean up legacy leftovers from n_tty line discipline Greg Kroah-Hartman
@ 2021-01-21  9:00 ` Greg Kroah-Hartman
  2021-01-21  9:00 ` [PATCH 6/6] tty: teach the n_tty ICANON case about the new "cookie continuations" too Greg Kroah-Hartman
  2021-01-21  9:39 ` [PATCH 1/6] tty: implement write_iter Jiri Slaby
  5 siblings, 0 replies; 19+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-21  9:00 UTC (permalink / raw)
  To: jirislaby, linux-serial
  Cc: gregkh, hch, viro, linux-kernel, ohw.giles, r.karszniewicz,
	Linus Torvalds

From: Linus Torvalds <torvalds@linux-foundation.org>

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 <torvalds@linux-foundation.org>
---
 drivers/tty/n_tty.c | 52 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 10 deletions(-)

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);
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 6/6] tty: teach the n_tty ICANON case about the new "cookie continuations" too
  2021-01-21  9:00 [PATCH 1/6] tty: implement write_iter Greg Kroah-Hartman
                   ` (3 preceding siblings ...)
  2021-01-21  9:00 ` [PATCH 5/6] tty: teach n_tty line discipline about the new "cookie continuations" Greg Kroah-Hartman
@ 2021-01-21  9:00 ` Greg Kroah-Hartman
  2021-01-21  9:39 ` [PATCH 1/6] tty: implement write_iter Jiri Slaby
  5 siblings, 0 replies; 19+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-21  9:00 UTC (permalink / raw)
  To: jirislaby, linux-serial
  Cc: gregkh, hch, viro, linux-kernel, ohw.giles, r.karszniewicz,
	Linus Torvalds

From: Linus Torvalds <torvalds@linux-foundation.org>

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 <torvalds@linux-foundation.org>
---
 drivers/tty/n_tty.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

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;
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/6] tty: implement write_iter
  2021-01-21  9:00 [PATCH 1/6] tty: implement write_iter Greg Kroah-Hartman
                   ` (4 preceding siblings ...)
  2021-01-21  9:00 ` [PATCH 6/6] tty: teach the n_tty ICANON case about the new "cookie continuations" too Greg Kroah-Hartman
@ 2021-01-21  9:39 ` Jiri Slaby
  2021-01-21 17:44   ` Linus Torvalds
  5 siblings, 1 reply; 19+ messages in thread
From: Jiri Slaby @ 2021-01-21  9:39 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-serial
  Cc: hch, viro, linux-kernel, ohw.giles, r.karszniewicz, Linus Torvalds

On 21. 01. 21, 10:00, Greg Kroah-Hartman wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> This makes the tty layer use the .write_iter() function instead of the
> traditional .write() functionality.
> 
> That allows writev(), but more importantly also makes it possible to
> enable .splice_write() for ttys, reinstating the "splice to tty"
> functionality that was lost in commit 36e2c7421f02 ("fs: don't allow
> splice read/write without explicit ops").
> 
> Fixes: 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
> Reported-by: Oliver Giles <ohw.giles@gmail.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>   drivers/tty/tty_io.c | 48 ++++++++++++++++++++++++--------------------
>   1 file changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 56ade99ef99f..338bc4ef5549 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -143,9 +143,8 @@ LIST_HEAD(tty_drivers);			/* linked list of tty drivers */
>   DEFINE_MUTEX(tty_mutex);
>   
>   static ssize_t tty_read(struct file *, char __user *, size_t, loff_t *);
> -static ssize_t tty_write(struct file *, const char __user *, size_t, loff_t *);
> -ssize_t redirected_tty_write(struct file *, const char __user *,
> -							size_t, loff_t *);
> +static ssize_t tty_write(struct kiocb *, struct iov_iter *);
> +ssize_t redirected_tty_write(struct kiocb *, struct iov_iter *);
>   static __poll_t tty_poll(struct file *, poll_table *);
>   static int tty_open(struct inode *, struct file *);
>   long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
> @@ -478,7 +477,8 @@ static void tty_show_fdinfo(struct seq_file *m, struct file *file)
>   static const struct file_operations tty_fops = {
>   	.llseek		= no_llseek,
>   	.read		= tty_read,
> -	.write		= tty_write,
> +	.write_iter	= tty_write,
> +	.splice_write	= iter_file_splice_write,
>   	.poll		= tty_poll,
>   	.unlocked_ioctl	= tty_ioctl,
>   	.compat_ioctl	= tty_compat_ioctl,
> @@ -491,7 +491,8 @@ static const struct file_operations tty_fops = {
>   static const struct file_operations console_fops = {
>   	.llseek		= no_llseek,
>   	.read		= tty_read,
> -	.write		= redirected_tty_write,
> +	.write_iter	= redirected_tty_write,
> +	.splice_write	= iter_file_splice_write,
>   	.poll		= tty_poll,
>   	.unlocked_ioctl	= tty_ioctl,
>   	.compat_ioctl	= tty_compat_ioctl,
> @@ -607,9 +608,9 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
>   	/* This breaks for file handles being sent over AF_UNIX sockets ? */
>   	list_for_each_entry(priv, &tty->tty_files, list) {
>   		filp = priv->file;
> -		if (filp->f_op->write == redirected_tty_write)
> +		if (filp->f_op->write_iter == redirected_tty_write)
>   			cons_filp = filp;
> -		if (filp->f_op->write != tty_write)
> +		if (filp->f_op->write_iter != tty_write)

This now relies on implicit value of hung_up_tty_fops.write_iter (i.e. 
NULL), okay.

>   			continue;
>   		closecount++;
>   		__tty_fasync(-1, filp, 0);	/* can't block */
 > 		filp->f_op = &hung_up_tty_fops;

Isn't this racy with VFS layer in vfs_write:
         if (file->f_op->write)
                 ret = file->f_op->write(file, buf, count, pos);
         else if (file->f_op->write_iter)
                 ret = new_sync_write(file, buf, count, pos);

? hung_up_tty_fops do not set iter_write and tty_fops do not set write. 
When we switch from one to the other here, right after the 'if', but 
before the call, what happens? Likely nothing for the ->write case 
immediately as compilers cache the value, but for ->write_iter, I'm not 
sure. Anyway, this looks broken to me. (Read on.)

> @@ -956,14 +957,20 @@ static inline ssize_t do_tty_write(
>   		size_t size = count;
>   		if (size > chunk)
>   			size = chunk;
> +
>   		ret = -EFAULT;
> -		if (copy_from_user(tty->write_buf, buf, size))
> +		if (copy_from_iter(tty->write_buf, size, from) != size)
>   			break;
> +
>   		ret = write(tty, file, tty->write_buf, size);
>   		if (ret <= 0)
>   			break;
> +
> +		/* FIXME! Have Al check this! */
> +		if (ret != size)
> +			iov_iter_revert(from, size-ret);
> +
>   		written += ret;
> -		buf += ret;
>   		count -= ret;
>   		if (!count)
>   			break;
> @@ -1023,9 +1030,9 @@ void tty_write_message(struct tty_struct *tty, char *msg)
>    *	write method will not be invoked in parallel for each device.
>    */
>   
> -static ssize_t tty_write(struct file *file, const char __user *buf,
> -						size_t count, loff_t *ppos)
> +static ssize_t tty_write(struct kiocb *iocb, struct iov_iter *from)
>   {
> +	struct file *file = iocb->ki_filp;
>   	struct tty_struct *tty = file_tty(file);
>    	struct tty_ldisc *ld;
>   	ssize_t ret;
> @@ -1038,18 +1045,15 @@ static ssize_t tty_write(struct file *file, const char __user *buf,
>   	if (tty->ops->write_room == NULL)
>   		tty_err(tty, "missing write_room method\n");
>   	ld = tty_ldisc_ref_wait(tty);
> -	if (!ld)
> -		return hung_up_tty_write(file, buf, count, ppos);
> -	if (!ld->ops->write)
> +	if (!ld || !ld->ops->write)
>   		ret = -EIO;
>   	else
> -		ret = do_tty_write(ld->ops->write, tty, file, buf, count);
> +		ret = do_tty_write(ld->ops->write, tty, file, from);
>   	tty_ldisc_deref(ld);

Ok, here belongs my earlier note: "if ld == NULL => crash here." That is 
if hangup happens during the ldisc wait, the kernel will crash in 
tty_ldisc_deref.

Is there a reason not to convert hung_up_tty_fops too and leave the 
return hung_up_tty_write here intact? This would also solve the comments 
above.

>   	return ret;
>   }
>   
> -ssize_t redirected_tty_write(struct file *file, const char __user *buf,
> -						size_t count, loff_t *ppos)
> +ssize_t redirected_tty_write(struct kiocb *iocb, struct iov_iter *iter)
>   {
>   	struct file *p = NULL;
>   

thanks,
-- 
js

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/6] tty: implement read_iter
  2021-01-21  9:00 ` [PATCH 3/6] tty: implement read_iter Greg Kroah-Hartman
@ 2021-01-21 10:44   ` Jiri Slaby
  0 siblings, 0 replies; 19+ messages in thread
From: Jiri Slaby @ 2021-01-21 10:44 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-serial
  Cc: hch, viro, linux-kernel, ohw.giles, r.karszniewicz, Linus Torvalds

On 21. 01. 21, 10:00, Greg Kroah-Hartman wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> Now that the ldisc read() function takes kernel pointers, it's fairly
> straightforward to make the tty file operations use .read_iter() instead
> of .read().
> 
> That automatically gives us vread() and friends, and also makes it
> possible to do .splice_read() on ttys again.
> 
> Fixes: 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
> Reported-by: Oliver Giles <ohw.giles@gmail.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>   drivers/tty/tty_io.c | 36 ++++++++++++++++++------------------
>   1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index a34f8bcf875e..8846d3b99845 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
...
> @@ -907,10 +909,10 @@ static int iterate_tty_read(struct tty_ldisc *ld, struct tty_struct *tty, struct
>    *	read calls may be outstanding in parallel.
>    */
>   
> -static ssize_t tty_read(struct file *file, char __user *buf, size_t count,
> -			loff_t *ppos)
> +static ssize_t tty_read(struct kiocb *iocb, struct iov_iter *to)
>   {
>   	int i;
> +	struct file *file = iocb->ki_filp;
>   	struct inode *inode = file_inode(file);
>   	struct tty_struct *tty = file_tty(file);
>   	struct tty_ldisc *ld;
> @@ -923,11 +925,9 @@ static ssize_t tty_read(struct file *file, char __user *buf, size_t count,
>   	/* We want to wait for the line discipline to sort out in this
>   	   situation */
>   	ld = tty_ldisc_ref_wait(tty);
> -	if (!ld)
> -		return hung_up_tty_read(file, buf, count, ppos);
>   	i = -EIO;
> -	if (ld->ops->read)
> -		i = iterate_tty_read(ld, tty, file, buf, count);
> +	if (ld && ld->ops->read)
> +		i = iterate_tty_read(ld, tty, file, to);
>   	tty_ldisc_deref(ld);

Here we have the same problem as in tty_write.

And also the other one with hung_up_tty_read not converted.

thanks,
-- 
js

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/6] tty: convert tty_ldisc_ops 'read()' function to take a kernel pointer
  2021-01-21  9:00 ` [PATCH 2/6] tty: convert tty_ldisc_ops 'read()' function to take a kernel pointer Greg Kroah-Hartman
@ 2021-01-21 11:02   ` Jiri Slaby
  2021-01-21 17:46     ` Linus Torvalds
  0 siblings, 1 reply; 19+ messages in thread
From: Jiri Slaby @ 2021-01-21 11:02 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-serial
  Cc: hch, viro, linux-kernel, ohw.giles, r.karszniewicz, Linus Torvalds

On 21. 01. 21, 10:00, Greg Kroah-Hartman wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> 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 <hch@lst.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
...
> --- 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;

Or simply
size = min(count, sizeof(kernel_buf));

> +		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;

n_hdlc_tty_read will return EOVERFLOW when size is 0, so this EFAULT is 
never propagated, if I am looking correctly? n_tty seems to be fine 
(returns zero for zeroed size).

> +		}
> +	} while (cookie);
> +
> +	/* We always clear tty buffer in case they contained passwords */
> +	memzero_explicit(kernel_buf, sizeof(kernel_buf));
> +	return offset ? offset : retval;
> +}

thanks,
-- 
js

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/6] tty: implement write_iter
  2021-01-21  9:39 ` [PATCH 1/6] tty: implement write_iter Jiri Slaby
@ 2021-01-21 17:44   ` Linus Torvalds
  2021-01-21 17:57     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 19+ messages in thread
From: Linus Torvalds @ 2021-01-21 17:44 UTC (permalink / raw)
  To: Jiri Slaby
  Cc: Greg Kroah-Hartman, linux-serial, Christoph Hellwig, Al Viro,
	Linux Kernel Mailing List, Oliver Giles, Robert Karszniewicz

On Thu, Jan 21, 2021 at 1:40 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> Ok, here belongs my earlier note: "if ld == NULL => crash here." That is
> if hangup happens during the ldisc wait, the kernel will crash in
> tty_ldisc_deref.

Right you are, good catch.

> Is there a reason not to convert hung_up_tty_fops too and leave the
> return hung_up_tty_write here intact? This would also solve the comments
> above.

No, no reason. I started out just changing that one tty_write, then
noticed that I had to change the redirect case too, but never then got
to "yeah, I should have changed the hup case as well".

Greg, do you prefer a new series, or incremental patches?

               Linus

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/6] tty: convert tty_ldisc_ops 'read()' function to take a kernel pointer
  2021-01-21 11:02   ` Jiri Slaby
@ 2021-01-21 17:46     ` Linus Torvalds
  2021-01-21 17:57       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 19+ messages in thread
From: Linus Torvalds @ 2021-01-21 17:46 UTC (permalink / raw)
  To: Jiri Slaby
  Cc: Greg Kroah-Hartman, linux-serial, Christoph Hellwig, Al Viro,
	Linux Kernel Mailing List, Oliver Giles, Robert Karszniewicz

On Thu, Jan 21, 2021 at 3:02 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> n_hdlc_tty_read will return EOVERFLOW when size is 0, so this EFAULT is
> never propagated, if I am looking correctly? n_tty seems to be fine
> (returns zero for zeroed size).

I'll fix that up too.

Greg - same question - do you want an incremental patch, or just a new series?

              Linus

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/6] tty: implement write_iter
  2021-01-21 17:44   ` Linus Torvalds
@ 2021-01-21 17:57     ` Greg Kroah-Hartman
  2021-01-21 18:42       ` Linus Torvalds
  0 siblings, 1 reply; 19+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-21 17:57 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Jiri Slaby, linux-serial, Christoph Hellwig, Al Viro,
	Linux Kernel Mailing List, Oliver Giles, Robert Karszniewicz

On Thu, Jan 21, 2021 at 09:44:17AM -0800, Linus Torvalds wrote:
> On Thu, Jan 21, 2021 at 1:40 AM Jiri Slaby <jirislaby@kernel.org> wrote:
> >
> > Ok, here belongs my earlier note: "if ld == NULL => crash here." That is
> > if hangup happens during the ldisc wait, the kernel will crash in
> > tty_ldisc_deref.
> 
> Right you are, good catch.
> 
> > Is there a reason not to convert hung_up_tty_fops too and leave the
> > return hung_up_tty_write here intact? This would also solve the comments
> > above.
> 
> No, no reason. I started out just changing that one tty_write, then
> noticed that I had to change the redirect case too, but never then got
> to "yeah, I should have changed the hup case as well".
> 
> Greg, do you prefer a new series, or incremental patches?

Incremental patches please as these are already in my public branches
and I would have to revert them and add new ones but that's messy, so
fixes on top is fine.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/6] tty: convert tty_ldisc_ops 'read()' function to take a kernel pointer
  2021-01-21 17:46     ` Linus Torvalds
@ 2021-01-21 17:57       ` Greg Kroah-Hartman
  0 siblings, 0 replies; 19+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-21 17:57 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Jiri Slaby, linux-serial, Christoph Hellwig, Al Viro,
	Linux Kernel Mailing List, Oliver Giles, Robert Karszniewicz

On Thu, Jan 21, 2021 at 09:46:58AM -0800, Linus Torvalds wrote:
> On Thu, Jan 21, 2021 at 3:02 AM Jiri Slaby <jirislaby@kernel.org> wrote:
> >
> > n_hdlc_tty_read will return EOVERFLOW when size is 0, so this EFAULT is
> > never propagated, if I am looking correctly? n_tty seems to be fine
> > (returns zero for zeroed size).
> 
> I'll fix that up too.
> 
> Greg - same question - do you want an incremental patch, or just a new series?

Incremental is best, thanks!

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/6] tty: implement write_iter
  2021-01-21 17:57     ` Greg Kroah-Hartman
@ 2021-01-21 18:42       ` Linus Torvalds
  2021-01-21 19:43         ` Greg Kroah-Hartman
  2021-01-22  7:33         ` Jiri Slaby
  0 siblings, 2 replies; 19+ messages in thread
From: Linus Torvalds @ 2021-01-21 18:42 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, linux-serial, Christoph Hellwig, Al Viro,
	Linux Kernel Mailing List, Oliver Giles, Robert Karszniewicz

[-- Attachment #1: Type: text/plain, Size: 1616 bytes --]

On Thu, Jan 21, 2021 at 9:57 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Incremental patches please as these are already in my public branches
> and I would have to revert them and add new ones but that's messy, so
> fixes on top is fine.

Ok. And since I think you put that first tty_write conversion patch in
a different branch from the tty_read one, I did the fixup patches for
the two as separate patches, even though they really just do the exact
same thing.

So here's three patches: the two fixups for the hung_up_tty case, and
the EOVERFLOW error case that Jiri also noted. I've also updated the
'tty-splice' branch if you prefer them that way.

And I *should* say that I still haven't tested _any_ of the HDLC
changes. I have no idea how to do that, and if somebody can point to a
test-case (or better yet, actually has a real life situation where
they use it and can test this all) it would be great.

Jiri, any other issues, or any comment of yours I missed? I didn't do
the min() thing, I find the explicit conditional more legible myself,
but won't complain if somebody else then disagrees and wants to clean
it up.

(On the matter of cleanups: when reading through the ICANON handling
in canon_copy_from_read_buf(), that code is really completely
incomprehensible. I know how it works, and why it does it, but I had
to remind myself, because the code just looks crazy and does things
like "*nr+1" to walk _past_ the point we actually copy etc. I was very
tempted to rewrite that entirely, but wanting to keep my changes
minimal and targeted made me not do so).

                Linus

[-- Attachment #2: 0001-tty-fix-up-hung_up_tty_write-conversion.patch --]
[-- Type: text/x-patch, Size: 1940 bytes --]

From bf6ee858fdff2a1800fd198bbe90034dcd60f3ef Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 21 Jan 2021 10:04:27 -0800
Subject: [PATCH 1/3] tty: fix up hung_up_tty_write() conversion

In commit "tty: implement write_iter", I left the write_iter conversion
of the hung up tty case alone, because I incorrectly thought it didn't
matter.

Jiri showed me the errors of my ways, and pointed out the problems with
that incomplete conversion.  Fix it all up.

Reported-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 drivers/tty/tty_io.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 8846d3b99845..52489f8b7401 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -437,8 +437,7 @@ static ssize_t hung_up_tty_read(struct file *file, char __user *buf,
 	return 0;
 }
 
-static ssize_t hung_up_tty_write(struct file *file, const char __user *buf,
-				 size_t count, loff_t *ppos)
+static ssize_t hung_up_tty_write(struct kiocb *iocb, struct iov_iter *from)
 {
 	return -EIO;
 }
@@ -506,7 +505,7 @@ static const struct file_operations console_fops = {
 static const struct file_operations hung_up_tty_fops = {
 	.llseek		= no_llseek,
 	.read		= hung_up_tty_read,
-	.write		= hung_up_tty_write,
+	.write_iter	= hung_up_tty_write,
 	.poll		= hung_up_tty_poll,
 	.unlocked_ioctl	= hung_up_tty_ioctl,
 	.compat_ioctl	= hung_up_tty_compat_ioctl,
@@ -1103,7 +1102,9 @@ static ssize_t tty_write(struct kiocb *iocb, struct iov_iter *from)
 	if (tty->ops->write_room == NULL)
 		tty_err(tty, "missing write_room method\n");
 	ld = tty_ldisc_ref_wait(tty);
-	if (!ld || !ld->ops->write)
+	if (!ld)
+		return hung_up_tty_write(iocb, from);
+	if (!ld->ops->write)
 		ret = -EIO;
 	else
 		ret = do_tty_write(ld->ops->write, tty, file, from);
-- 
2.29.2.157.g1d47791a39


[-- Attachment #3: 0002-tty-fix-up-hung_up_tty_read-conversion.patch --]
[-- Type: text/x-patch, Size: 1908 bytes --]

From 1443b92a1ff3a0af5f0e5a177db2d843273a2ca1 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 21 Jan 2021 10:08:15 -0800
Subject: [PATCH 2/3] tty: fix up hung_up_tty_read() conversion

In commit "tty: implement read_iter", I left the read_iter conversion of
the hung up tty case alone, because I incorrectly thought it didn't
matter.

Jiri showed me the errors of my ways, and pointed out the problems with
that incomplete conversion.  Fix it all up.

Reported-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 drivers/tty/tty_io.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 52489f8b7401..d7883da7ba3d 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -431,8 +431,7 @@ struct tty_driver *tty_find_polling_driver(char *name, int *line)
 EXPORT_SYMBOL_GPL(tty_find_polling_driver);
 #endif
 
-static ssize_t hung_up_tty_read(struct file *file, char __user *buf,
-				size_t count, loff_t *ppos)
+static ssize_t hung_up_tty_read(struct kiocb *iocb, struct iov_iter *to)
 {
 	return 0;
 }
@@ -504,7 +503,7 @@ static const struct file_operations console_fops = {
 
 static const struct file_operations hung_up_tty_fops = {
 	.llseek		= no_llseek,
-	.read		= hung_up_tty_read,
+	.read_iter	= hung_up_tty_read,
 	.write_iter	= hung_up_tty_write,
 	.poll		= hung_up_tty_poll,
 	.unlocked_ioctl	= hung_up_tty_ioctl,
@@ -924,8 +923,10 @@ static ssize_t tty_read(struct kiocb *iocb, struct iov_iter *to)
 	/* We want to wait for the line discipline to sort out in this
 	   situation */
 	ld = tty_ldisc_ref_wait(tty);
+	if (!ld)
+		return hung_up_tty_read(iocb, to);
 	i = -EIO;
-	if (ld && ld->ops->read)
+	if (ld->ops->read)
 		i = iterate_tty_read(ld, tty, file, to);
 	tty_ldisc_deref(ld);
 
-- 
2.29.2.157.g1d47791a39


[-- Attachment #4: 0003-tty-fix-up-iterate_tty_read-EOVERFLOW-handling.patch --]
[-- Type: text/x-patch, Size: 2129 bytes --]

From 2b3da8cf7ecafe48704f62046fce5da5d17b9e6a Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 21 Jan 2021 10:17:25 -0800
Subject: [PATCH 3/3] tty: fix up iterate_tty_read() EOVERFLOW handling

When I converted the tty_ldisc_ops 'read()' function to take a kernel
pointer, I was a bit too aggressive about the ldisc returning EOVERFLOW.

Yes, we want to have EOVERFLOW override any partially read data (because
the whole point is that the buffer was too small for the whole packet,
and we don't want to see partial packets), but it shouldn't override a
previous EFAULT.

And in fact, it really is just EOVERFLOW that is special and should
throw away any partially read data, not "any error".  Admittedly
EOVERFLOW is currently the only one that can happen for a continuation
read - and if the first read iteration returns an error we won't have this issue.

So this is more of a technicality, but let's just make the intent very
explicit, and re-organize the error handling a bit so that this is all
clearer.

Reported-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 drivers/tty/tty_io.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index d7883da7ba3d..88b4c4963461 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -861,13 +861,20 @@ static int iterate_tty_read(struct tty_ldisc *ld, struct tty_struct *tty,
 		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;
+			/* Did we have an earlier error (ie -EFAULT)? */
+			if (retval)
+				break;
+			retval = size;
+
+			/*
+			 * -EOVERFLOW means we didn't have enough space
+			 * for a whole packet, and we shouldn't return
+			 * a partial result.
+			 */
+			if (retval == -EOVERFLOW)
+				offset = 0;
+			break;
 		}
 
 		copied = copy_to_iter(kernel_buf, size, to);
-- 
2.29.2.157.g1d47791a39


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/6] tty: implement write_iter
  2021-01-21 18:42       ` Linus Torvalds
@ 2021-01-21 19:43         ` Greg Kroah-Hartman
  2021-01-21 21:09           ` Linus Torvalds
  2021-01-22  7:33         ` Jiri Slaby
  1 sibling, 1 reply; 19+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-21 19:43 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Jiri Slaby, linux-serial, Christoph Hellwig, Al Viro,
	Linux Kernel Mailing List, Oliver Giles, Robert Karszniewicz

On Thu, Jan 21, 2021 at 10:42:04AM -0800, Linus Torvalds wrote:
> On Thu, Jan 21, 2021 at 9:57 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > Incremental patches please as these are already in my public branches
> > and I would have to revert them and add new ones but that's messy, so
> > fixes on top is fine.
> 
> Ok. And since I think you put that first tty_write conversion patch in
> a different branch from the tty_read one, I did the fixup patches for
> the two as separate patches, even though they really just do the exact
> same thing.
> 
> So here's three patches: the two fixups for the hung_up_tty case, and
> the EOVERFLOW error case that Jiri also noted. I've also updated the
> 'tty-splice' branch if you prefer them that way.

This works, thanks for these.  I'll wait for Jiri to review them before
applying them to my branches...

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/6] tty: implement write_iter
  2021-01-21 19:43         ` Greg Kroah-Hartman
@ 2021-01-21 21:09           ` Linus Torvalds
  2021-01-22  7:07             ` Jiri Slaby
  0 siblings, 1 reply; 19+ messages in thread
From: Linus Torvalds @ 2021-01-21 21:09 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Jiri Slaby, linux-serial, Christoph Hellwig, Al Viro,
	Linux Kernel Mailing List, Oliver Giles, Robert Karszniewicz

On Thu, Jan 21, 2021 at 11:43 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> This works, thanks for these.  I'll wait for Jiri to review them before
> applying them to my branches...

Let's hope Jiri sees them, since he had some email issue earlier..

I'll add his suse address here too.

           Linus

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/6] tty: implement write_iter
  2021-01-21 21:09           ` Linus Torvalds
@ 2021-01-22  7:07             ` Jiri Slaby
  0 siblings, 0 replies; 19+ messages in thread
From: Jiri Slaby @ 2021-01-22  7:07 UTC (permalink / raw)
  To: Linus Torvalds, Greg Kroah-Hartman
  Cc: Jiri Slaby, linux-serial, Christoph Hellwig, Al Viro,
	Linux Kernel Mailing List, Oliver Giles, Robert Karszniewicz

On 21. 01. 21, 22:09, Linus Torvalds wrote:
> On Thu, Jan 21, 2021 at 11:43 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>>
>> This works, thanks for these.  I'll wait for Jiri to review them before
>> applying them to my branches...
> 
> Let's hope Jiri sees them, since he had some email issue earlier..
> 
> I'll add his suse address here too.

Thanks, I am fixed and nothing was lost :).

-- 
js
suse labs

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/6] tty: implement write_iter
  2021-01-21 18:42       ` Linus Torvalds
  2021-01-21 19:43         ` Greg Kroah-Hartman
@ 2021-01-22  7:33         ` Jiri Slaby
  2021-01-22  7:43           ` Greg Kroah-Hartman
  1 sibling, 1 reply; 19+ messages in thread
From: Jiri Slaby @ 2021-01-22  7:33 UTC (permalink / raw)
  To: Linus Torvalds, Greg Kroah-Hartman
  Cc: linux-serial, Christoph Hellwig, Al Viro,
	Linux Kernel Mailing List, Oliver Giles, Robert Karszniewicz

On 21. 01. 21, 19:42, Linus Torvalds wrote:
> On Thu, Jan 21, 2021 at 9:57 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>>
>> Incremental patches please as these are already in my public branches
>> and I would have to revert them and add new ones but that's messy, so
>> fixes on top is fine.
> 
> Ok. And since I think you put that first tty_write conversion patch in
> a different branch from the tty_read one, I did the fixup patches for
> the two as separate patches, even though they really just do the exact
> same thing.
> 
> So here's three patches: the two fixups for the hung_up_tty case, and
> the EOVERFLOW error case that Jiri also noted. I've also updated the
> 'tty-splice' branch if you prefer them that way.
> 
> And I *should* say that I still haven't tested _any_ of the HDLC
> changes. I have no idea how to do that, and if somebody can point to a
> test-case (or better yet, actually has a real life situation where
> they use it and can test this all) it would be great.
> 
> Jiri, any other issues, or any comment of yours I missed? I didn't do
> the min() thing, I find the explicit conditional more legible myself,
> but won't complain if somebody else then disagrees and wants to clean
> it up.

I cannot find anything else.

All three:
Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

thanks,
-- 
js

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/6] tty: implement write_iter
  2021-01-22  7:33         ` Jiri Slaby
@ 2021-01-22  7:43           ` Greg Kroah-Hartman
  0 siblings, 0 replies; 19+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-22  7:43 UTC (permalink / raw)
  To: Jiri Slaby
  Cc: Linus Torvalds, linux-serial, Christoph Hellwig, Al Viro,
	Linux Kernel Mailing List, Oliver Giles, Robert Karszniewicz

On Fri, Jan 22, 2021 at 08:33:33AM +0100, Jiri Slaby wrote:
> On 21. 01. 21, 19:42, Linus Torvalds wrote:
> > On Thu, Jan 21, 2021 at 9:57 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > 
> > > Incremental patches please as these are already in my public branches
> > > and I would have to revert them and add new ones but that's messy, so
> > > fixes on top is fine.
> > 
> > Ok. And since I think you put that first tty_write conversion patch in
> > a different branch from the tty_read one, I did the fixup patches for
> > the two as separate patches, even though they really just do the exact
> > same thing.
> > 
> > So here's three patches: the two fixups for the hung_up_tty case, and
> > the EOVERFLOW error case that Jiri also noted. I've also updated the
> > 'tty-splice' branch if you prefer them that way.
> > 
> > And I *should* say that I still haven't tested _any_ of the HDLC
> > changes. I have no idea how to do that, and if somebody can point to a
> > test-case (or better yet, actually has a real life situation where
> > they use it and can test this all) it would be great.
> > 
> > Jiri, any other issues, or any comment of yours I missed? I didn't do
> > the min() thing, I find the explicit conditional more legible myself,
> > but won't complain if somebody else then disagrees and wants to clean
> > it up.
> 
> I cannot find anything else.
> 
> All three:
> Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

Thanks for the review, I'll go apply these in a bit...

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-01-22  7:44 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-21  9:00 [PATCH 1/6] tty: implement write_iter Greg Kroah-Hartman
2021-01-21  9:00 ` [PATCH 2/6] tty: convert tty_ldisc_ops 'read()' function to take a kernel pointer Greg Kroah-Hartman
2021-01-21 11:02   ` Jiri Slaby
2021-01-21 17:46     ` Linus Torvalds
2021-01-21 17:57       ` Greg Kroah-Hartman
2021-01-21  9:00 ` [PATCH 3/6] tty: implement read_iter Greg Kroah-Hartman
2021-01-21 10:44   ` Jiri Slaby
2021-01-21  9:00 ` [PATCH 4/6] tty: clean up legacy leftovers from n_tty line discipline Greg Kroah-Hartman
2021-01-21  9:00 ` [PATCH 5/6] tty: teach n_tty line discipline about the new "cookie continuations" Greg Kroah-Hartman
2021-01-21  9:00 ` [PATCH 6/6] tty: teach the n_tty ICANON case about the new "cookie continuations" too Greg Kroah-Hartman
2021-01-21  9:39 ` [PATCH 1/6] tty: implement write_iter Jiri Slaby
2021-01-21 17:44   ` Linus Torvalds
2021-01-21 17:57     ` Greg Kroah-Hartman
2021-01-21 18:42       ` Linus Torvalds
2021-01-21 19:43         ` Greg Kroah-Hartman
2021-01-21 21:09           ` Linus Torvalds
2021-01-22  7:07             ` Jiri Slaby
2021-01-22  7:33         ` Jiri Slaby
2021-01-22  7:43           ` Greg Kroah-Hartman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.