Re: [syzbot] possible deadlock in pipe_lock (5)

[Miklos Szeredi]

On Sun, Jul 25, 2021 at 3:31 AM Hillf Danton <hdanton@xxxxxxxx> wrote:
>
> On Sat, 24 Jul 2021 12:07:20 -0700
> >syzbot found the following issue on:
> >
> >HEAD commit:    8cae8cd89f05 seq_file: disallow extremely large seq buffer..
> >git tree:       upstream
> >console output: https://syzkaller.appspot.com/x/log.txt?x=1083e8cc300000
> >kernel config:  https://syzkaller.appspot.com/x/.config?x=7273c75708b55890
> >dashboard link: https://syzkaller.appspot.com/bug?extid=579885d1a9a833336209
> >syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=163905f2300000
> >C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=165bd0ea300000
> >
> >The issue was bisected to:
> >
> >commit 82a763e61e2b601309d696d4fa514c77d64ee1be
> >Author: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> >Date:   Mon Dec 14 14:26:14 2020 +0000
> >
> >    ovl: simplify file splice
>
>
> If this commit is innocent then is it false positive lockdep warning again,
> given another report [1]?

Appears to be legit.

Attached partial revert + sync with ovl_write_iter() should fix it
(fingers crossed).

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
master

Thanks,
Miklos
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 4d53d3b7e5fe..d081faa55e83 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -392,6 +392,51 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 	return ret;
 }
 
+/*
+ * Calling iter_file_splice_write() directly from overlay's f_op may deadlock
+ * due to lock order inversion between pipe->mutex in iter_file_splice_write()
+ * and file_start_write(real.file) in ovl_write_iter().
+ *
+ * So do everything ovl_write_iter() does and call iter_file_splice_write() on
+ * the real file.
+ */
+static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
+				loff_t *ppos, size_t len, unsigned int flags)
+{
+	struct fd real;
+	const struct cred *old_cred;
+	struct inode *inode = file_inode(out);
+	struct inode *realinode = ovl_inode_real(inode);
+	ssize_t ret;
+
+	inode_lock(inode);
+	/* Update mode */
+	ovl_copyattr(realinode, inode);
+	ret = file_remove_privs(out);
+	if (ret)
+		goto out_unlock;
+
+	ret = ovl_real_fdget(out, &real);
+	if (ret)
+		goto out_unlock;
+
+	old_cred = ovl_override_creds(inode->i_sb);
+	file_start_write(real.file);
+
+	ret = iter_file_splice_write(pipe, real.file, ppos, len, flags);
+
+	file_end_write(real.file);
+	/* Update size */
+	ovl_copyattr(realinode, inode);
+	revert_creds(old_cred);
+	fdput(real);
+
+out_unlock:
+	inode_unlock(inode);
+
+	return ret;
+}
+
 static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 {
 	struct fd real;
@@ -603,7 +648,7 @@ const struct file_operations ovl_file_operations = {
 	.fadvise	= ovl_fadvise,
 	.flush		= ovl_flush,
 	.splice_read    = generic_file_splice_read,
-	.splice_write   = iter_file_splice_write,
+	.splice_write   = ovl_splice_write,
 
 	.copy_file_range	= ovl_copy_file_range,
 	.remap_file_range	= ovl_remap_file_range,