Message-ID: <b221d2cf-7dc0-4624-a040-85c131ed72a1@I-love.SAKURA.ne.jp>
Date: Sun, 12 May 2024 22:45:46 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: LKML <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
        Marco Elver <elver@...gle.com>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: [fs] Are you OK with updating "struct file"->f_op value dynamically?

Hello.

This is a broadcast for making sure that nobody (especially filesystems
including out-of-tree proprietary modules) gets trouble with this change.



syzbot is reporting data race between __tty_hangup() and __fput() [1], and
Dmitry Vyukov mentioned that this race has possibility of NULL pointer
dereference, for tty_fops implements e.g. splice_read callback whereas
hung_up_tty_fops does not.

  CPU0                                  CPU1
  ----                                  ----
  do_splice_read() {
                                        __tty_hangup() {
    // f_op->splice_read was copy_splice_read
    if (unlikely(!in->f_op->splice_read))
      return warn_unsupported(in, "read");
                                          filp->f_op = &hung_up_tty_fops;
    // f_op->splice_read is now NULL
    return in->f_op->splice_read(in, ppos, pipe, len, flags);
                                        }
  }

Therefore, I was proposing a patch that avoids updating
"struct file"->f_op after "struct file" became visible to others.

 drivers/tty/tty_io.c | 46 +++++++++++++++++++++-----------------------
 include/linux/tty.h  |  1 +
 2 files changed, 23 insertions(+), 24 deletions(-)

 (patch body is shown bottom of this post)

During the discussion, Linus Torvalds commented that we don't want to add
data_race() annotation for reading "struct file"->f_op value [2], and
Marco Elver proposed __data_racy qualifier [3] so that we don't need to
scatter data_race() annotation to "struct file"->f_op readers/updaters.

And now, Linus is expecting a patch that updates "struct file"->f_op
value dynamically [4].

 drivers/tty/tty_io.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/fs.h   |  2 +-
 2 files changed, 35 insertions(+), 1 deletion(-)

 (patch body is shown bottom of this post)

But I want a confirmation before going that way. If someone is assuming that
"struct file"->f_op does not change as long as "struct file" is visible to
others, going that way can break that someone's code. Therefore, if you have
an assumption that "struct file"->f_op does not change, please be sure to
respond.


Link: https://syzkaller.appspot.com/bug?extid=b7c3ba8cdc2f6cf83c21 [1]
Link: https://lkml.kernel.org/r/CAHk-=wi3iondeh_9V2g3Qz5oHTRjLsOpoy83hb58MVh=nRZe0A@mail.gmail.com [2]
Link: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=31f605a308e627f06e4e6ab77254473f1c90f0bf [3]
Link: https://lkml.kernel.org/r/CAHk-=wgSOa_g+bxjNi+HQpC=6sHK2yKeoW-xOhb0-FVGMTDWjg@mail.gmail.com [4]



A patch that avoids updating "struct file"->f_op after "struct file"
became visible to others:

------------------------------------------------------------
 drivers/tty/tty_io.c | 46 +++++++++++++++++++++-----------------------
 include/linux/tty.h  |  1 +
 2 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 407b0d87b7c10..aeea5eb13f48c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -187,6 +187,7 @@ int tty_alloc_file(struct file *file)
 	if (!priv)
 		return -ENOMEM;
 
+	priv->hung = false;
 	file->private_data = priv;
 
 	return 0;
@@ -420,35 +421,35 @@ 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 kiocb *iocb, struct iov_iter *to)
+static inline ssize_t hung_up_tty_read(struct kiocb *iocb, struct iov_iter *to)
 {
 	return 0;
 }
 
-static ssize_t hung_up_tty_write(struct kiocb *iocb, struct iov_iter *from)
+static inline ssize_t hung_up_tty_write(struct kiocb *iocb, struct iov_iter *from)
 {
 	return -EIO;
 }
 
 /* No kernel lock held - none needed ;) */
-static __poll_t hung_up_tty_poll(struct file *filp, poll_table *wait)
+static inline __poll_t hung_up_tty_poll(struct file *filp, poll_table *wait)
 {
 	return EPOLLIN | EPOLLOUT | EPOLLERR | EPOLLHUP | EPOLLRDNORM | EPOLLWRNORM;
 }
 
-static long hung_up_tty_ioctl(struct file *file, unsigned int cmd,
+static inline long hung_up_tty_ioctl(struct file *file, unsigned int cmd,
 		unsigned long arg)
 {
 	return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
 }
 
-static long hung_up_tty_compat_ioctl(struct file *file,
+static inline long hung_up_tty_compat_ioctl(struct file *file,
 				     unsigned int cmd, unsigned long arg)
 {
 	return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
 }
 
-static int hung_up_tty_fasync(int fd, struct file *file, int on)
+static inline int hung_up_tty_fasync(int fd, struct file *file, int on)
 {
 	return -ENOTTY;
 }
@@ -490,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;
 
@@ -618,7 +608,7 @@ 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;
+		priv->hung = true;
 	}
 	spin_unlock(&tty->files_lock);
 
@@ -742,7 +732,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);
+	return filp && filp->f_op == &tty_fops &&
+		((struct tty_file_private *) filp->private_data)->hung;
 }
 EXPORT_SYMBOL(tty_hung_up_p);
 
@@ -921,6 +912,8 @@ static ssize_t tty_read(struct kiocb *iocb, struct iov_iter *to)
 	struct tty_ldisc *ld;
 	ssize_t ret;
 
+	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))
@@ -1080,6 +1073,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))
@@ -2166,11 +2161,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);
@@ -2204,6 +2194,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;
 
@@ -2256,6 +2248,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);
@@ -2684,6 +2678,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;
 
@@ -2969,6 +2965,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;
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 2372f9357240d..56c250247df9b 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -248,6 +248,7 @@ struct tty_file_private {
 	struct tty_struct *tty;
 	struct file *file;
 	struct list_head list;
+	bool __data_racy hung; /* Whether __tty_hangup() was called or not. */
 };
 
 /**
------------------------------------------------------------

A patch that updates "struct file"->f_op value dynamically:

------------------------------------------------------------
 drivers/tty/tty_io.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/fs.h   |  2 +-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 407b0d87b7c10..16e135687226a 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -430,6 +430,24 @@ static ssize_t hung_up_tty_write(struct kiocb *iocb, struct iov_iter *from)
 	return -EIO;
 }
 
+static ssize_t hung_up_copy_splice_read(struct file *in, loff_t *ppos,
+					struct pipe_inode_info *pipe,
+					size_t len, unsigned int flags)
+{
+	return -EINVAL;
+}
+
+static ssize_t hung_up_iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out,
+					      loff_t *ppos, size_t len, unsigned int flags)
+{
+	return -EINVAL;
+}
+
+static int hung_up_no_open(struct inode *inode, struct file *file)
+{
+	return -ENXIO;
+}
+
 /* No kernel lock held - none needed ;) */
 static __poll_t hung_up_tty_poll(struct file *filp, poll_table *wait)
 {
@@ -462,6 +480,12 @@ static void tty_show_fdinfo(struct seq_file *m, struct file *file)
 }
 
 static const struct file_operations tty_fops = {
+	/*
+	 * WARNING: You must implement all callbacks defined in tty_fops in
+	 * hung_up_tty_fops, for tty_fops and hung_up_tty_fops are toggled
+	 * after "struct file" is published. Failure to synchronize has a risk
+	 * of NULL pointer dereference bug.
+	 */
 	.llseek		= no_llseek,
 	.read_iter	= tty_read,
 	.write_iter	= tty_write,
@@ -491,14 +515,24 @@ static const struct file_operations console_fops = {
 };
 
 static const struct file_operations hung_up_tty_fops = {
+	/*
+	 * WARNING: You must implement all callbacks defined in hung_up_tty_fops
+	 * in tty_fops, for tty_fops and hung_up_tty_fops are toggled after
+	 * "struct file" is published. Failure to synchronize has a risk of
+	 * NULL pointer dereference bug.
+	 */
 	.llseek		= no_llseek,
 	.read_iter	= hung_up_tty_read,
 	.write_iter	= hung_up_tty_write,
+	.splice_read    = hung_up_copy_splice_read,
+	.splice_write   = hung_up_iter_file_splice_write,
 	.poll		= hung_up_tty_poll,
 	.unlocked_ioctl	= hung_up_tty_ioctl,
 	.compat_ioctl	= hung_up_tty_compat_ioctl,
+	.open           = hung_up_no_open,
 	.release	= tty_release,
 	.fasync		= hung_up_tty_fasync,
+	.show_fdinfo    = tty_show_fdinfo,
 };
 
 static DEFINE_SPINLOCK(redirect_lock);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1394347b4fda5..2d14b26ace792 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1008,7 +1008,7 @@ struct file {
 	struct file_ra_state	f_ra;
 	struct path		f_path;
 	struct inode		*f_inode;	/* cached value */
-	const struct file_operations	*f_op;
+	const struct file_operations	*__data_racy f_op;
 
 	u64			f_version;
 #ifdef CONFIG_SECURITY
------------------------------------------------------------

Powered by blists - more mailing lists