Re: [PATCH 1/6] tty: implement write_iter

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- 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