review: fix use after free of log buffers on shutdown.

review: fix use after free of log buffers on shutdown.

[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.

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.
-- 
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 */
 
 /*

Re: review: fix use after free of log buffers on shutdown.

[Christoph Hellwig]

On Thu, Apr 19, 2007 at 05:53:38PM +1000, David Chinner wrote:
> 
> 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.

Looks good to me.

> 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 */
>  
>  /*
> 
> 
---end quoted text---