stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Luis Chamberlain <mcgrof@kernel.org>
To: linux-xfs@vger.kernel.org, gregkh@linuxfoundation.org,
	Alexander.Levin@microsoft.com
Cc: stable@vger.kernel.org, amir73il@gmail.com, hch@infradead.org,
	Dave Chinner <dchinner@redhat.com>,
	"Darrick J . Wong" <darrick.wong@oracle.com>,
	Luis Chamberlain <mcgrof@kernel.org>
Subject: [PATCH v2 06/10] xfs: fix transient reference count error in xfs_buf_resubmit_failed_buffers
Date: Mon,  4 Feb 2019 08:54:23 -0800	[thread overview]
Message-ID: <20190204165427.23607-7-mcgrof@kernel.org> (raw)
In-Reply-To: <20190204165427.23607-1-mcgrof@kernel.org>

From: Dave Chinner <dchinner@redhat.com>

commit d43aaf1685aa471f0593685c9f54d53e3af3cf3f upstream.

When retrying a failed inode or dquot buffer,
xfs_buf_resubmit_failed_buffers() clears all the failed flags from
the inde/dquot log items. In doing so, it also drops all the
reference counts on the buffer that the failed log items hold. This
means it can drop all the active references on the buffer and hence
free the buffer before it queues it for write again.

Putting the buffer on the delwri queue takes a reference to the
buffer (so that it hangs around until it has been written and
completed), but this goes bang if the buffer has already been freed.

Hence we need to add the buffer to the delwri queue before we remove
the failed flags from the log items attached to the buffer to ensure
it always remains referenced during the resubmit process.

Reported-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 fs/xfs/xfs_buf_item.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 12d8455bfbb2..010db5f8fb00 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1233,9 +1233,23 @@ xfs_buf_iodone(
 }
 
 /*
- * Requeue a failed buffer for writeback
+ * Requeue a failed buffer for writeback.
  *
- * Return true if the buffer has been re-queued properly, false otherwise
+ * We clear the log item failed state here as well, but we have to be careful
+ * about reference counts because the only active reference counts on the buffer
+ * may be the failed log items. Hence if we clear the log item failed state
+ * before queuing the buffer for IO we can release all active references to
+ * the buffer and free it, leading to use after free problems in
+ * xfs_buf_delwri_queue. It makes no difference to the buffer or log items which
+ * order we process them in - the buffer is locked, and we own the buffer list
+ * so nothing on them is going to change while we are performing this action.
+ *
+ * Hence we can safely queue the buffer for IO before we clear the failed log
+ * item state, therefore  always having an active reference to the buffer and
+ * avoiding the transient zero-reference state that leads to use-after-free.
+ *
+ * Return true if the buffer was added to the buffer list, false if it was
+ * already on the buffer list.
  */
 bool
 xfs_buf_resubmit_failed_buffers(
@@ -1243,16 +1257,16 @@ xfs_buf_resubmit_failed_buffers(
 	struct list_head	*buffer_list)
 {
 	struct xfs_log_item	*lip;
+	bool			ret;
+
+	ret = xfs_buf_delwri_queue(bp, buffer_list);
 
 	/*
-	 * Clear XFS_LI_FAILED flag from all items before resubmit
-	 *
-	 * XFS_LI_FAILED set/clear is protected by ail_lock, caller  this
+	 * XFS_LI_FAILED set/clear is protected by ail_lock, caller of this
 	 * function already have it acquired
 	 */
 	list_for_each_entry(lip, &bp->b_li_list, li_bio_list)
 		xfs_clear_li_failed(lip);
 
-	/* Add this buffer back to the delayed write list */
-	return xfs_buf_delwri_queue(bp, buffer_list);
+	return ret;
 }
-- 
2.18.0


  parent reply	other threads:[~2019-02-04 16:54 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-04 16:54 [PATCH v2 00/10] xfs: stable fixes for v4.19.y Luis Chamberlain
2019-02-04 16:54 ` [PATCH v2 01/10] xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat Luis Chamberlain
2019-02-04 16:54 ` [PATCH v2 02/10] xfs: cancel COW blocks before swapext Luis Chamberlain
2019-02-04 16:54 ` [PATCH v2 03/10] xfs: Fix error code in 'xfs_ioc_getbmap()' Luis Chamberlain
2019-02-04 16:54 ` [PATCH v2 04/10] xfs: fix overflow in xfs_attr3_leaf_verify Luis Chamberlain
2019-02-04 16:54 ` [PATCH v2 05/10] xfs: fix shared extent data corruption due to missing cow reservation Luis Chamberlain
2019-02-04 16:54 ` Luis Chamberlain [this message]
2019-02-04 16:54 ` [PATCH v2 07/10] xfs: delalloc -> unwritten COW fork allocation can go wrong Luis Chamberlain
2019-02-04 16:54 ` [PATCH v2 08/10] fs/xfs: fix f_ffree value for statfs when project quota is set Luis Chamberlain
2019-02-04 16:54 ` [PATCH v2 09/10] xfs: fix PAGE_MASK usage in xfs_free_file_space Luis Chamberlain
2019-02-04 16:54 ` [PATCH v2 10/10] xfs: fix inverted return from xfs_btree_sblock_verify_crc Luis Chamberlain
2019-02-05  6:44 ` [PATCH v2 00/10] xfs: stable fixes for v4.19.y Amir Goldstein
2019-02-05 22:06 ` Dave Chinner
2019-02-06  4:05   ` Sasha Levin
2019-02-06 21:54     ` Dave Chinner
2019-02-08  6:06       ` Sasha Levin
2019-02-08 20:06         ` Luis Chamberlain
2019-02-08 21:29         ` Dave Chinner
2019-02-09 17:53           ` Sasha Levin
2019-02-08 22:17         ` Luis Chamberlain
2019-02-09 21:56           ` Sasha Levin
2019-02-11 19:46             ` Luis Chamberlain
2019-02-08 19:48   ` Luis Chamberlain
2019-02-08 21:32     ` Dave Chinner
2019-02-08 21:50       ` Luis Chamberlain
2019-02-10 22:12         ` Dave Chinner
2019-02-11 20:09     ` Luis Chamberlain
2019-02-10  0:06 ` Sasha Levin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190204165427.23607-7-mcgrof@kernel.org \
    --to=mcgrof@kernel.org \
    --cc=Alexander.Levin@microsoft.com \
    --cc=amir73il@gmail.com \
    --cc=darrick.wong@oracle.com \
    --cc=dchinner@redhat.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).