review: fix use after free of log buffers on shutdown.
When unmounting the filesystem we write an unmount record into the log just before we start freeing up in memory structures.
When we wait for the unmount record to hit the disk, we don't wait for the log buffers to be finished with, we only wait for part of the iodone callback to be run - the bit that processes the unmount record completion.
Hence when the unmount wakes up, it races with the remainder of the log io completion and pretty much the first thing it does is free the log buffers.
As a result, when iodone processing completes and we check the buffer's async status, the buffer can already have been freed.
Luckily, all log I/O is issued asynchronously, so we don't really need the async check and so we can avoid this use after free easily.
Cheers,
Dave.
Signature · 37 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
---
fs/xfs/xfs_log.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
Index: 2.6.x-xfs-new/fs/xfs/xfs_log.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_log.c 2007-04-19 17:18:14.097380099 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_log.c 2007-04-19 17:51:03.017078512 +1000
@@ -988,14 +988,16 @@ xlog_iodone(xfs_buf_t *bp)
} else if (iclog->ic_state & XLOG_STATE_IOERROR) {
aborted = XFS_LI_ABORTED;
}
+
+ /* log I/O is always issued ASYNC */
+ ASSERT(XFS_BUF_ISASYNC(bp));
xlog_state_done_syncing(iclog, aborted);
- if (!(XFS_BUF_ISASYNC(bp))) {
- /*
- * Corresponding psema() will be done in bwrite(). If we don't
- * vsema() here, panic.
- */
- XFS_BUF_V_IODONESEMA(bp);
- }
+ /*
+ * do not reference the buffer (bp) here as we could race
+ * with it being freed after writing the unmount record to the
+ * log.
+ */
+
} /* xlog_iodone */
/*Quoted 13 lines · David Chinner
When unmounting the filesystem we write an unmount record into the log just before we start freeing up in memory structures.
When we wait for the unmount record to hit the disk, we don't wait for the log buffers to be finished with, we only wait for part of the iodone callback to be run - the bit that processes the unmount record completion.
Hence when the unmount wakes up, it races with the remainder of the log io completion and pretty much the first thing it does is free the log buffers.
Quoted 6 lines
As a result, when iodone processing completes and we check the buffer's async status, the buffer can already have been freed.
Luckily, all log I/O is issued asynchronously, so we don't really need the async check and so we can avoid this use after free easily.
Looks good to me.
Quoted 20 lines
Index:
2.6.x-xfs-new/fs/xfs/xfs_log.c
===================================================================fs/xfs/xfs_log.c +3
-6@@ -988,14 +988,16 @@ xlog_iodone(xfs_buf_t *bp) }else if (iclog->ic_state &XLOG_STATE_IOERROR ) { aborted =XFS_LI_ABORTED ; } + +/* log I/O is always issued ASYNC */ +ASSERT (XFS_BUF_ISASYNC (bp));xlog_state_done_syncing (iclog, aborted); -if (!(XFS_BUF_ISASYNC (bp))) { -/* -* Corresponding psema() will be done in bwrite(). If we don't -* vsema() here, panic. -*/ -XFS_BUF_V_IODONESEMA (bp);
fs/xfs/xfs_log.c xlog_iodone(xfs_buf_t *bp) · line 1003
Quoted 10 lines
fs/xfs/xfs_log.c +6
-1- } +/* +* do not reference the buffer (bp) here as we could race +* with it being freed after writing the unmount record to the +* log. +*/ + }/* xlog_iodone */ /*
---end quoted text---