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 | 4 +- fs/ext3/inode.c | 18 +++++------ fs/ext3/ioctl.c | 4 +- fs/ext3/namei.c | 16 +++++----- fs/ext3/super.c | 69 +++++++++++++++++++++++++++++++++++++++++++ fs/ext3/xattr.c | 2 - include/linux/ext3_jbd.h | 74 +++-------------------------------------------- include/linux/jbd.h | 4 ++ 8 files changed, 100 insertions(+), 91 deletions(-) diff -puN include/linux/ext3_jbd.h~ext3_writepage-use-after-free-fix include/linux/ext3_jbd.h --- 25/include/linux/ext3_jbd.h~ext3_writepage-use-after-free-fix 2003-03-20 18:25:39.000000000 -0800 +++ 25-akpm/include/linux/ext3_jbd.h 2003-03-20 18:25:39.000000000 -0800 @@ -93,24 +93,8 @@ int ext3_mark_inode_dirty(handle_t *hand * 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 #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) { diff -puN include/linux/jbd.h~ext3_writepage-use-after-free-fix include/linux/jbd.h --- 25/include/linux/jbd.h~ext3_writepage-use-after-free-fix 2003-03-20 18:25:39.000000000 -0800 +++ 25-akpm/include/linux/jbd.h 2003-03-20 18:25:39.000000000 -0800 @@ -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; }; /* diff -puN fs/ext3/acl.c~ext3_writepage-use-after-free-fix fs/ext3/acl.c --- 25/fs/ext3/acl.c~ext3_writepage-use-after-free-fix 2003-03-20 18:25:39.000000000 -0800 +++ 25-akpm/fs/ext3/acl.c 2003-03-20 18:25:39.000000000 -0800 @@ -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, 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); diff -puN fs/ext3/inode.c~ext3_writepage-use-after-free-fix fs/ext3/inode.c --- 25/fs/ext3/inode.c~ext3_writepage-use-after-free-fix 2003-03-20 18:25:39.000000000 -0800 +++ 25-akpm/fs/ext3/inode.c 2003-03-20 18:25:39.000000000 -0800 @@ -235,7 +235,7 @@ void ext3_delete_inode (struct inode * i 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 fil } 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 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 *p 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 @@ out_stop: ret = err; } } - err = ext3_journal_stop(handle, inode); + err = ext3_journal_stop(handle); if (ret == 0) ret = err; unlock_kernel(); @@ -2140,7 +2140,7 @@ out_stop: 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, 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 *inod 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(struc 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; diff -puN fs/ext3/ioctl.c~ext3_writepage-use-after-free-fix fs/ext3/ioctl.c --- 25/fs/ext3/ioctl.c~ext3_writepage-use-after-free-fix 2003-03-20 18:25:39.000000000 -0800 +++ 25-akpm/fs/ext3/ioctl.c 2003-03-20 18:25:39.000000000 -0800 @@ -90,7 +90,7 @@ int ext3_ioctl (struct inode * inode, st 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 @@ flags_err: 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 diff -puN fs/ext3/namei.c~ext3_writepage-use-after-free-fix fs/ext3/namei.c --- 25/fs/ext3/namei.c~ext3_writepage-use-after-free-fix 2003-03-20 18:25:39.000000000 -0800 +++ 25-akpm/fs/ext3/namei.c 2003-03-20 18:25:39.000000000 -0800 @@ -1661,7 +1661,7 @@ static int ext3_create (struct inode * d 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; } @@ -1693,7 +1693,7 @@ static int ext3_mknod (struct inode * di #endif err = ext3_add_nondir(handle, dentry, inode); } - ext3_journal_stop(handle, dir); + ext3_journal_stop(handle); unlock_kernel(); return err; } @@ -1767,7 +1767,7 @@ static int ext3_mkdir(struct inode * dir 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; } @@ -2040,7 +2040,7 @@ static int ext3_rmdir (struct inode * di ext3_mark_inode_dirty(handle, dir); end_rmdir: - ext3_journal_stop(handle, dir); + ext3_journal_stop(handle); brelse (bh); unlock_kernel(); return retval; @@ -2096,7 +2096,7 @@ static int ext3_unlink(struct inode * di retval = 0; end_unlink: - ext3_journal_stop(handle, dir); + ext3_journal_stop(handle); unlock_kernel(); brelse (bh); return retval; @@ -2155,7 +2155,7 @@ static int ext3_symlink (struct inode * 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; } @@ -2188,7 +2188,7 @@ static int ext3_link (struct dentry * ol 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; } @@ -2345,7 +2345,7 @@ end_rename: brelse (dir_bh); brelse (old_bh); brelse (new_bh); - ext3_journal_stop(handle, old_dir); + ext3_journal_stop(handle); unlock_kernel(); return retval; } diff -puN fs/ext3/xattr.c~ext3_writepage-use-after-free-fix fs/ext3/xattr.c --- 25/fs/ext3/xattr.c~ext3_writepage-use-after-free-fix 2003-03-20 18:25:39.000000000 -0800 +++ 25-akpm/fs/ext3/xattr.c 2003-03-20 18:25:39.000000000 -0800 @@ -854,7 +854,7 @@ ext3_xattr_set(struct inode *inode, int 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; diff -puN fs/ext3/super.c~ext3_writepage-use-after-free-fix fs/ext3/super.c --- 25/fs/ext3/super.c~ext3_writepage-use-after-free-fix 2003-03-20 18:25:39.000000000 -0800 +++ 25-akpm/fs/ext3/super.c 2003-03-20 18:25:39.000000000 -0800 @@ -106,6 +106,72 @@ static void clear_ro_after(struct super_ #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]; @@ -1426,6 +1492,7 @@ static journal_t *ext3_get_journal(struc 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; } @@ -1490,6 +1557,7 @@ static journal_t *ext3_get_dev_journal(s 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)) { @@ -1556,7 +1624,6 @@ static int ext3_load_journal(struct supe 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); _