linux-ext4 - Re: [PATCH 2/2] ext3: Add journal error check into ext3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 22 Nov 2010 19:05:01 +0100
From:	Jan Kara <jack@...e.cz>
To:	Namhyung Kim <namhyung@...il.com>
Cc:	Jan Kara <jack@...e.cz>, Andrew Morton <akpm@...ux-foundation.org>,
	Andreas Dilger <adilger.kernel@...ger.ca>,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] ext3: Add journal error check into ext3_rename()

On Fri 19-11-10 16:28:36, Namhyung Kim wrote:
> Check return value of ext3_journal_get_write_access() and
> ext3_journal_dirty_metadata(). 'new_bh' should be kept in
> order to get revoked in case of journal error in dir_bh.
> 
> Signed-off-by: Namhyung Kim <namhyung@...il.com>
> ---
>  fs/ext3/namei.c |   27 +++++++++++++++++++++------
>  1 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
> index 672cea1..8061281 100644
> --- a/fs/ext3/namei.c
> +++ b/fs/ext3/namei.c
> @@ -2371,7 +2371,9 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
>  			goto end_rename;
>  	} else {
>  		BUFFER_TRACE(new_bh, "get write access");
> -		ext3_journal_get_write_access(handle, new_bh);
> +		retval = ext3_journal_get_write_access(handle, new_bh);
> +		if (retval)
> +			goto journal_error1;
>  		new_de->inode = cpu_to_le32(old_inode->i_ino);
>  		if (EXT3_HAS_INCOMPAT_FEATURE(new_dir->i_sb,
>  					      EXT3_FEATURE_INCOMPAT_FILETYPE))
> @@ -2380,9 +2382,12 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
>  		new_dir->i_ctime = new_dir->i_mtime = CURRENT_TIME_SEC;
>  		ext3_mark_inode_dirty(handle, new_dir);
>  		BUFFER_TRACE(new_bh, "call ext3_journal_dirty_metadata");
> -		ext3_journal_dirty_metadata(handle, new_bh);
> -		brelse(new_bh);
> -		new_bh = NULL;
> +		retval = ext3_journal_dirty_metadata(handle, new_bh);
> +		if (retval) {
> +journal_error1:
> +			ext3_std_error(new_dir->i_sb, retval);
> +			goto end_rename;
> +		}
>  	}
>  
>  	/*
> @@ -2429,10 +2434,20 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
>  	ext3_update_dx_flag(old_dir);
>  	if (dir_bh) {
>  		BUFFER_TRACE(dir_bh, "get_write_access");
> -		ext3_journal_get_write_access(handle, dir_bh);
> +		retval = ext3_journal_get_write_access(handle, dir_bh);
> +		if (retval)
> +			goto journal_error2;
>  		PARENT_INO(dir_bh->b_data) = cpu_to_le32(new_dir->i_ino);
>  		BUFFER_TRACE(dir_bh, "call ext3_journal_dirty_metadata");
> -		ext3_journal_dirty_metadata(handle, dir_bh);
> +		retval = ext3_journal_dirty_metadata(handle, dir_bh);
> +		if (retval) {
> +journal_error2:
> +			if (new_bh)
> +				ext3_journal_revoke(handle, new_bh->b_blocknr,
> +						    new_bh);
  Uhuh, why ext3_journal_revoke()? I expect you want to cancel the changes
you possibly did to new_bh. ext3_journal_forget() is for that but even that
doesn't necessarily do what you want because it could cancel also changes
some unrelated operation did to the buffer. So the only way to really undo
the change is to set new_de->inode and new_de->file_type to original
values. Also since we already unlinked the inode from the old directory,
I'm not sure it's even beneficial to undo linking it to the new one. So I'd
just bail out as fast as we can and leave on fsck to handle the mess...

								Honza

> +			ext3_std_error(new_dir->i_sb, retval);
> +			goto end_rename;
> +		}
>  		drop_nlink(old_dir);
>  		if (new_inode) {
>  			drop_nlink(new_inode);
> -- 
> 1.7.0.4
> 
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html