Message ID | 20181119210459.8506-4-david@fromorbit.com (mailing list archive) |
---|---|
State | Accepted, archived |
Headers | show |
Series | xfs: various fixes for 4.20 | expand |
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Tue, Nov 20, 2018 at 08:04:55AM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > 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> Looks ok, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > 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.19.1 >
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; }