| Message ID | 20210121090020.3147058-1-gregkh@linuxfoundation.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] tty: implement write_iter | expand |
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,
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,
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
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
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 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);
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
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
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
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
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;