[PATCH 5.10 CANDIDATE 0/7] xfs stable candidate patches for 5.10.y (from v5.17)

[PATCH 5.10 CANDIDATE 0/7] xfs stable candidate patches for 5.10.y (from v5.17)

[Amir Goldstein]

Hi Darrick,

This is my collection of backports from v5.17.

Patch 1 is a small fix picked from Leah's part 3 series for 5.15.y [1].

Patch 2 is a small fix picked from Leah's part 4 series [2].
I have some more fixes queued from part 4, but they are not from v5.17,
so will be posted in another series for v5.18/v5.19 fixes.

Patches 3-6 are debt from the joint 5.10.y/5.15.y series [3].
Per your request in the review of that series, I collected all
the sync_fs patches and verified that they fix test xfs/546.

These patches have been spinning on kdevops for several days with
no regressions observed.

Please ACK.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-xfs/20220721213610.2794134-1-leah.rumancik@gmail.com/
[2] https://lore.kernel.org/linux-xfs/20220819181431.4113819-1-leah.rumancik@gmail.com/
[3] https://lore.kernel.org/linux-xfs/CAOQ4uxjrLUjStjDGOV2-0SK6ur07KZ8hAzb6JP+Dsm8=0iEbSA@mail.gmail.com/

Christoph Hellwig (1):
  fs: remove __sync_filesystem

Dan Carpenter (1):
  xfs: prevent a WARN_ONCE() in xfs_ioc_attr_list()

Darrick J. Wong (4):
  xfs: reject crazy array sizes being fed to XFS_IOC_GETBMAP*
  vfs: make sync_filesystem return errors from ->sync_fs
  xfs: return errors in xfs_fs_sync_fs
  xfs: only bother with sync_filesystem during readonly remount

 fs/sync.c          | 48 ++++++++++++++++++++++++----------------------
 fs/xfs/xfs_ioctl.c |  4 ++--
 fs/xfs/xfs_ioctl.h |  5 +++--
 fs/xfs/xfs_super.c | 13 ++++++++++---
 4 files changed, 40 insertions(+), 30 deletions(-)

-- 
2.25.1

[PATCH 5.10 CANDIDATE 1/6] xfs: prevent a WARN_ONCE() in xfs_ioc_attr_list()

[Amir Goldstein]

From: Dan Carpenter <dan.carpenter@oracle.com>

commit 6ed6356b07714e0198be3bc3ecccc8b40a212de4 upstream.

The "bufsize" comes from the root user.  If "bufsize" is negative then,
because of type promotion, neither of the validation checks at the start
of the function are able to catch it:

	if (bufsize < sizeof(struct xfs_attrlist) ||
	    bufsize > XFS_XATTR_LIST_MAX)
		return -EINVAL;

This means "bufsize" will trigger (WARN_ON_ONCE(size > INT_MAX)) in
kvmalloc_node().  Fix this by changing the type from int to size_t.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/xfs_ioctl.c | 2 +-
 fs/xfs/xfs_ioctl.h | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 646735aad45d..d973350d5946 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -371,7 +371,7 @@ int
 xfs_ioc_attr_list(
 	struct xfs_inode		*dp,
 	void __user			*ubuf,
-	int				bufsize,
+	size_t				bufsize,
 	int				flags,
 	struct xfs_attrlist_cursor __user *ucursor)
 {
diff --git a/fs/xfs/xfs_ioctl.h b/fs/xfs/xfs_ioctl.h
index bab6a5a92407..416e20de66e7 100644
--- a/fs/xfs/xfs_ioctl.h
+++ b/fs/xfs/xfs_ioctl.h
@@ -38,8 +38,9 @@ xfs_readlink_by_handle(
 int xfs_ioc_attrmulti_one(struct file *parfilp, struct inode *inode,
 		uint32_t opcode, void __user *uname, void __user *value,
 		uint32_t *len, uint32_t flags);
-int xfs_ioc_attr_list(struct xfs_inode *dp, void __user *ubuf, int bufsize,
-	int flags, struct xfs_attrlist_cursor __user *ucursor);
+int xfs_ioc_attr_list(struct xfs_inode *dp, void __user *ubuf,
+		      size_t bufsize, int flags,
+		      struct xfs_attrlist_cursor __user *ucursor);
 
 extern struct dentry *
 xfs_handle_to_dentry(
-- 
2.25.1

[PATCH 5.10 CANDIDATE 2/6] xfs: reject crazy array sizes being fed to XFS_IOC_GETBMAP*

[Amir Goldstein]

From: "Darrick J. Wong" <djwong@kernel.org>

commit 29d650f7e3ab55283b89c9f5883d0c256ce478b5 upstream.

Syzbot tripped over the following complaint from the kernel:

WARNING: CPU: 2 PID: 15402 at mm/util.c:597 kvmalloc_node+0x11e/0x125 mm/util.c:597

While trying to run XFS_IOC_GETBMAP against the following structure:

struct getbmap fubar = {
	.bmv_count	= 0x22dae649,
};

Obviously, this is a crazy huge value since the next thing that the
ioctl would do is allocate 37GB of memory.  This is enough to make
kvmalloc mad, but isn't large enough to trip the validation functions.
In other words, I'm fussing with checks that were **already sufficient**
because that's easier than dealing with 644 internal bug reports.  Yes,
that's right, six hundred and forty-four.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
Reviewed-by: Catherine Hoang <catherine.hoang@oracle.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/xfs_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index d973350d5946..103fa8381e7d 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1689,7 +1689,7 @@ xfs_ioc_getbmap(
 
 	if (bmx.bmv_count < 2)
 		return -EINVAL;
-	if (bmx.bmv_count > ULONG_MAX / recsize)
+	if (bmx.bmv_count >= INT_MAX / recsize)
 		return -ENOMEM;
 
 	buf = kvzalloc(bmx.bmv_count * sizeof(*buf), GFP_KERNEL);
-- 
2.25.1

[PATCH 5.10 CANDIDATE 3/6] fs: remove __sync_filesystem

[Amir Goldstein]

From: Christoph Hellwig <hch@lst.de>

commit 9a208ba5c9afa62c7b1e9c6f5e783066e84e2d3c upstream.

[backported for dependency]

There is no clear benefit in having this helper vs just open coding it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Link: https://lore.kernel.org/r/20211019062530.2174626-2-hch@lst.de
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/sync.c | 38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/fs/sync.c b/fs/sync.c
index 1373a610dc78..0d6cdc507cb9 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -21,25 +21,6 @@
 #define VALID_FLAGS (SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE| \
 			SYNC_FILE_RANGE_WAIT_AFTER)
 
-/*
- * Do the filesystem syncing work. For simple filesystems
- * writeback_inodes_sb(sb) just dirties buffers with inodes so we have to
- * submit IO for these buffers via __sync_blockdev(). This also speeds up the
- * wait == 1 case since in that case write_inode() functions do
- * sync_dirty_buffer() and thus effectively write one block at a time.
- */
-static int __sync_filesystem(struct super_block *sb, int wait)
-{
-	if (wait)
-		sync_inodes_sb(sb);
-	else
-		writeback_inodes_sb(sb, WB_REASON_SYNC);
-
-	if (sb->s_op->sync_fs)
-		sb->s_op->sync_fs(sb, wait);
-	return __sync_blockdev(sb->s_bdev, wait);
-}
-
 /*
  * Write out and wait upon all dirty data associated with this
  * superblock.  Filesystem data as well as the underlying block
@@ -61,10 +42,25 @@ int sync_filesystem(struct super_block *sb)
 	if (sb_rdonly(sb))
 		return 0;
 
-	ret = __sync_filesystem(sb, 0);
+	/*
+	 * Do the filesystem syncing work.  For simple filesystems
+	 * writeback_inodes_sb(sb) just dirties buffers with inodes so we have
+	 * to submit I/O for these buffers via __sync_blockdev().  This also
+	 * speeds up the wait == 1 case since in that case write_inode()
+	 * methods call sync_dirty_buffer() and thus effectively write one block
+	 * at a time.
+	 */
+	writeback_inodes_sb(sb, WB_REASON_SYNC);
+	if (sb->s_op->sync_fs)
+		sb->s_op->sync_fs(sb, 0);
+	ret = __sync_blockdev(sb->s_bdev, 0);
 	if (ret < 0)
 		return ret;
-	return __sync_filesystem(sb, 1);
+
+	sync_inodes_sb(sb);
+	if (sb->s_op->sync_fs)
+		sb->s_op->sync_fs(sb, 1);
+	return __sync_blockdev(sb->s_bdev, 1);
 }
 EXPORT_SYMBOL(sync_filesystem);
 
-- 
2.25.1

[PATCH 5.10 CANDIDATE 4/6] vfs: make sync_filesystem return errors from ->sync_fs

[Amir Goldstein]

From: "Darrick J. Wong" <djwong@kernel.org>

commit 5679897eb104cec9e99609c3f045a0c20603da4c upstream.

[backport to 5.10 only differs in __sync_blockdev helper]

Strangely, sync_filesystem ignores the return code from the ->sync_fs
call, which means that syscalls like syncfs(2) never see the error.
This doesn't seem right, so fix that.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/sync.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/sync.c b/fs/sync.c
index 0d6cdc507cb9..79180e58d862 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -28,7 +28,7 @@
  */
 int sync_filesystem(struct super_block *sb)
 {
-	int ret;
+	int ret = 0;
 
 	/*
 	 * We need to be protected against the filesystem going from
@@ -51,15 +51,21 @@ int sync_filesystem(struct super_block *sb)
 	 * at a time.
 	 */
 	writeback_inodes_sb(sb, WB_REASON_SYNC);
-	if (sb->s_op->sync_fs)
-		sb->s_op->sync_fs(sb, 0);
+	if (sb->s_op->sync_fs) {
+		ret = sb->s_op->sync_fs(sb, 0);
+		if (ret)
+			return ret;
+	}
 	ret = __sync_blockdev(sb->s_bdev, 0);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	sync_inodes_sb(sb);
-	if (sb->s_op->sync_fs)
-		sb->s_op->sync_fs(sb, 1);
+	if (sb->s_op->sync_fs) {
+		ret = sb->s_op->sync_fs(sb, 1);
+		if (ret)
+			return ret;
+	}
 	return __sync_blockdev(sb->s_bdev, 1);
 }
 EXPORT_SYMBOL(sync_filesystem);
-- 
2.25.1

[PATCH 5.10 CANDIDATE 5/6] xfs: return errors in xfs_fs_sync_fs

[Amir Goldstein]

From: "Darrick J. Wong" <djwong@kernel.org>

commit 2d86293c70750e4331e9616aded33ab6b47c299d upstream.

Now that the VFS will do something with the return values from
->sync_fs, make ours pass on error codes.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/xfs_super.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 6323974d6b3e..ff686cb16c7b 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -757,6 +757,7 @@ xfs_fs_sync_fs(
 	int			wait)
 {
 	struct xfs_mount	*mp = XFS_M(sb);
+	int			error;
 
 	/*
 	 * Doing anything during the async pass would be counterproductive.
@@ -764,7 +765,10 @@ xfs_fs_sync_fs(
 	if (!wait)
 		return 0;
 
-	xfs_log_force(mp, XFS_LOG_SYNC);
+	error = xfs_log_force(mp, XFS_LOG_SYNC);
+	if (error)
+		return error;
+
 	if (laptop_mode) {
 		/*
 		 * The disk must be active because we're syncing.
-- 
2.25.1

[PATCH 5.10 CANDIDATE 6/6] xfs: only bother with sync_filesystem during readonly remount

[Amir Goldstein]

From: "Darrick J. Wong" <djwong@kernel.org>

commit b97cca3ba9098522e5a1c3388764ead42640c1a5 upstream.

In commit 02b9984d6408, we pushed a sync_filesystem() call from the VFS
into xfs_fs_remount.  The only time that we ever need to push dirty file
data or metadata to disk for a remount is if we're remounting the
filesystem read only, so this really could be moved to xfs_remount_ro.

Once we've moved the call site, actually check the return value from
sync_filesystem.

Fixes: 02b9984d6408 ("fs: push sync_filesystem() down to the file system's remount_fs()")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/xfs_super.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index ff686cb16c7b..434c87cc9fbf 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1720,6 +1720,11 @@ xfs_remount_ro(
 	};
 	int			error;
 
+	/* Flush all the dirty data to disk. */
+	error = sync_filesystem(mp->m_super);
+	if (error)
+		return error;
+
 	/*
 	 * Cancel background eofb scanning so it cannot race with the final
 	 * log force+buftarg wait and deadlock the remount.
@@ -1790,8 +1795,6 @@ xfs_fc_reconfigure(
 	if (error)
 		return error;
 
-	sync_filesystem(mp->m_super);
-
 	/* inode32 -> inode64 */
 	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
 	    !(new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {
-- 
2.25.1

Re: [PATCH 5.10 CANDIDATE 0/7] xfs stable candidate patches for 5.10.y (from v5.17)

[Darrick J. Wong]

On Mon, Aug 22, 2022 at 07:27:56PM +0300, Amir Goldstein wrote:
> Hi Darrick,
> 
> This is my collection of backports from v5.17.
> 
> Patch 1 is a small fix picked from Leah's part 3 series for 5.15.y [1].
> 
> Patch 2 is a small fix picked from Leah's part 4 series [2].
> I have some more fixes queued from part 4, but they are not from v5.17,
> so will be posted in another series for v5.18/v5.19 fixes.
> 
> Patches 3-6 are debt from the joint 5.10.y/5.15.y series [3].
> Per your request in the review of that series, I collected all
> the sync_fs patches and verified that they fix test xfs/546.
> 
> These patches have been spinning on kdevops for several days with
> no regressions observed.
> 
> Please ACK.

Looks good to me,
Acked-by: Darrick J. Wong <djwong@kernel.org>

--D

> 
> Thanks,
> Amir.
> 
> [1] https://lore.kernel.org/linux-xfs/20220721213610.2794134-1-leah.rumancik@gmail.com/
> [2] https://lore.kernel.org/linux-xfs/20220819181431.4113819-1-leah.rumancik@gmail.com/
> [3] https://lore.kernel.org/linux-xfs/CAOQ4uxjrLUjStjDGOV2-0SK6ur07KZ8hAzb6JP+Dsm8=0iEbSA@mail.gmail.com/
> 
> Christoph Hellwig (1):
>   fs: remove __sync_filesystem
> 
> Dan Carpenter (1):
>   xfs: prevent a WARN_ONCE() in xfs_ioc_attr_list()
> 
> Darrick J. Wong (4):
>   xfs: reject crazy array sizes being fed to XFS_IOC_GETBMAP*
>   vfs: make sync_filesystem return errors from ->sync_fs
>   xfs: return errors in xfs_fs_sync_fs
>   xfs: only bother with sync_filesystem during readonly remount
> 
>  fs/sync.c          | 48 ++++++++++++++++++++++++----------------------
>  fs/xfs/xfs_ioctl.c |  4 ++--
>  fs/xfs/xfs_ioctl.h |  5 +++--
>  fs/xfs/xfs_super.c | 13 ++++++++++---
>  4 files changed, 40 insertions(+), 30 deletions(-)
> 
> -- 
> 2.25.1
>