[PATCH] jfs: fix statfs racing with a read-write remount

[PATCH] jfs: fix statfs racing with a read-write remount

[Wang Yibo]

jfs_remount() rebuilds the in-memory inode and block maps when a
read-only file system is remounted read-write, so that changes made by
fsck while the volume was read-only are picked up.  The rebuild frees
the old structures first and installs the new ones afterwards:

	diUnmount()  -> kfree(imap); JFS_IP(imap)->i_imap = NULL
	diMount()    -> ...;         JFS_IP(imap)->i_imap = imap
	dbUnmount()  -> kfree(bmp);  JFS_SBI(sb)->bmap = NULL
	dbMount()    -> ...;         JFS_SBI(sb)->bmap = bmp

jfs_statfs() reads both maps without taking any lock, so a statfs that
runs while the rebuild is in progress observes the NULL - or a pointer
to memory that has just been freed - and faults:

	BUG: KASAN: user-memory-access in jfs_statfs+0x20e/0x510
	Read of size 4 at addr 00000000000050c0 by task syz-executor415/5092
	...
	 jfs_statfs fs/jfs/super.c:140
	 __x64_sys_statfs

0x50c0 is offsetof(struct inomap, im_numinos) for the reporter's
configuration, i.e. i_imap was NULL when statfs() ran.

The inode map rebuild is already serialized by RDWRLOCK_IMAP, but that
only orders the inode map users among themselves; statfs() is not one of
them.  Take the same lock for reading in jfs_statfs() and keep the write
side held across the block map rebuild too, so that both maps are at all
times either the old or the new structures.  A failed rebuild can still
leave the maps torn down, so return -EIO instead of dereferencing NULL
in that case as well.

An earlier AI-generated RFC from syzbot proposed a different fix for
the same remount window; it was never reviewed and it does not cover the
report below.  It is linked only for context.

Reported-by: syzbot+cea4fad5485bc30243a9@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=cea4fad5485bc30243a9
Link: https://lore.kernel.org/all/80218035-7c1f-4784-93d1-c34d9ccc252a@mail.kernel.org/
Signed-off-by: Wang Yibo <szwyb123@gmail.com>
---
 fs/jfs/jfs_mount.c |  9 ++++++++-
 fs/jfs/super.c     | 17 ++++++++++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/fs/jfs/jfs_mount.c b/fs/jfs/jfs_mount.c
index dac822f15070..19099b888b53 100644
--- a/fs/jfs/jfs_mount.c
+++ b/fs/jfs/jfs_mount.c
@@ -235,6 +235,12 @@ int jfs_mount_rw(struct super_block *sb, int remount)
 		truncate_inode_pages(sbi->ipimap->i_mapping, 0);
 		truncate_inode_pages(sbi->ipbmap->i_mapping, 0);
 
+		/*
+		 * Rebuild both maps under the inode map lock so that
+		 * statfs(), which reads them without any other means of
+		 * synchronization, never observes the window where the
+		 * old structures are torn down.
+		 */
 		IWRITE_LOCK(sbi->ipimap, RDWRLOCK_IMAP);
 		diUnmount(sbi->ipimap, 1);
 		if ((rc = diMount(sbi->ipimap))) {
@@ -242,13 +248,14 @@ int jfs_mount_rw(struct super_block *sb, int remount)
 			jfs_err("jfs_mount_rw: diMount failed!");
 			return rc;
 		}
-		IWRITE_UNLOCK(sbi->ipimap);
 
 		dbUnmount(sbi->ipbmap, 1);
 		if ((rc = dbMount(sbi->ipbmap))) {
+			IWRITE_UNLOCK(sbi->ipimap);
 			jfs_err("jfs_mount_rw: dbMount failed!");
 			return rc;
 		}
+		IWRITE_UNLOCK(sbi->ipimap);
 	}
 
 	/*
diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index 8180d83d33fe..7a8365fdf2b2 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -120,9 +120,23 @@ static int jfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 {
 	struct jfs_sb_info *sbi = JFS_SBI(dentry->d_sb);
 	s64 maxinodes;
-	struct inomap *imap = JFS_IP(sbi->ipimap)->i_imap;
+	struct inomap *imap;
 
 	jfs_info("In jfs_statfs");
+
+	/*
+	 * A concurrent read-write remount tears down the in-memory inode
+	 * and block maps before installing the freshly built ones, leaving
+	 * them briefly NULL.  Exclude that window, and bail out if a failed
+	 * remount left us without maps at all.
+	 */
+	IREAD_LOCK(sbi->ipimap, RDWRLOCK_IMAP);
+	imap = JFS_IP(sbi->ipimap)->i_imap;
+	if (!imap || !sbi->bmap) {
+		IREAD_UNLOCK(sbi->ipimap);
+		return -EIO;
+	}
+
 	buf->f_type = JFS_SUPER_MAGIC;
 	buf->f_bsize = sbi->bsize;
 	buf->f_blocks = sbi->bmap->db_mapsize;
@@ -150,6 +164,7 @@ static int jfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 				      sizeof(sbi->uuid)/2);
 
 	buf->f_namelen = JFS_NAME_MAX;
+	IREAD_UNLOCK(sbi->ipimap);
 	return 0;
 }
 
-- 
2.25.1