Re: [5.11 regression] "tty: implement write_iter" breaks TIOCCONS

Re: [5.11 regression] "tty: implement write_iter" breaks TIOCCONS

[Linus Torvalds]

On Fri, Jan 29, 2021 at 6:54 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> While testing 5.11-rc5 I noticed that flicker-free boot was no longer flicker free,
> when plymouth loads and tells systemd to start logging detailed messages these start
> showing upon the fbcon instead of being redirected to the ptmx which plymouth is
> trying to redirect the messages too.

I think this should already be fixed in current -git by commit 9f12e37cae44.

Can you check?

               Linus

Re: [5.11 regression] "tty: implement write_iter" breaks TIOCCONS

[Hans de Goede]

Hi,

On 1/29/21 7:09 PM, Linus Torvalds wrote:
> On Fri, Jan 29, 2021 at 6:54 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> While testing 5.11-rc5 I noticed that flicker-free boot was no longer flicker free,
>> when plymouth loads and tells systemd to start logging detailed messages these start
>> showing upon the fbcon instead of being redirected to the ptmx which plymouth is
>> trying to redirect the messages too.
> 
> I think this should already be fixed in current -git by commit 9f12e37cae44.
> 
> Can you check?

So I just did the following

1. Started with my local tree (5.11-rc5 + patches I'm working on, nothing tty related)
   ( https://github.com/jwrdegoede/linux-sunxi/commits/master )
2. Dropped the 2 reverts which I was carrying to workaround this locally
3. Cherry-picked 9f12e37cae44
4. Build + installed a new bzImage

Unfortunately this does not resolve the problem, after this the problem is back.

I can build a kernel from a clean torvalds/linux/master but I don't expect
any different results from that. Let me know if you still want me to do
a clean build to make sure this is not something in my local tree.

Regards,

Hans

Re: [5.11 regression] "tty: implement write_iter" breaks TIOCCONS

[Hans de Goede]

Hi Linus,

On 1/29/21 7:23 PM, Hans de Goede wrote:
> Hi,
> 
> On 1/29/21 7:09 PM, Linus Torvalds wrote:
>> On Fri, Jan 29, 2021 at 6:54 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> While testing 5.11-rc5 I noticed that flicker-free boot was no longer flicker free,
>>> when plymouth loads and tells systemd to start logging detailed messages these start
>>> showing upon the fbcon instead of being redirected to the ptmx which plymouth is
>>> trying to redirect the messages too.
>>
>> I think this should already be fixed in current -git by commit 9f12e37cae44.
>>
>> Can you check?
> 
> So I just did the following
> 
> 1. Started with my local tree (5.11-rc5 + patches I'm working on, nothing tty related)
>    ( https://github.com/jwrdegoede/linux-sunxi/commits/master )
> 2. Dropped the 2 reverts which I was carrying to workaround this locally
> 3. Cherry-picked 9f12e37cae44
> 4. Build + installed a new bzImage
> 
> Unfortunately this does not resolve the problem, after this the problem is back.
> 
> I can build a kernel from a clean torvalds/linux/master but I don't expect
> any different results from that. Let me know if you still want me to do
> a clean build to make sure this is not something in my local tree.

p.s.

You are using Fedora now a days, right ?  In that case you should be
able to reproduce this yourself (depending on how custom your kernel
setup is) if you are using the standard Fedora initrd generated by
dracut and have "rhgb" on your kernel cmdline, then you can check
for this problem by doing:

sudo cat /var/log/boot.log 

For me the end of that file looks like this:

------------ Wed Jan 27 08:28:15 CET 2021 ------------
------------ Wed Jan 27 17:27:53 CET 2021 ------------
------------ Thu Jan 28 11:11:50 CET 2021 ------------
------------ Fri Jan 29 10:32:04 CET 2021 ------------
------------ Fri Jan 29 19:16:40 CET 2021 ------------
------------ Fri Jan 29 19:19:05 CET 2021 ------------

There should be lines like this in between those separators
(each separator is the start of the logs of a new boot):

[  OK  ] Started User Login Management.
[  OK  ] Started GNOME Display Manager.
etc.

Regards,

Hans

Re: [5.11 regression] "tty: implement write_iter" breaks TIOCCONS

[Linus Torvalds]

On Fri, Jan 29, 2021 at 10:31 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> You are using Fedora now a days, right ?  In that case you should be
> able to reproduce this yourself (depending on how custom your kernel
> setup is) if you are using the standard Fedora initrd generated by
> dracut and have "rhgb" on your kernel cmdline, then you can check
> for this problem by doing:

Thanks, I can see it, that should make it much easier to figure out.

                 Linus

Re: [5.11 regression] "tty: implement write_iter" breaks TIOCCONS

[Linus Torvalds]

On Fri, Jan 29, 2021 at 11:17 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, Jan 29, 2021 at 10:31 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > You are using Fedora now a days, right ?  In that case you should be
> > able to reproduce this yourself (depending on how custom your kernel
> > setup is) if you are using the standard Fedora initrd generated by
> > dracut and have "rhgb" on your kernel cmdline, then you can check
> > for this problem by doing:
>
> Thanks, I can see it, that should make it much easier to figure out.

Ahh, interesting.

It turns out that the problem isn't actually really in the tty layer,
it's that vfs_iocb_iter_write() is very very subtly buggy.

So the tty layer "trivial" conversion from using "vfs_write()" - for
the old redirected tty_write() call - to using "vfs_iocb_iter_write()"
caused problems.

Why? Because both vfs_write() and vfs_iocb_iter_write() take the
target "struct file *file" as an argument, but vfs_iocb_iter_write()
doesn't actually *use* that target file!

Well, to be specific, it does actually use the target file pointer for
two things:

 - the security checks

 - to pick the actual ->write_iter function.

But once you actually call ->write_iter() to do the IO, the 'file'
pointer isn't actually passed down at all, and the write_iter()
function depends not on 'file', but on 'iocb->ki_filp".

In other words, vfs_iocb_iter_write() is completely broken, because it
will do the preliminary work using one 'struct file *', but then do
the actual IO using _another_ 'struct file *' entirely.

In the case of the console redirect code, that meant that the
"redirect" never actually redirected anything, it really just called
tty_write() with the original iocb, which used the original target
file pointer.

Let's just say that I stared at those tty changes for a while, saying
"there is no *POSSIBLE* way that introduces a bug". And yeah, the tty
changes themselves were actually not the real culprit.

Of course, there is only one other user of vfs_iocb_iter_write() -
ovlfs - and that one fills in the iocb with the same file pointer that
it uses as the first argument, so nobody has ever noticed this oddity
before.

The function has been buggy like this since the very first
implementation, and vfs_iocb_iter_read() has the exact same issue.

It's fairly easy to work around in this in the tty layer by just
avoiding that function entirely, so I'll cook up a patch to do that.
But I'm adding the appropriate people to the participants here because
this really is very subtle if you ever hit it.

It might be best to just remove the "struct file *file" argument from
vfs_iocb_iter_{read,write}(), because it really is very wrong to use
anything but iocb.ki_file, and it's really subtle.

                     Linus

Re: [5.11 regression] "tty: implement write_iter" breaks TIOCCONS

[Linus Torvalds]

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

On Fri, Jan 29, 2021 at 12:02 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> It's fairly easy to work around in this in the tty layer by just
> avoiding that function entirely, so I'll cook up a patch to do that.
> But I'm adding the appropriate people to the participants here because
> this really is very subtle if you ever hit it.

Here's the patch to make the tty layer just do the redirection
entirely internally, avoiding that mis-designed vfs_iocb_iter_write()
function.

Hans, does this fix things for you? I'm pretty confident it will, but
always best to double-check..

                 Linus

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 1809 bytes --]

 drivers/tty/tty_io.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index bf7be8e69745..d8a68f6a0fdd 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1092,9 +1092,8 @@ 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 kiocb *iocb, struct iov_iter *from)
+static ssize_t file_tty_write(struct file *file, 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;
@@ -1117,6 +1116,11 @@ static ssize_t tty_write(struct kiocb *iocb, struct iov_iter *from)
 	return ret;
 }
 
+static ssize_t tty_write(struct kiocb *iocb, struct iov_iter *from)
+{
+	return file_tty_write(iocb->ki_filp, iocb, from);
+}
+
 ssize_t redirected_tty_write(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct file *p = NULL;
@@ -1126,9 +1130,13 @@ ssize_t redirected_tty_write(struct kiocb *iocb, struct iov_iter *iter)
 		p = get_file(redirect);
 	spin_unlock(&redirect_lock);
 
+	/*
+	 * We know the redirected tty is just another tty, we can can
+	 * call file_tty_write() directly with that file pointer.
+	 */
 	if (p) {
 		ssize_t res;
-		res = vfs_iocb_iter_write(p, iocb, iter);
+		res = file_tty_write(p, iocb, iter);
 		fput(p);
 		return res;
 	}
@@ -2374,6 +2382,12 @@ static int tioccons(struct file *file)
 			fput(f);
 		return 0;
 	}
+	if (file->f_op->write_iter != tty_write)
+		return -ENOTTY;
+	if (!(file->f_mode & FMODE_WRITE))
+		return -EBADF;
+	if (!(file->f_mode & FMODE_CAN_WRITE))
+		return -EINVAL;
 	spin_lock(&redirect_lock);
 	if (redirect) {
 		spin_unlock(&redirect_lock);

Re: [5.11 regression] "tty: implement write_iter" breaks TIOCCONS

[Hans de Goede]

Hi,

On 1/29/21 9:28 PM, Linus Torvalds wrote:
> On Fri, Jan 29, 2021 at 12:02 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> It's fairly easy to work around in this in the tty layer by just
>> avoiding that function entirely, so I'll cook up a patch to do that.
>> But I'm adding the appropriate people to the participants here because
>> this really is very subtle if you ever hit it.
> 
> Here's the patch to make the tty layer just do the redirection
> entirely internally, avoiding that mis-designed vfs_iocb_iter_write()
> function.
> 
> Hans, does this fix things for you? I'm pretty confident it will, but
> always best to double-check..

I can confirm that the attached patch fixes things for me, thanks.

Regards,

Hans