linux-kernel - Re: [PATCH] hfsplus: hfsplus_file_fsync() does not check for return value of sync_inode

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] [day] [month] [year] [list]

Message-ID: <1427130990.2874.13.camel@slavad-ubuntu-14.04>
Date:	Mon, 23 Mar 2015 10:16:30 -0700
From:	Viacheslav Dubeyko <slava@...eyko.com>
To:	Changwoo Min <changwoo.m@...il.com>
Cc:	viro@...iv.linux.org.uk, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, taesoo@...ech.edu,
	changwoo@...ech.edu, sanidhya@...ech.edu, blee@...ech.edu
Subject: Re: [PATCH] hfsplus: hfsplus_file_fsync() does not check for return
 value of sync_inode_metadata()

On Mon, 2015-03-23 at 11:33 -0400, Changwoo Min wrote:
> hfsplus_file_fsync() siliently ignores the return value of sync_inode_metadata(). 
> If an error occurs at sync_inode_metadata() and subsequent updates of other file
> system metadata (b-trees) succeed, file system metadata will be inconsistent. 
> 
> Signed-off-by: Changwoo Min <changwoo.m@...il.com>
> ---
>  fs/hfsplus/inode.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
> index 0cf786f..9444719 100644
> --- a/fs/hfsplus/inode.c
> +++ b/fs/hfsplus/inode.c
> @@ -286,7 +286,11 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
>  	/*
>  	 * Sync inode metadata into the catalog and extent trees.
>  	 */
> -	sync_inode_metadata(inode, 1);
> +	error = sync_inode_metadata(inode, 1);
> +	if (error) {
> +		mutex_unlock(&inode->i_mutex);
> +		return error;
> +	}

Thank you for suggestion. But I think that it's not proper fix.

Trying to break the flushing is not good idea. Even if we have some
troubles with sync_inode_metadata() call then it makes sense to flush
all other dirty pages. Of course, we will have corrupted file system but
to fix the corrupted state is responsibility of fsck tool. From my point
of view, to try to flush as many as possible dirty pages is good
strategy in the case of troubles during flushing. Because anyway we will
have corrupted file system.

Moreover, leaving memory pages in dirty state can hang writeback thread
in the case of improper processing of dirty pages on file system driver
in the case of likewise issue. I am not ready to say right now how
proper hfsplus driver will process "ignored" dirty memory pages in
writepage() call by writeback thread in the environment of such
solution.

Could you prove that your solution is right way? How it can be right
solution? Could you describe some issue and how this change fixes the
issue?

Thanks,
Vyacheslav Dubeyko.

>  
>  	/*
>  	 * And explicitly write out the btrees.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/