linux-kernel - Re: [patch 7/7] fs: fix or note I_DIRTY handling bugs in filesystems

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]

Message-ID: <20101123225148.GZ22876@dastard>
Date:	Wed, 24 Nov 2010 09:51:48 +1100
From:	Dave Chinner <david@...morbit.com>
To:	npiggin@...nel.dk
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [patch 7/7] fs: fix or note I_DIRTY handling bugs in
 filesystems

On Wed, Nov 24, 2010 at 01:06:17AM +1100, npiggin@...nel.dk wrote:
> Comments?

How did you test the changes?

> +++ linux-2.6/fs/xfs/linux-2.6/xfs_file.c	2010-11-24 00:08:03.000000000 +1100
> @@ -99,6 +99,7 @@ xfs_file_fsync(
>  	struct xfs_trans	*tp;
>  	int			error = 0;
>  	int			log_flushed = 0;
> +	unsigned		dirty, mask;
>  
>  	trace_xfs_file_fsync(ip);
>  
> @@ -132,9 +133,16 @@ xfs_file_fsync(
>  	 * might gets cleared when the inode gets written out via the AIL
>  	 * or xfs_iflush_cluster.
>  	 */
> -	if (((inode->i_state & I_DIRTY_DATASYNC) ||
> -	    ((inode->i_state & I_DIRTY_SYNC) && !datasync)) &&
> -	    ip->i_update_core) {
> +	spin_lock(&inode_lock);
> +	inode_writeback_begin(inode, 1);
> +	if (datasync)
> +		mask = I_DIRTY_DATASYNC;
> +	else
> +		mask = I_DIRTY_SYNC | I_DIRTY_DATASYNC;
> +	dirty = inode->i_state & mask;
> +	inode->i_state &= ~mask;
> +	spin_unlock(&inode_lock);
> +	if (dirty && ip->i_update_core) {

It looks to me like the pattern "inode_writeback_begin(); get dirty
state from i_state" repeated for each filesystem is wrong. The
inode_writeback_begin() helper does this:

	inode->i_state &= ~I_DIRTY;

which clears all the dirty bits from the i_state, which means the
followup:

	dirty = inode->i_state & mask;

will always result in a zero value for dirty.  IOWs, this seems to
ensure that ->fsync never sees dirty inodes anymore. This will break
fsync on XFS, and probably on all the other filesystems you modified
to use this pattern as well.

Also, I think the pattern is racy with respect to concurrent page
cache dirtiers. i.e if the inode was dirtied between writeback and
->fsync() in vfs_fsync_range(), then this new code clears the
I_DIRTY_PAGES bit in i_state without writing back the dirty pages.

And FWIW, I'm not sure that we want to be propagating the inode_lock
into every filesystem...

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com
--
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/