[PATCHSET 0/3] xfs: fix buffer cancellation table leak during log recovery

[PATCHSET 0/3] xfs: fix buffer cancellation table leak during log recovery

[Darrick J. Wong]

Hi all,

As part of solving the memory leaks and UAF problems, kmemleak also
reported that log recovery will leak the table used to hash buffer
cancellations if the recovery fails.  Fix this problem by creating
alloc/free helpers that initialize and free the hashtable contents
correctly.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=log-recovery-leak-fixes-5.19
---
 fs/xfs/libxfs/xfs_log_recover.h |    2 ++
 fs/xfs/xfs_buf_item_recover.c   |   45 +++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_log_recover.c        |   23 ++++++++------------
 3 files changed, 56 insertions(+), 14 deletions(-)

[PATCH 1/3] xfs: refactor buffer cancellation table allocation

[Darrick J. Wong]

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

Move the code that allocates and frees the buffer cancellation tables
used by log recovery into the file that actually uses the tables.  This
is a precursor to some cleanups and a memory leak fix.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_log_recover.h |    2 ++
 fs/xfs/xfs_buf_item_recover.c   |   26 ++++++++++++++++++++++++++
 fs/xfs/xfs_log_recover.c        |   21 +++++++--------------
 3 files changed, 35 insertions(+), 14 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
index 32e216255cb0..c96102484aa6 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -128,5 +128,7 @@ int xlog_recover_iget(struct xfs_mount *mp, xfs_ino_t ino,
 		struct xfs_inode **ipp);
 void xlog_recover_release_intent(struct xlog *log, unsigned short intent_type,
 		uint64_t intent_id);
+void xlog_alloc_buf_cancel_table(struct xlog *log);
+void xlog_free_buf_cancel_table(struct xlog *log);
 
 #endif	/* __XFS_LOG_RECOVER_H__ */
diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
index e484251dc9c8..8624ac22ca4a 100644
--- a/fs/xfs/xfs_buf_item_recover.c
+++ b/fs/xfs/xfs_buf_item_recover.c
@@ -993,3 +993,29 @@ const struct xlog_recover_item_ops xlog_buf_item_ops = {
 	.commit_pass1		= xlog_recover_buf_commit_pass1,
 	.commit_pass2		= xlog_recover_buf_commit_pass2,
 };
+
+void
+xlog_alloc_buf_cancel_table(
+	struct xlog	*log)
+{
+	int		i;
+
+	ASSERT(log->l_buf_cancel_table == NULL);
+
+	log->l_buf_cancel_table = kmem_zalloc(XLOG_BC_TABLE_SIZE *
+						 sizeof(struct list_head),
+						 0);
+	for (i = 0; i < XLOG_BC_TABLE_SIZE; i++)
+		INIT_LIST_HEAD(&log->l_buf_cancel_table[i]);
+}
+
+void
+xlog_free_buf_cancel_table(
+	struct xlog	*log)
+{
+	if (!log->l_buf_cancel_table)
+		return;
+
+	kmem_free(log->l_buf_cancel_table);
+	log->l_buf_cancel_table = NULL;
+}
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 97b941c07957..bb10686ef88a 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3230,7 +3230,7 @@ xlog_do_log_recovery(
 	xfs_daddr_t	head_blk,
 	xfs_daddr_t	tail_blk)
 {
-	int		error, i;
+	int		error;
 
 	ASSERT(head_blk != tail_blk);
 
@@ -3238,19 +3238,13 @@ xlog_do_log_recovery(
 	 * First do a pass to find all of the cancelled buf log items.
 	 * Store them in the buf_cancel_table for use in the second pass.
 	 */
-	log->l_buf_cancel_table = kmem_zalloc(XLOG_BC_TABLE_SIZE *
-						 sizeof(struct list_head),
-						 0);
-	for (i = 0; i < XLOG_BC_TABLE_SIZE; i++)
-		INIT_LIST_HEAD(&log->l_buf_cancel_table[i]);
+	xlog_alloc_buf_cancel_table(log);
 
 	error = xlog_do_recovery_pass(log, head_blk, tail_blk,
 				      XLOG_RECOVER_PASS1, NULL);
-	if (error != 0) {
-		kmem_free(log->l_buf_cancel_table);
-		log->l_buf_cancel_table = NULL;
-		return error;
-	}
+	if (error != 0)
+		goto out_cancel;
+
 	/*
 	 * Then do a second pass to actually recover the items in the log.
 	 * When it is complete free the table of buf cancel items.
@@ -3266,9 +3260,8 @@ xlog_do_log_recovery(
 	}
 #endif	/* DEBUG */
 
-	kmem_free(log->l_buf_cancel_table);
-	log->l_buf_cancel_table = NULL;
-
+out_cancel:
+	xlog_free_buf_cancel_table(log);
 	return error;
 }
 

[PATCH 2/3] xfs: don't leak xfs_buf_cancel structures when recovery fails

[Darrick J. Wong]

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

If log recovery fails, we free the memory used by the buffer
cancellation buckets, but we don't actually traverse each bucket list to
free the individual xfs_buf_cancel objects.  This leads to a memory
leak, as reported by kmemleak in xfs/051:

unreferenced object 0xffff888103629560 (size 32):
  comm "mount", pid 687045, jiffies 4296935916 (age 10.752s)
  hex dump (first 32 bytes):
    08 d3 0a 01 00 00 00 00 08 00 00 00 01 00 00 00  ................
    d0 f5 0b 92 81 88 ff ff 80 64 64 25 81 88 ff ff  .........dd%....
  backtrace:
    [<ffffffffa0317c83>] kmem_alloc+0x73/0x140 [xfs]
    [<ffffffffa03234a9>] xlog_recover_buf_commit_pass1+0x139/0x200 [xfs]
    [<ffffffffa032dc27>] xlog_recover_commit_trans+0x307/0x350 [xfs]
    [<ffffffffa032df15>] xlog_recovery_process_trans+0xa5/0xe0 [xfs]
    [<ffffffffa032e12d>] xlog_recover_process_data+0x8d/0x140 [xfs]
    [<ffffffffa032e49d>] xlog_do_recovery_pass+0x19d/0x740 [xfs]
    [<ffffffffa032f22d>] xlog_do_log_recovery+0x6d/0x150 [xfs]
    [<ffffffffa032f343>] xlog_do_recover+0x33/0x1d0 [xfs]
    [<ffffffffa032faba>] xlog_recover+0xda/0x190 [xfs]
    [<ffffffffa03194bc>] xfs_log_mount+0x14c/0x360 [xfs]
    [<ffffffffa030bfed>] xfs_mountfs+0x50d/0xa60 [xfs]
    [<ffffffffa03124b5>] xfs_fs_fill_super+0x6a5/0x950 [xfs]
    [<ffffffff812b92a5>] get_tree_bdev+0x175/0x280
    [<ffffffff812b7c3a>] vfs_get_tree+0x1a/0x80
    [<ffffffff812e366f>] path_mount+0x6ff/0xaa0
    [<ffffffff812e3b13>] __x64_sys_mount+0x103/0x140

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_buf_item_recover.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)


diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
index 8624ac22ca4a..d1e484649a33 100644
--- a/fs/xfs/xfs_buf_item_recover.c
+++ b/fs/xfs/xfs_buf_item_recover.c
@@ -1013,9 +1013,22 @@ void
 xlog_free_buf_cancel_table(
 	struct xlog	*log)
 {
+	int		i;
+
 	if (!log->l_buf_cancel_table)
 		return;
 
+	for (i = 0; i < XLOG_BC_TABLE_SIZE; i++) {
+		struct xfs_buf_cancel	*bc;
+
+		while ((bc = list_first_entry_or_null(
+				&log->l_buf_cancel_table[i],
+				struct xfs_buf_cancel, bc_list))) {
+			list_del(&bc->bc_list);
+			kmem_free(bc);
+		}
+	}
+
 	kmem_free(log->l_buf_cancel_table);
 	log->l_buf_cancel_table = NULL;
 }

[PATCH 3/3] xfs: convert buf_cancel_table allocation to kmalloc_array

[Darrick J. Wong]

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

While we're messing around with how recovery allocates and frees the
buffer cancellation table, convert the allocation to use kmalloc_array
instead of the old kmem_alloc APIs, and make it handle a null return,
even though that's not likely.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_log_recover.h |    2 +-
 fs/xfs/xfs_buf_item_recover.c   |   14 ++++++++++----
 fs/xfs/xfs_log_recover.c        |    4 +++-
 3 files changed, 14 insertions(+), 6 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
index c96102484aa6..8227c74e75ec 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -128,7 +128,7 @@ int xlog_recover_iget(struct xfs_mount *mp, xfs_ino_t ino,
 		struct xfs_inode **ipp);
 void xlog_recover_release_intent(struct xlog *log, unsigned short intent_type,
 		uint64_t intent_id);
-void xlog_alloc_buf_cancel_table(struct xlog *log);
+int xlog_alloc_buf_cancel_table(struct xlog *log);
 void xlog_free_buf_cancel_table(struct xlog *log);
 
 #endif	/* __XFS_LOG_RECOVER_H__ */
diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
index d1e484649a33..4b54b808fb07 100644
--- a/fs/xfs/xfs_buf_item_recover.c
+++ b/fs/xfs/xfs_buf_item_recover.c
@@ -994,19 +994,25 @@ const struct xlog_recover_item_ops xlog_buf_item_ops = {
 	.commit_pass2		= xlog_recover_buf_commit_pass2,
 };
 
-void
+int
 xlog_alloc_buf_cancel_table(
 	struct xlog	*log)
 {
+	void		*p;
 	int		i;
 
 	ASSERT(log->l_buf_cancel_table == NULL);
 
-	log->l_buf_cancel_table = kmem_zalloc(XLOG_BC_TABLE_SIZE *
-						 sizeof(struct list_head),
-						 0);
+	p = kmalloc_array(XLOG_BC_TABLE_SIZE, sizeof(struct list_head),
+			GFP_KERNEL | __GFP_NOWARN);
+	if (!p)
+		return -ENOMEM;
+
+	log->l_buf_cancel_table = p;
 	for (i = 0; i < XLOG_BC_TABLE_SIZE; i++)
 		INIT_LIST_HEAD(&log->l_buf_cancel_table[i]);
+
+	return 0;
 }
 
 void
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index bb10686ef88a..f77e072426cd 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3238,7 +3238,9 @@ xlog_do_log_recovery(
 	 * First do a pass to find all of the cancelled buf log items.
 	 * Store them in the buf_cancel_table for use in the second pass.
 	 */
-	xlog_alloc_buf_cancel_table(log);
+	error = xlog_alloc_buf_cancel_table(log);
+	if (error)
+		return error;
 
 	error = xlog_do_recovery_pass(log, head_blk, tail_blk,
 				      XLOG_RECOVER_PASS1, NULL);

Re: [PATCH 1/3] xfs: refactor buffer cancellation table allocation

[Christoph Hellwig]

On Wed, May 18, 2022 at 11:55:29AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Move the code that allocates and frees the buffer cancellation tables
> used by log recovery into the file that actually uses the tables.  This
> is a precursor to some cleanups and a memory leak fix.

While you're at it, I'd also move XLOG_BC_TABLE_SIZE and
XLOG_BUF_CANCEL_BUCKET to xfs_buf_item_recover.c.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] xfs: don't leak xfs_buf_cancel structures when recovery fails

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/3] xfs: convert buf_cancel_table allocation to kmalloc_array

[Christoph Hellwig]

On Wed, May 18, 2022 at 11:55:40AM -0700, Darrick J. Wong wrote:
> +	p = kmalloc_array(XLOG_BC_TABLE_SIZE, sizeof(struct list_head),
> +			GFP_KERNEL | __GFP_NOWARN);

Why the __GFP_NOWARN?

Re: [PATCH 3/3] xfs: convert buf_cancel_table allocation to kmalloc_array

[Darrick J. Wong]

On Thu, May 19, 2022 at 01:36:57AM -0700, Christoph Hellwig wrote:
> On Wed, May 18, 2022 at 11:55:40AM -0700, Darrick J. Wong wrote:
> > +	p = kmalloc_array(XLOG_BC_TABLE_SIZE, sizeof(struct list_head),
> > +			GFP_KERNEL | __GFP_NOWARN);
> 
> Why the __GFP_NOWARN?

It's a straight port of xfs_km_flags_t==0, which is what the old code
did.  I suspect it doesn't make any practical difference since at most
this will be allocating 1k of memory.  Want me to make it GFP_KERNEL
only?

--D

Re: [PATCH 3/3] xfs: convert buf_cancel_table allocation to kmalloc_array

[Christoph Hellwig]

On Thu, May 19, 2022 at 10:08:44AM -0700, Darrick J. Wong wrote:
> On Thu, May 19, 2022 at 01:36:57AM -0700, Christoph Hellwig wrote:
> > On Wed, May 18, 2022 at 11:55:40AM -0700, Darrick J. Wong wrote:
> > > +	p = kmalloc_array(XLOG_BC_TABLE_SIZE, sizeof(struct list_head),
> > > +			GFP_KERNEL | __GFP_NOWARN);
> > 
> > Why the __GFP_NOWARN?
> 
> It's a straight port of xfs_km_flags_t==0, which is what the old code
> did.  I suspect it doesn't make any practical difference since at most
> this will be allocating 1k of memory.  Want me to make it GFP_KERNEL
> only?

I think that would make more sense.  With that:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/3] xfs: refactor buffer cancellation table allocation

[Darrick J. Wong]

On Thu, May 19, 2022 at 01:35:58AM -0700, Christoph Hellwig wrote:
> On Wed, May 18, 2022 at 11:55:29AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Move the code that allocates and frees the buffer cancellation tables
> > used by log recovery into the file that actually uses the tables.  This
> > is a precursor to some cleanups and a memory leak fix.
> 
> While you're at it, I'd also move XLOG_BC_TABLE_SIZE and
> XLOG_BUF_CANCEL_BUCKET to xfs_buf_item_recover.c.

Ok.  There's a bit of debugging code that will have to move too, but
that's not a big deal.  Thanks for the review. :)

--D

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/3] xfs: don't leak xfs_buf_cancel structures when recovery fails

[Darrick J. Wong]

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

If log recovery fails, we free the memory used by the buffer
cancellation buckets, but we don't actually traverse each bucket list to
free the individual xfs_buf_cancel objects.  This leads to a memory
leak, as reported by kmemleak in xfs/051:

unreferenced object 0xffff888103629560 (size 32):
  comm "mount", pid 687045, jiffies 4296935916 (age 10.752s)
  hex dump (first 32 bytes):
    08 d3 0a 01 00 00 00 00 08 00 00 00 01 00 00 00  ................
    d0 f5 0b 92 81 88 ff ff 80 64 64 25 81 88 ff ff  .........dd%....
  backtrace:
    [<ffffffffa0317c83>] kmem_alloc+0x73/0x140 [xfs]
    [<ffffffffa03234a9>] xlog_recover_buf_commit_pass1+0x139/0x200 [xfs]
    [<ffffffffa032dc27>] xlog_recover_commit_trans+0x307/0x350 [xfs]
    [<ffffffffa032df15>] xlog_recovery_process_trans+0xa5/0xe0 [xfs]
    [<ffffffffa032e12d>] xlog_recover_process_data+0x8d/0x140 [xfs]
    [<ffffffffa032e49d>] xlog_do_recovery_pass+0x19d/0x740 [xfs]
    [<ffffffffa032f22d>] xlog_do_log_recovery+0x6d/0x150 [xfs]
    [<ffffffffa032f343>] xlog_do_recover+0x33/0x1d0 [xfs]
    [<ffffffffa032faba>] xlog_recover+0xda/0x190 [xfs]
    [<ffffffffa03194bc>] xfs_log_mount+0x14c/0x360 [xfs]
    [<ffffffffa030bfed>] xfs_mountfs+0x50d/0xa60 [xfs]
    [<ffffffffa03124b5>] xfs_fs_fill_super+0x6a5/0x950 [xfs]
    [<ffffffff812b92a5>] get_tree_bdev+0x175/0x280
    [<ffffffff812b7c3a>] vfs_get_tree+0x1a/0x80
    [<ffffffff812e366f>] path_mount+0x6ff/0xaa0
    [<ffffffff812e3b13>] __x64_sys_mount+0x103/0x140

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf_item_recover.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)


diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
index d2e2dff01b99..f983af4de0a5 100644
--- a/fs/xfs/xfs_buf_item_recover.c
+++ b/fs/xfs/xfs_buf_item_recover.c
@@ -1034,9 +1034,22 @@ void
 xlog_free_buf_cancel_table(
 	struct xlog	*log)
 {
+	int		i;
+
 	if (!log->l_buf_cancel_table)
 		return;
 
+	for (i = 0; i < XLOG_BC_TABLE_SIZE; i++) {
+		struct xfs_buf_cancel	*bc;
+
+		while ((bc = list_first_entry_or_null(
+				&log->l_buf_cancel_table[i],
+				struct xfs_buf_cancel, bc_list))) {
+			list_del(&bc->bc_list);
+			kmem_free(bc);
+		}
+	}
+
 	kmem_free(log->l_buf_cancel_table);
 	log->l_buf_cancel_table = NULL;
 }

Re: [PATCH 2/3] xfs: don't leak xfs_buf_cancel structures when recovery fails

[Dave Chinner]

On Mon, May 23, 2022 at 10:35:56PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If log recovery fails, we free the memory used by the buffer
> cancellation buckets, but we don't actually traverse each bucket list to
> free the individual xfs_buf_cancel objects.  This leads to a memory
> leak, as reported by kmemleak in xfs/051:
> 
> unreferenced object 0xffff888103629560 (size 32):
>   comm "mount", pid 687045, jiffies 4296935916 (age 10.752s)
>   hex dump (first 32 bytes):
>     08 d3 0a 01 00 00 00 00 08 00 00 00 01 00 00 00  ................
>     d0 f5 0b 92 81 88 ff ff 80 64 64 25 81 88 ff ff  .........dd%....
>   backtrace:
>     [<ffffffffa0317c83>] kmem_alloc+0x73/0x140 [xfs]
>     [<ffffffffa03234a9>] xlog_recover_buf_commit_pass1+0x139/0x200 [xfs]
>     [<ffffffffa032dc27>] xlog_recover_commit_trans+0x307/0x350 [xfs]
>     [<ffffffffa032df15>] xlog_recovery_process_trans+0xa5/0xe0 [xfs]
>     [<ffffffffa032e12d>] xlog_recover_process_data+0x8d/0x140 [xfs]
>     [<ffffffffa032e49d>] xlog_do_recovery_pass+0x19d/0x740 [xfs]
>     [<ffffffffa032f22d>] xlog_do_log_recovery+0x6d/0x150 [xfs]
>     [<ffffffffa032f343>] xlog_do_recover+0x33/0x1d0 [xfs]
>     [<ffffffffa032faba>] xlog_recover+0xda/0x190 [xfs]
>     [<ffffffffa03194bc>] xfs_log_mount+0x14c/0x360 [xfs]
>     [<ffffffffa030bfed>] xfs_mountfs+0x50d/0xa60 [xfs]
>     [<ffffffffa03124b5>] xfs_fs_fill_super+0x6a5/0x950 [xfs]
>     [<ffffffff812b92a5>] get_tree_bdev+0x175/0x280
>     [<ffffffff812b7c3a>] vfs_get_tree+0x1a/0x80
>     [<ffffffff812e366f>] path_mount+0x6ff/0xaa0
>     [<ffffffff812e3b13>] __x64_sys_mount+0x103/0x140
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_buf_item_recover.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com