linux-kernel - Re: [patch 7/8] 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 for Android: free password hash cracker in your pocket

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

Date:	Mon, 3 Jan 2011 11:58:21 -0500
From:	Christoph Hellwig <hch@...radead.org>
To:	Steven Whitehouse <swhiteho@...hat.com>
Cc:	Christoph Hellwig <hch@...radead.org>,
	Nick Piggin <npiggin@...nel.dk>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>, mfasheh@...e.com,
	joel.becker@...cle.com
Subject: Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in filesystems

On Mon, Jan 03, 2011 at 03:03:29PM +0000, Steven Whitehouse wrote:
> 
>  - With "journaled data" files
>    - Do a log flush conditional upon the inode's glock
>    - The core code then writes back any dirty pages

Any data writeback is done before calling ->fsync.

>  - With regular files/directories
>   - If datasync is not set, we need to write back the metadata including
> timestamp updates, so that is done via ->write_inode. Note that an extra
> complication here is that we need to get the glock on the inode if we
> don't already have it in order to check and conditionally update the
> atime.The call to ->write_inode includes an implicit (conditional) log
> flush.
>  - If datasync is set, we assume that only the data pages need to be
> written out. My understanding of datasync was that it was only supposed
> to write out data and never any of the metadata. The reason for the call
> to flush the log for "stuffed" files is that the data shares a disk
> block with the inode metadata, so we cannot avoid the log flush in this
> case, since we must unpin the block to write it back.

What happens to indirect blocks, inode size updates, etc?  In general
the only correct form to use the datasync argument is along the lines
of:

	if ((inode->i_state & I_DIRTY_DATASYNC) ||
	    ((inode->i_state & I_DIRTY_SYNC) && !datasync)) {
		/* write out the inode */
	} else {
		/*
		 * VFS inode not dirty, no need to write it out.
		 *
		 * If the filesystem support asynchronous inode writes,
		 * we may have to wait for them here.
		 */
	}

or rather mostly correct, as pointed out by Nick in this series, that's
why the above gets replaced with an equivalent check that also
participates in the writeback locking protocol in this series.

For gfs2 on current mainline an fsync respecting that would look like:

static int gfs2_fsync(struct file *file, int datasync)
{
	struct inode *inode = file->f_mapping->host;
	struct gfs2_inode *ip = GFS2_I(inode);
	int ret = 0;

	if (gfs2_is_jdata(ip) {
		gfs2_log_flush(GFS2_SB(inode), ip);
		return 0;
	}

	if ((inode->i_state & I_DIRTY_DATASYNC) ||
	    ((inode->i_state & I_DIRTY_SYNC) && !datasync))
		sync_inode_metadata(inode, 1);
	else if (gfs2_is_stuffed(ip))
		gfs2_log_flush(GFS2_SB(inode), ip->i_gl);
}

Note that the asynchronous write_inode_now is replaced with a
sync_inode_metadata, which doesn't incorrectly write data again, and
makes sure we do a synchronous write.

I'm still not quite sure how the gfs2_log_flush are supposed to work.
What's the reason we don't need the ->write_inode call for journaled
data mode?  Also is it guaranteed that we might not have an asynchronous
transaction that update the inode in the log, e.g. why doesn't gfs2
need some sort of log flush even if the VFS inode is not dirty, unlike
most other journaled filesystems.

--
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/