xfs: fix buffer use after free on IO error

PATCH In review Eric Sandeen 5 participants 9 messages Mar 22, 2014 – Mar 25, 2014 XFS filesystem

lore

Share & tools

lore

Eric Sandeen

author

2014-03-22 02:48 UTC #0 lore raw

PATCH 1/1 xfs: fix buffer use after free on IO error

When testing exhaustion of dm snapshots, the following appeared with CONFIG_DEBUG_OBJECTS_FREE enabled:

ODEBUG: free active (active state 0) object type: work_struct hint: xfs_buf_iodone_work+0x0/0x1d0 [xfs]

indicating that we'd freed a buffer which still had a pending reference, down this path:

[  190.867975]  [<ffffffff8133e6fb>] debug_check_no_obj_freed+0x22b/0x270
[  190.880820]  [<ffffffff811da1d0>] kmem_cache_free+0xd0/0x370
[  190.892615]  [<ffffffffa02c5924>] xfs_buf_free+0xe4/0x210 [xfs]
[  190.905629]  [<ffffffffa02c6167>] xfs_buf_rele+0xe7/0x270 [xfs]
[  190.911770]  [<ffffffffa034c826>] xfs_trans_read_buf_map+0x7b6/0xac0 [xfs]

At issue is the fact that if IO fails in xfs_buf_iorequest, we'll queue completion unconditionally, and then call xfs_buf_rele; but if IO failed, there are no IOs remaining, and xfs_buf_rele will free the bp while work is still queued.

Fix this by not scheduling completion if the buffer has an error on it; run it immediately. The rest is only comment changes.

Thanks to dchinner for spotting the root cause.

Signed-off-by Eric Sandeen <[email protected]>

---

p.s. or maybe this could be moved into _xfs_buf_ioend ... I think I see some nice cleanups for xfs_buf_ioend vs. _xfs_buf_ioend w.r.t. when "schedule" is true, so maybe I can clean it up then.

fs/xfs/xfs_buf.c +12 -4

index 9c061ef..45eb5ef 100644
@@ -1361,21 +1361,29 @@ xfs_buf_iorequest(
 		xfs_buf_wait_unpin(bp);
 	xfs_buf_hold(bp);
 
-	/* Set the count to 1 initially, this will stop an I/O
+	/*
+	 * Set the count to 1 initially, this will stop an I/O
 	 * completion callout which happens before we have started
 	 * all the I/O from calling xfs_buf_ioend too early.
 	 */
 	atomic_set(&bp->b_io_remaining, 1);
 	_xfs_buf_ioapply(bp);
-	_xfs_buf_ioend(bp, 1);
+	/*
+	 * If _xfs_buf_ioapply failed, we'll get back here with
+	 * only the reference we took above.  _xfs_buf_ioend will
+	 * drop it to zero, so we'd better not queue it for later,
+	 * or we'll free it before it's done.
+	 */
+	_xfs_buf_ioend(bp, bp->b_error ? 0 : 1);
 
 	xfs_buf_rele(bp);
 }
 
 /*
  * Waits for I/O to complete on the buffer supplied.  It returns immediately if
- * no I/O is pending or there is already a pending error on the buffer.  It
- * returns the I/O error code, if any, or 0 if there was no error.
+ * no I/O is pending or there is already a pending error on the buffer, in which
+ * case nothing will ever complete.  It returns the I/O error code, if any, or
+ * 0 if there was no error.
  */
 int
 xfs_buf_iowait(

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Brian Foster

Reviewed-by on v1 1/1

2014-03-25 12:58 UTC #1 lore raw

Quoted 49 lines · Eric Sandeen

When testing exhaustion of dm snapshots, the following appeared with CONFIG_DEBUG_OBJECTS_FREE enabled:
ODEBUG: free active (active state 0) object type: work_struct hint: xfs_buf_iodone_work+0x0/0x1d0 [xfs]
indicating that we'd freed a buffer which still had a pending reference, down this path:
[  190.867975]  [<ffffffff8133e6fb>] debug_check_no_obj_freed+0x22b/0x270
[  190.880820]  [<ffffffff811da1d0>] kmem_cache_free+0xd0/0x370
[  190.892615]  [<ffffffffa02c5924>] xfs_buf_free+0xe4/0x210 [xfs]
[  190.905629]  [<ffffffffa02c6167>] xfs_buf_rele+0xe7/0x270 [xfs]
[  190.911770]  [<ffffffffa034c826>] xfs_trans_read_buf_map+0x7b6/0xac0 [xfs]
At issue is the fact that if IO fails in xfs_buf_iorequest, we'll queue completion unconditionally, and then call xfs_buf_rele; but if IO failed, there are no IOs remaining, and xfs_buf_rele will free the bp while work is still queued.
Fix this by not scheduling completion if the buffer has an error on it; run it immediately. The rest is only comment changes.
Thanks to dchinner for spotting the root cause.
Signed-off-by Eric Sandeen <[email protected]>
---
p.s. or maybe this could be moved into _xfs_buf_ioend ... I think I see some nice cleanups for xfs_buf_ioend vs. _xfs_buf_ioend w.r.t. when "schedule" is true, so maybe I can clean it up then.
fs/xfs/xfs_buf.c +2 -1
index 9c061ef..45eb5ef 100644
@@ -1361,21 +1361,29 @@ xfs_buf_iorequest(
 		xfs_buf_wait_unpin(bp);
 	xfs_buf_hold(bp);
 
-	/* Set the count to 1 initially, this will stop an I/O
+	/*
+	 * Set the count to 1 initially, this will stop an I/O
 	 * completion callout which happens before we have started
 	 * all the I/O from calling xfs_buf_ioend too early.
 	 */
 	atomic_set(&bp->b_io_remaining, 1);

fs/xfs/xfs_buf.c xfs_buf_iorequest( · line 1378

Quoted 10 lines

fs/xfs/xfs_buf.c +7 -1

 	_xfs_buf_ioapply(bp);
-	_xfs_buf_ioend(bp, 1);
+	/*
+	 * If _xfs_buf_ioapply failed, we'll get back here with
+	 * only the reference we took above.  _xfs_buf_ioend will
+	 * drop it to zero, so we'd better not queue it for later,
+	 * or we'll free it before it's done.
+	 */
+	_xfs_buf_ioend(bp, bp->b_error ? 0 : 1);

Out of curiosity, is there any major reason we don't use 0 here unconditionally? Are we worried about I/O completing before we have a chance to decrement the reference?

Looks good to me either way:

Reviewed-by Brian Foster <[email protected]>

Quoted 14 lines

fs/xfs/xfs_buf.c +3 -2

 	xfs_buf_rele(bp);
 }
 
 /*
  * Waits for I/O to complete on the buffer supplied.  It returns immediately if
- * no I/O is pending or there is already a pending error on the buffer.  It
- * returns the I/O error code, if any, or 0 if there was no error.
+ * no I/O is pending or there is already a pending error on the buffer, in which
+ * case nothing will ever complete.  It returns the I/O error code, if any, or
+ * 0 if there was no error.
  */
 int
 xfs_buf_iowait(

Quoted 4 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

2014-03-25 13:17 UTC #2 lore raw

Quoted 5 lines · Eric Sandeen

Quoted 4 lines

fs/xfs/xfs_buf.c +4 -0

+	/*
+	 * If _xfs_buf_ioapply failed, we'll get back here with
+	 * only the reference we took above.  _xfs_buf_ioend will
+	 * drop it to zero, so we'd better not queue it for later,

Quoted 8 lines

fs/xfs/xfs_buf.c xfs_buf_iorequest( · line 1377
Quoted 4 lines
fs/xfs/xfs_buf.c +3 -0
+	 * or we'll free it before it's done.
+	 */
+	_xfs_buf_ioend(bp, bp->b_error ? 0 : 1);
Out of curiosity, is there any major reason we don't use 0 here unconditionally? Are we worried about I/O completing before we have a chance to decrement the reference?

I think this should unconditionally avoid the schedule, and while we're at it we should kill _xfs_buf_ioend and opencode it here and at the other callsite.

Also atomic_dec_and_test really just returns true/false - there should ne no need for the explicit == 1 in the conditional.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2014-03-25 16:05 UTC #3 lore raw

Quoted 14 lines · Christoph Hellwig

Quoted 5 lines · Eric Sandeen
Quoted 4 lines
fs/xfs/xfs_buf.c +4 -0
+	/*
+	 * If _xfs_buf_ioapply failed, we'll get back here with
+	 * only the reference we took above.  _xfs_buf_ioend will
+	 * drop it to zero, so we'd better not queue it for later,
Quoted 8 lines
fs/xfs/xfs_buf.c xfs_buf_iorequest( · line 1377
Quoted 4 lines
fs/xfs/xfs_buf.c +3 -0
+	 * or we'll free it before it's done.
+	 */
+	_xfs_buf_ioend(bp, bp->b_error ? 0 : 1);
Out of curiosity, is there any major reason we don't use 0 here unconditionally? Are we worried about I/O completing before we have a chance to decrement the reference?

Quoted 3 lines

I think this should unconditionally avoid the schedule, and while we're at it we should kill _xfs_buf_ioend and opencode it here and at the other callsite.

And then remove the flag from xfs_buf_ioend which is always 0 at that point ...

Quoted 2 lines

Also atomic_dec_and_test really just returns true/false - there should ne no need for the explicit == 1 in the conditional.

Yeah I have a patch to do that as well; I wanted to separate the bugfix from the more invasive cleanup, though - and I wanted to get the fix out for review sooner.

But yeah, I was unsure about whether or not to schedule at all here. We come here from a lot of callsites and I'm honestly not sure what the implications are yet.

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

2014-03-25 17:25 UTC #4 lore raw

Quoted 10 lines · Eric Sandeen

Quoted 7 lines
Quoted 3 lines
Out of curiosity, is there any major reason we don't use 0 here unconditionally? Are we worried about I/O completing before we have a chance to decrement the reference?
I think this should unconditionally avoid the schedule, and while we're at it we should kill _xfs_buf_ioend and opencode it here and at the other callsite.
And then remove the flag from xfs_buf_ioend which is always 0 at that point ...

Is it? xfs_buf_bio_end_io should stil be passing 1, the bio end_io handler is the place we really need the workqueue for anyway.

Quoted 3 lines

Yeah I have a patch to do that as well; I wanted to separate the bugfix from the more invasive cleanup, though - and I wanted to get the fix out for review sooner.

Sure, feel free to leave all the cleanups to another patch.

Quoted 3 lines

But yeah, I was unsure about whether or not to schedule at all here. We come here from a lot of callsites and I'm honestly not sure what the implications are yet.

I think the the delayed completion is always wrong from the submission path. The error path is just a special case of a completion happening before _xfs_buf_ioapply returns. The combination of incredibly fast hardware and bad preemption could cause the same bug you observed.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

author

2014-03-25 17:40 UTC #5 lore raw

Quoted 9 lines · Christoph Hellwig

Quoted 8 lines · Eric Sandeen
Quoted 7 lines
Quoted 3 lines
Out of curiosity, is there any major reason we don't use 0 here unconditionally? Are we worried about I/O completing before we have a chance to decrement the reference?
I think this should unconditionally avoid the schedule, and while we're at it we should kill _xfs_buf_ioend and opencode it here and at the other callsite.

Quoted 5 lines

Quoted 2 lines
And then remove the flag from xfs_buf_ioend which is always 0 at that point ...
Is it? xfs_buf_bio_end_io should stil be passing 1, the bio end_io handler is the place we really need the workqueue for anyway.

These are the callers of xfs_buf_ioend:

  File              Function                 Line
0 xfs_buf.c         xfs_bioerror             1085 xfs_buf_ioend(bp, 0);
1 xfs_buf.c         _xfs_buf_ioend           1177 xfs_buf_ioend(bp, schedule);
2 xfs_buf_item.c    xfs_buf_item_unpin        494 xfs_buf_ioend(bp, 0);
3 xfs_buf_item.c    xfs_buf_iodone_callbacks 1138 xfs_buf_ioend(bp, 0);
4 xfs_inode.c       xfs_iflush_cluster       3015 xfs_buf_ioend(bp, 0);
5 xfs_log.c         xlog_bdstrat             1644 xfs_buf_ioend(bp, 0);

6 xfs_log_recover.c xlog_recover_iodone 386 xfs_buf_ioend(bp, 0);

so only _xfs_buf_ioend *might* pass something other than 0, and:

File Function Line
0 xfs_buf.c xfs_buf_bio_end_io 1197 _xfs_buf_ioend(bp, 1);
1 xfs_buf.c xfs_buf_iorequest 1377 _xfs_buf_ioend(bp, bp->b_error ? 0 : 1);

At least up until now that was always called with "1"

Quoted 6 lines

Quoted 3 lines
Yeah I have a patch to do that as well; I wanted to separate the bugfix from the more invasive cleanup, though - and I wanted to get the fix out for review sooner.
Sure, feel free to leave all the cleanups to another patch.

Quoted 8 lines

Quoted 3 lines
But yeah, I was unsure about whether or not to schedule at all here. We come here from a lot of callsites and I'm honestly not sure what the implications are yet.
I think the the delayed completion is always wrong from the submission path. The error path is just a special case of a completion happening before _xfs_buf_ioapply returns. The combination of incredibly fast hardware and bad preemption could cause the same bug you observed.

I wondered about that.

I'm not sure; I don't think it was the buf_rele inside xfs_buf_iorequest that freed it, I think it was specifically the error path afterwards - in my case, in xfs_trans_read_buf_map():

                                xfs_buf_	(bp);
					// xfs_buf_iorequest code below
					xfs_buf_hold(bp);
					atomic_set(&bp->b_io_remaining, 1);
					_xfs_buf_ioapply(bp); <-- gets error
					if (atomic_dec_and_test(&bp->b_io_remaining)
						xfs_buf_ioend(bp, bp->b_error ? 0 : 1);
					xfs_buf_rele(bp); <- releases our hold
                        }

                        error = xfs_buf_iowait(bp); <-- sees error; would have waited otherwise
                        if (error) {
                                xfs_buf_ioerror_alert(bp, __func__);
                                xfs_buf_relse(bp); <--- freed here ?

but my bp refcounting & lifetime knowledge is lacking :(

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

2014-03-25 17:44 UTC #6 lore raw

Quoted 7 lines · Eric Sandeen

so only _xfs_buf_ioend *might* pass something other than 0, and:
File Function Line
0 xfs_buf.c xfs_buf_bio_end_io 1197 _xfs_buf_ioend(bp, 1);
1 xfs_buf.c xfs_buf_iorequest 1377 _xfs_buf_ioend(bp, bp->b_error ? 0 : 1);
At least up until now that was always called with "1"

Right, _was_. But that changes to one always passing 1, and one passing 0 or one with your patch. Or one passing always 1 and one always passing 0 with the suggestion from Brian and me. Either way we'd still have versions passing 1.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2014-03-25 18:08 UTC #7 lore raw

In reply to Christoph Hellwig

Quoted 14 lines · Christoph Hellwig

Quoted 5 lines · Eric Sandeen
Quoted 4 lines
fs/xfs/xfs_buf.c +4 -0
+	/*
+	 * If _xfs_buf_ioapply failed, we'll get back here with
+	 * only the reference we took above.  _xfs_buf_ioend will
+	 * drop it to zero, so we'd better not queue it for later,
Quoted 8 lines
fs/xfs/xfs_buf.c xfs_buf_iorequest( · line 1377
Quoted 4 lines
fs/xfs/xfs_buf.c +3 -0
+	 * or we'll free it before it's done.
+	 */
+	_xfs_buf_ioend(bp, bp->b_error ? 0 : 1);
Out of curiosity, is there any major reason we don't use 0 here unconditionally? Are we worried about I/O completing before we have a chance to decrement the reference?

Quoted 3 lines

I think this should unconditionally avoid the schedule, and while we're at it we should kill _xfs_buf_ioend and opencode it here and at the other callsite.

I thought we schduled here because we can issue IO from IO completion and so we need to requeue the IO completion rather than run it inline in the current IO completion that hasn't fully completed it's processing yet..

Quoted 2 lines

Also atomic_dec_and_test really just returns true/false - there should ne no need for the explicit == 1 in the conditional.

Right, there's several other cleanups around this code that can be done as Eric has mentioned...

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

2014-03-25 18:13 UTC #8 lore raw

Quoted 8 lines · Dave Chinner

Quoted 3 lines
I think this should unconditionally avoid the schedule, and while we're at it we should kill _xfs_buf_ioend and opencode it here and at the other callsite.
I thought we schduled here because we can issue IO from IO completion and so we need to requeue the IO completion rather than run it inline in the current IO completion that hasn't fully completed it's processing yet..

Actually we get a recursive callchain there from the I/O error handling. I switched from always 0 to always 1 here in:

commit 08023d6dbe840dc4271805a9ea376fcbdee9f744

Author: Christoph Hellwig <[email protected]>
Date:   Mon Jul 2 06:00:04 2012 -0400

xfs: prevent recursion in xfs_buf_iorequest

but I never got around fixing the root cause by cleaning up the way we handle buffer I/O completions on a shut down filesystem.

So I guess Eric's fix is the best we can do for now.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs