[PATCH] ext3: fix use-after-free bug

ext3_writepage() calls ext3_journal_stop(), which dereferences the affected
inode.

It does this _after_ writing the page out, which is illegal.  The IO can
complete, the page can be repeased from the inode and the inode can be freed
up.

It's a long-standing bug.  It has been reported happening on preemptible
kernels, where the timing window is larger.

Fix that up by teaching ext3_journal_stop to locate the superblock via the
journal structure, not via the inode.

This means that ext3_journal_stop() does not need the inode argument at all.

Also uninline the affected functions.  It saves 5.5 kbytes.

Also remove the setting of sb->s_dirt in ext3_journal_stop().  That was an
awkward way of telling sys_sync() that the filesystem needs a commit, and
with the ext3_sync_fs() that is no longer needed.

fs/ext3/acl.c

@@ -420,7 +420,7 @@ ext3_acl_chmod(struct inode *inode)
 			return PTR_ERR(handle);
 		}
 		error = ext3_set_acl(handle, inode, ACL_TYPE_ACCESS, clone);
-		ext3_journal_stop(handle, inode);
+		ext3_journal_stop(handle);
 	}
 	posix_acl_release(clone);
 	return error;
@@ -522,7 +522,7 @@ ext3_xattr_set_acl(struct inode *inode, int type, const void *value,
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
 	error = ext3_set_acl(handle, inode, type, acl);
-	ext3_journal_stop(handle, inode);
+	ext3_journal_stop(handle);
 
 release_and_out:
 	posix_acl_release(acl);

fs/ext3/inode.c

@@ -235,7 +235,7 @@ void ext3_delete_inode (struct inode * inode)
 		clear_inode(inode);
 	else
 		ext3_free_inode(handle, inode);
-	ext3_journal_stop(handle, inode);
+	ext3_journal_stop(handle);
 	unlock_kernel();
 	return;
 no_delete:
@@ -1107,7 +1107,7 @@ static int ext3_prepare_write(struct file *file, struct page *page,
 	}
 prepare_write_failed:
 	if (ret)
-		ext3_journal_stop(handle, inode);
+		ext3_journal_stop(handle);
 out:
 	unlock_kernel();
 	return ret;
@@ -1176,7 +1176,7 @@ static int ext3_commit_write(struct file *file, struct page *page,
 		if (!ret) 
 			ret = ret2;
 	}
-	ret2 = ext3_journal_stop(handle, inode);
+	ret2 = ext3_journal_stop(handle);
 	unlock_kernel();
 	if (!ret)
 		ret = ret2;
@@ -1374,7 +1374,7 @@ static int ext3_writepage(struct page *page, struct writeback_control *wbc)
 				PAGE_CACHE_SIZE, NULL, bput_one);
 	}
 
-	err = ext3_journal_stop(handle, inode);
+	err = ext3_journal_stop(handle);
 	if (!ret)
 		ret = err;
 	unlock_kernel();
@@ -1479,7 +1479,7 @@ static int ext3_direct_IO(int rw, struct kiocb *iocb,
 					ret = err;
 			}
 		}
-		err = ext3_journal_stop(handle, inode);
+		err = ext3_journal_stop(handle);
 		if (ret == 0)
 			ret = err;
 		unlock_kernel();
@@ -2140,7 +2140,7 @@ void ext3_truncate(struct inode * inode)
 	if (inode->i_nlink)
 		ext3_orphan_del(handle, inode);
 
-	ext3_journal_stop(handle, inode);
+	ext3_journal_stop(handle);
 	unlock_kernel();
 }
 
@@ -2560,7 +2560,7 @@ int ext3_setattr(struct dentry *dentry, struct iattr *attr)
 		rc = ext3_mark_inode_dirty(handle, inode);
 		if (!error)
 			error = rc;
-		ext3_journal_stop(handle, inode);
+		ext3_journal_stop(handle);
 	}
 	
 	rc = inode_setattr(inode, attr);
@@ -2737,7 +2737,7 @@ void ext3_dirty_inode(struct inode *inode)
 				current_handle);
 		ext3_mark_inode_dirty(handle, inode);
 	}
-	ext3_journal_stop(handle, inode);
+	ext3_journal_stop(handle);
 out:
 	unlock_kernel();
 }
@@ -2818,7 +2818,7 @@ int ext3_change_inode_journal_flag(struct inode *inode, int val)
 
 	err = ext3_mark_inode_dirty(handle, inode);
 	handle->h_sync = 1;
-	ext3_journal_stop(handle, inode);
+	ext3_journal_stop(handle);
 	ext3_std_error(inode->i_sb, err);
 	
 	return err;

fs/ext3/ioctl.c

@@ -90,7 +90,7 @@ int ext3_ioctl (struct inode * inode, struct file * filp, unsigned int cmd,
 
 		err = ext3_mark_iloc_dirty(handle, inode, &iloc);
 flags_err:
-		ext3_journal_stop(handle, inode);
+		ext3_journal_stop(handle);
 		if (err)
 			return err;
 		
@@ -126,7 +126,7 @@ int ext3_ioctl (struct inode * inode, struct file * filp, unsigned int cmd,
 		inode->i_generation = generation;
 
 		err = ext3_mark_iloc_dirty(handle, inode, &iloc);
-		ext3_journal_stop(handle, inode);
+		ext3_journal_stop(handle);
 		return err;
 	}
 #ifdef CONFIG_JBD_DEBUG

fs/ext3/namei.c

@@ -1615,7 +1615,7 @@ static int ext3_create (struct inode * dir, struct dentry * dentry, int mode)
 			inode->i_mapping->a_ops = &ext3_aops;
 		err = ext3_add_nondir(handle, dentry, inode);
 	}
-	ext3_journal_stop(handle, dir);
+	ext3_journal_stop(handle);
 	unlock_kernel();
 	return err;
 }
@@ -1647,7 +1647,7 @@ static int ext3_mknod (struct inode * dir, struct dentry *dentry,
 #endif
 		err = ext3_add_nondir(handle, dentry, inode);
 	}
-	ext3_journal_stop(handle, dir);
+	ext3_journal_stop(handle);
 	unlock_kernel();
 	return err;
 }
@@ -1721,7 +1721,7 @@ static int ext3_mkdir(struct inode * dir, struct dentry * dentry, int mode)
 	ext3_mark_inode_dirty(handle, dir);
 	d_instantiate(dentry, inode);
 out_stop:
-	ext3_journal_stop(handle, dir);
+	ext3_journal_stop(handle);
 	unlock_kernel();
 	return err;
 }
@@ -1994,7 +1994,7 @@ static int ext3_rmdir (struct inode * dir, struct dentry *dentry)
 	ext3_mark_inode_dirty(handle, dir);
 
 end_rmdir:
-	ext3_journal_stop(handle, dir);
+	ext3_journal_stop(handle);
 	brelse (bh);
 	unlock_kernel();
 	return retval;
@@ -2050,7 +2050,7 @@ static int ext3_unlink(struct inode * dir, struct dentry *dentry)
 	retval = 0;
 
 end_unlink:
-	ext3_journal_stop(handle, dir);
+	ext3_journal_stop(handle);
 	unlock_kernel();
 	brelse (bh);
 	return retval;
@@ -2109,7 +2109,7 @@ static int ext3_symlink (struct inode * dir,
 	EXT3_I(inode)->i_disksize = inode->i_size;
 	err = ext3_add_nondir(handle, dentry, inode);
 out_stop:
-	ext3_journal_stop(handle, dir);
+	ext3_journal_stop(handle);
 	unlock_kernel();
 	return err;
 }
@@ -2142,7 +2142,7 @@ static int ext3_link (struct dentry * old_dentry,
 	atomic_inc(&inode->i_count);
 
 	err = ext3_add_nondir(handle, dentry, inode);
-	ext3_journal_stop(handle, dir);
+	ext3_journal_stop(handle);
 	unlock_kernel();
 	return err;
 }
@@ -2299,7 +2299,7 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
 	brelse (dir_bh);
 	brelse (old_bh);
 	brelse (new_bh);
-	ext3_journal_stop(handle, old_dir);
+	ext3_journal_stop(handle);
 	unlock_kernel();
 	return retval;
 }

fs/ext3/super.c

@@ -108,6 +108,72 @@ static void clear_ro_after(struct super_block *sb)
 #define clear_ro_after(sb)	do {} while (0)
 #endif
 
+/* 
+ * Wrappers for journal_start/end.
+ *
+ * The only special thing we need to do here is to make sure that all
+ * journal_end calls result in the superblock being marked dirty, so
+ * that sync() will call the filesystem's write_super callback if
+ * appropriate. 
+ */
+handle_t *ext3_journal_start(struct inode *inode, int nblocks)
+{
+	journal_t *journal;
+	
+	if (inode->i_sb->s_flags & MS_RDONLY)
+		return ERR_PTR(-EROFS);
+
+	/* Special case here: if the journal has aborted behind our
+	 * backs (eg. EIO in the commit thread), then we still need to
+	 * take the FS itself readonly cleanly. */
+	journal = EXT3_JOURNAL(inode);
+	if (is_journal_aborted(journal)) {
+		ext3_abort(inode->i_sb, __FUNCTION__,
+			   "Detected aborted journal");
+		return ERR_PTR(-EROFS);
+	}
+	
+	return journal_start(journal, nblocks);
+}
+
+/* 
+ * The only special thing we need to do here is to make sure that all
+ * journal_stop calls result in the superblock being marked dirty, so
+ * that sync() will call the filesystem's write_super callback if
+ * appropriate. 
+ */
+int __ext3_journal_stop(const char *where, handle_t *handle)
+{
+	struct super_block *sb;
+	int err;
+	int rc;
+
+	sb = handle->h_transaction->t_journal->j_private;
+	err = handle->h_err;
+	rc = journal_stop(handle);
+
+	if (!err)
+		err = rc;
+	if (err)
+		__ext3_std_error(sb, where, err);
+	return err;
+}
+
+void ext3_journal_abort_handle(const char *caller, const char *err_fn,
+		struct buffer_head *bh, handle_t *handle, int err)
+{
+	char nbuf[16];
+	const char *errstr = ext3_decode_error(NULL, err, nbuf);
+	
+	printk(KERN_ERR "%s: aborting transaction: %s in %s", 
+	       caller, errstr, err_fn);
+
+	if (bh)
+		BUFFER_TRACE(bh, "abort");
+	journal_abort_handle(handle);
+	if (!handle->h_err)
+		handle->h_err = err;
+}
 
 static char error_buf[1024];
 
@@ -1431,6 +1497,7 @@ static journal_t *ext3_get_journal(struct super_block *sb, int journal_inum)
 		printk(KERN_ERR "EXT3-fs: Could not load journal inode\n");
 		iput(journal_inode);
 	}
+	journal->j_private = sb;
 	ext3_init_journal_params(EXT3_SB(sb), journal);
 	return journal;
 }
@@ -1495,6 +1562,7 @@ static journal_t *ext3_get_dev_journal(struct super_block *sb,
 		printk(KERN_ERR "EXT3-fs: failed to create device journal\n");
 		goto out_bdev;
 	}
+	journal->j_private = sb;
 	ll_rw_block(READ, 1, &journal->j_sb_buffer);
 	wait_on_buffer(journal->j_sb_buffer);
 	if (!buffer_uptodate(journal->j_sb_buffer)) {
@@ -1561,7 +1629,6 @@ static int ext3_load_journal(struct super_block * sb,
 		if (!(journal = ext3_get_dev_journal(sb, journal_dev)))
 			return -EINVAL;
 	}
-	
 
 	if (!really_read_only && test_opt(sb, UPDATE_JOURNAL)) {
 		err = journal_update_format(journal);

fs/ext3/xattr.c

@@ -855,7 +855,7 @@ ext3_xattr_set(struct inode *inode, int name_index, const char *name,
 	else
 		error = ext3_xattr_set_handle(handle, inode, name_index, name,
 					      value, value_len, flags);
-	error2 = ext3_journal_stop(handle, inode);
+	error2 = ext3_journal_stop(handle);
 	unlock_kernel();
 
 	return error ? error : error2;

include/linux/ext3_jbd.h

@@ -93,24 +93,8 @@ int ext3_mark_inode_dirty(handle_t *handle, struct inode *inode);
  * been done yet.
  */
 
-static inline void ext3_journal_abort_handle(const char *caller, 
-					     const char *err_fn,
-					     struct buffer_head *bh,
-					     handle_t *handle,
-					     int err)
-{
-	char nbuf[16];
-	const char *errstr = ext3_decode_error(NULL, err, nbuf);
-	
-	printk(KERN_ERR "%s: aborting transaction: %s in %s", 
-	       caller, errstr, err_fn);
-
-	if (bh)
-		BUFFER_TRACE(bh, "abort");
-	journal_abort_handle(handle);
-	if (!handle->h_err)
-		handle->h_err = err;
-}
+void ext3_journal_abort_handle(const char *caller, const char *err_fn,
+		struct buffer_head *bh, handle_t *handle, int err);
 
 static inline int
 __ext3_journal_get_undo_access(const char *where,
@@ -180,57 +164,11 @@ __ext3_journal_dirty_metadata(const char *where,
 #define ext3_journal_dirty_metadata(handle, bh) \
 	__ext3_journal_dirty_metadata(__FUNCTION__, (handle), (bh))
 
+handle_t *ext3_journal_start(struct inode *inode, int nblocks);
+int __ext3_journal_stop(const char *where, handle_t *handle);
 
-
-/* 
- * Wrappers for journal_start/end.
- *
- * The only special thing we need to do here is to make sure that all
- * journal_end calls result in the superblock being marked dirty, so
- * that sync() will call the filesystem's write_super callback if
- * appropriate. 
- */
-static inline handle_t *ext3_journal_start(struct inode *inode, int nblocks)
-{
-	journal_t *journal;
-	
-	if (inode->i_sb->s_flags & MS_RDONLY)
-		return ERR_PTR(-EROFS);
-
-	/* Special case here: if the journal has aborted behind our
-	 * backs (eg. EIO in the commit thread), then we still need to
-	 * take the FS itself readonly cleanly. */
-	journal = EXT3_JOURNAL(inode);
-	if (is_journal_aborted(journal)) {
-		ext3_abort(inode->i_sb, __FUNCTION__,
-			   "Detected aborted journal");
-		return ERR_PTR(-EROFS);
-	}
-	
-	return journal_start(journal, nblocks);
-}
-
-/* 
- * The only special thing we need to do here is to make sure that all
- * journal_stop calls result in the superblock being marked dirty, so
- * that sync() will call the filesystem's write_super callback if
- * appropriate. 
- */
-static inline int __ext3_journal_stop(const char *where,
-				      handle_t *handle, struct inode *inode)
-{
-	int err = handle->h_err;
-	int rc = journal_stop(handle);
-
-	inode->i_sb->s_dirt = 1;
-	if (!err)
-		err = rc;
-	if (err)
-		__ext3_std_error(inode->i_sb, where, err);
-	return err;
-}
-#define ext3_journal_stop(handle, inode) \
-	__ext3_journal_stop(__FUNCTION__, (handle), (inode))
+#define ext3_journal_stop(handle) \
+	__ext3_journal_stop(__FUNCTION__, (handle))
 
 static inline handle_t *ext3_journal_current_handle(void)
 {

include/linux/jbd.h

@@ -633,6 +633,10 @@ struct journal_s
 	/* The revoke table: maintains the list of revoked blocks in the */
         /*  current transaction. */
 	struct jbd_revoke_table_s *j_revoke;
+
+	/* An opaque pointer to fs-private information.  ext3 puts its
+	 * superblock pointer here */
+	void *j_private;
 };
 
 /*