From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
To: Al Viro <viro@zeniv.linux.org.uk>,
Jiri Slaby <jirislaby@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Dmitry Vyukov <dvyukov@google.com>,
syzbot <syzbot+b7c3ba8cdc2f6cf83c21@syzkaller.appspotmail.com>,
linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com
Subject: Re: [syzbot] [kernel?] KCSAN: data-race in __fput / __tty_hangup (4)
Date: Tue, 25 Apr 2023 23:47:20 +0900 [thread overview]
Message-ID: <2fca7932-5030-32c3-dd61-48dd78e58e11@I-love.SAKURA.ne.jp> (raw)
In-Reply-To: <8e21256a-736e-4c2d-1ff4-723775bcac46@I-love.SAKURA.ne.jp>
tty_open() is doing "filp->f_op = &tty_fops;".
__tty_hangup() is doing "filp->f_op = &hung_up_tty_fops;".
tty_hung_up_p() is doing "filp->f_op == &hung_up_tty_fops",
and most functions are checking tty_hung_up_p().
Therefore, if we can remember whether __tty_hangup() did
"filp->f_op = &hung_up_tty_fops;" without changing filp->f_op,
we can reuse tty_hung_up_p().
Since tty_open() allocates sizeof("struct tty_file_private") bytes
of memory associated with each "struct file", we can record whether
__tty_hangup() did "filp->f_op = &hung_up_tty_fops;" using this memory.
Therefore, diff will be something like this?
(Explanation and question follows this diff.)
drivers/tty/tty_io.c | 36 +++++++++++++++++-------------------
include/linux/tty.h | 1 +
2 files changed, 18 insertions(+), 19 deletions(-)
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 093935e97f42..d7fa18f8c526 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -255,6 +255,7 @@ struct tty_file_private {
struct tty_struct *tty;
struct file *file;
struct list_head list;
+ bool hung;
};
/**
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 36fb945fdad4..bebd236679f0 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -182,7 +182,7 @@ int tty_alloc_file(struct file *file)
{
struct tty_file_private *priv;
- priv = kmalloc(sizeof(*priv), GFP_KERNEL);
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
@@ -491,17 +491,6 @@ static const struct file_operations console_fops = {
.fasync = tty_fasync,
};
-static const struct file_operations hung_up_tty_fops = {
- .llseek = no_llseek,
- .read_iter = hung_up_tty_read,
- .write_iter = hung_up_tty_write,
- .poll = hung_up_tty_poll,
- .unlocked_ioctl = hung_up_tty_ioctl,
- .compat_ioctl = hung_up_tty_compat_ioctl,
- .release = tty_release,
- .fasync = hung_up_tty_fasync,
-};
-
static DEFINE_SPINLOCK(redirect_lock);
static struct file *redirect;
@@ -619,7 +608,8 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
continue;
closecount++;
__tty_fasync(-1, filp, 0); /* can't block */
- filp->f_op = &hung_up_tty_fops;
+ /* Accept race with tty_hung_up_p() test. */
+ data_race(priv->hung = true);
}
spin_unlock(&tty->files_lock);
@@ -743,7 +733,8 @@ void tty_vhangup_session(struct tty_struct *tty)
*/
int tty_hung_up_p(struct file *filp)
{
- return (filp && filp->f_op == &hung_up_tty_fops);
+ /* Accept race with __tty_hangup(). */
+ return filp && data_race(((struct tty_file_private *) filp->private_data)->hung);
}
EXPORT_SYMBOL(tty_hung_up_p);
@@ -911,6 +902,8 @@ static ssize_t tty_read(struct kiocb *iocb, struct iov_iter *to)
struct tty_struct *tty = file_tty(file);
struct tty_ldisc *ld;
+ if (tty_hung_up_p(file))
+ return hung_up_tty_read(iocb, to);
if (tty_paranoia_check(tty, inode, "tty_read"))
return -EIO;
if (!tty || tty_io_error(tty))
@@ -1073,6 +1066,8 @@ static ssize_t file_tty_write(struct file *file, struct kiocb *iocb, struct iov_
struct tty_ldisc *ld;
ssize_t ret;
+ if (tty_hung_up_p(file))
+ return hung_up_tty_write(iocb, from);
if (tty_paranoia_check(tty, file_inode(file), "tty_write"))
return -EIO;
if (!tty || !tty->ops->write || tty_io_error(tty))
@@ -2159,11 +2154,6 @@ static int tty_open(struct inode *inode, struct file *filp)
return retval;
schedule();
- /*
- * Need to reset f_op in case a hangup happened.
- */
- if (tty_hung_up_p(filp))
- filp->f_op = &tty_fops;
goto retry_open;
}
clear_bit(TTY_HUPPED, &tty->flags);
@@ -2197,6 +2187,8 @@ static __poll_t tty_poll(struct file *filp, poll_table *wait)
struct tty_ldisc *ld;
__poll_t ret = 0;
+ if (tty_hung_up_p(filp))
+ return hung_up_tty_poll(filp, wait);
if (tty_paranoia_check(tty, file_inode(filp), "tty_poll"))
return 0;
@@ -2249,6 +2241,8 @@ static int tty_fasync(int fd, struct file *filp, int on)
struct tty_struct *tty = file_tty(filp);
int retval = -ENOTTY;
+ if (tty_hung_up_p(filp))
+ return hung_up_tty_fasync(fd, filp, on);
tty_lock(tty);
if (!tty_hung_up_p(filp))
retval = __tty_fasync(fd, filp, on);
@@ -2658,6 +2652,8 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
int retval;
struct tty_ldisc *ld;
+ if (tty_hung_up_p(file))
+ return hung_up_tty_ioctl(file, cmd, arg);
if (tty_paranoia_check(tty, file_inode(file), "tty_ioctl"))
return -EINVAL;
@@ -2943,6 +2939,8 @@ static long tty_compat_ioctl(struct file *file, unsigned int cmd,
return tty_ioctl(file, cmd, arg);
}
+ if (tty_hung_up_p(file))
+ return hung_up_tty_compat_ioctl(file, cmd, arg);
if (tty_paranoia_check(tty, file_inode(file), "tty_ioctl"))
return -EINVAL;
Since tty_read() is called via file->f_op->read_iter() from call_read_iter() from
generic_file_splice_read(), no change is needed for tty_fops->splice_read.
Since tty_write() is called via file->f_op->write_iter() from call_write_iter()
from do_iter_readv_writev() from do_iter_write() from vfs_iter_write() from
iter_file_splice_write(), no change is needed for tty_fops->splice_write.
Since both tty_fops and hung_up_tty_fops point to the same callback for
llseek/release, no change is needed for tty_fops->llseek and tty_fops->release.
Regarding tty_read()/tty_write()/tty_poll()/tty_ioctl()/tty_compat_ioctl(),
respectively embed hung_up_tty_read()/hung_up_tty_write()/hung_up_tty_poll()/
hung_up_tty_ioctl()/hung_up_tty_compat_ioctl() right before tty_paranoia_check().
Regarding tty_fasync(), embed hung_up_tty_fasync() right before tty_lock() in
tty_fasync() rather than tty_paranoia_check() in __tty_fasync(), for
tty_hung_up_p() is checked right after tty_lock() returned.
I don't know what changes are required for tty_open() and tty_show_fdinfo().
I assume that making no change for tty_show_fdinfo() is harmless.
But how does "hung_up_tty_fops does not call tty_open()" affects here?
next prev parent reply other threads:[~2023-04-25 14:47 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-21 8:18 [syzbot] [kernel?] KCSAN: data-race in __fput / __tty_hangup (4) syzbot
2023-04-21 8:21 ` Dmitry Vyukov
2023-04-21 15:12 ` Tetsuo Handa
2023-04-21 16:02 ` Tetsuo Handa
2023-04-23 23:34 ` Al Viro
2023-04-23 23:55 ` Tetsuo Handa
2023-04-24 0:44 ` Al Viro
2023-04-24 1:09 ` Tetsuo Handa
2023-04-25 14:47 ` Tetsuo Handa [this message]
2023-04-25 16:03 ` Al Viro
2023-04-25 22:09 ` Tetsuo Handa
2023-04-26 11:05 ` [PATCH] tty: tty_io: remove hung_up_tty_fops Tetsuo Handa
2023-04-28 16:27 ` Nathan Chancellor
2023-04-28 16:41 ` Tetsuo Handa
2023-04-28 17:11 ` Al Viro
2023-04-29 10:43 ` Tetsuo Handa
2023-04-28 17:31 ` Greg Kroah-Hartman
2023-04-29 15:21 ` Guenter Roeck
2023-05-01 18:42 ` Geert Uytterhoeven
2023-05-14 1:02 ` [PATCH v2] " Tetsuo Handa
2023-05-30 10:44 ` Greg Kroah-Hartman
2023-05-30 11:57 ` Tetsuo Handa
2023-05-30 12:51 ` Greg Kroah-Hartman
2024-04-27 6:20 ` [PATCH v3] " Tetsuo Handa
2024-04-27 19:02 ` Linus Torvalds
2024-04-28 10:19 ` Tetsuo Handa
2024-04-28 18:50 ` Linus Torvalds
2024-04-29 13:55 ` Marco Elver
2024-04-29 15:38 ` Linus Torvalds
2024-05-01 18:45 ` Paul E. McKenney
2024-05-01 18:56 ` Linus Torvalds
2024-05-01 19:02 ` Paul E. McKenney
2024-05-01 20:14 ` Marco Elver
2024-05-01 21:06 ` Linus Torvalds
2024-05-01 21:20 ` Linus Torvalds
2024-05-01 21:49 ` Paul E. McKenney
2024-05-01 22:32 ` Paul E. McKenney
2024-05-02 16:37 ` Boqun Feng
2024-05-03 23:59 ` Paul E. McKenney
2024-05-04 0:14 ` Linus Torvalds
2024-05-04 5:08 ` Paul E. McKenney
2024-05-04 17:50 ` Linus Torvalds
2024-05-04 18:18 ` Paul E. McKenney
2024-05-04 19:11 ` Linus Torvalds
2024-05-04 19:25 ` Linus Torvalds
2024-05-04 22:17 ` Paul E. McKenney
2024-05-04 22:04 ` Paul E. McKenney
2024-05-02 14:14 ` Marco Elver
2024-05-02 16:42 ` Tetsuo Handa
2024-05-02 17:20 ` Marco Elver
2024-05-02 17:29 ` Linus Torvalds
2024-05-02 18:14 ` Al Viro
2024-05-02 19:29 ` Marco Elver
2024-05-02 23:54 ` Tetsuo Handa
2024-05-03 1:12 ` Linus Torvalds
2023-04-23 13:28 ` [syzbot] [kernel?] KCSAN: data-race in __fput / __tty_hangup (4) Tetsuo Handa
2023-04-23 14:00 ` Greg Kroah-Hartman
2023-04-23 14:03 ` Greg Kroah-Hartman
2023-04-23 14:17 ` Tetsuo Handa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2fca7932-5030-32c3-dd61-48dd78e58e11@I-love.SAKURA.ne.jp \
--to=penguin-kernel@i-love.sakura.ne.jp \
--cc=dvyukov@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=jirislaby@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=syzbot+b7c3ba8cdc2f6cf83c21@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.