linux-kernel - [PATCH][RESEND] fs: always set I_DIRTY_TIME to fsync correctly on lazytime

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

Message-Id: <20161031190245.13404-1-naota@elisp.net>
Date:   Tue,  1 Nov 2016 04:02:45 +0900
From:   Naohiro Aota <naota@...sp.net>
To:     linux-fsdevel@...r.kernel.org
Cc:     viro@...iv.linux.org.uk, linux-kernel@...r.kernel.org,
        tytso@....edu, asraaiteng@...il.com, Naohiro Aota <naota@...sp.net>
Subject: [PATCH][RESEND] fs: always set I_DIRTY_TIME to fsync correctly on lazytime

While lazytime states that "The on-disk timestamps are updated only
when: ... - the application employs fsync(2), syncfs(2), or sync(2)"
[1], it does not write a timestamp update on fsync().

[1] http://manpages.ubuntu.com/manpages/xenial/man8/mount.8.html

The following commands will reproduce the problem:

$ mount -o noatime,lazytime ext4.img /mnt/tmp
$ cd /mnt/tmp
(create an 128M file to fio, not to observe size update)
$ dd if=/dev/zero of=wxyz.0.0 bs=1M count=128
(do write/fsync)
$ fio --name wxyz --direct=1 --buffered=0 --size=128m --bs=64k --rw=write \
  --ioengine=sync --numjobs=1 --fsync=5

Since fio invokes 1 fsync per 5 writes, we should see rapid journal
commits for timestamp update by tracing jbd2:jbd2_end_commit trace
point. Only we can see are, however, some periodic (~5 sec) commits from
bdi flush like below.

$ trace jbd2:jbd2_end_commit
    jbd2/loop0-8-1617  [002] ....    96.637351: jbd2_end_commit: dev 7,0 transaction 5393 sync 0 head 5393
    jbd2/loop0-8-1617  [000] ....   101.679411: jbd2_end_commit: dev 7,0 transaction 5394 sync 0 head 5393
    jbd2/loop0-8-1617  [003] ....   106.743628: jbd2_end_commit: dev 7,0 transaction 5395 sync 0 head 5393
    jbd2/loop0-8-1617  [001] ....   111.801964: jbd2_end_commit: dev 7,0 transaction 5396 sync 0 head 5393
...

The problem is __mark_inode_dirty() does not always flag I_DIRTY_TIME.
It seems that it is no use to mark an inode I_DIRTY_TIME when the inode
is already I_DIRTY_INODE. However, by that decision, we're skipping
journal write if we invoke two fsync()s between two bdi flushes. As the
following table shows, any fsync after the first fsync do nothing (if
there's no update other than timestamp).

Event                | i_state      | journal
---------------------+--------------+------------------------
<timestamp update>   | I_DIRTY_TIME | no write (lazytime)
<fsync>              | I_DIRTY_SYNC | write timestamp update
<timestamp update>   | I_DIRTY_SYNC | no write (lazytime)
<fsync>              | I_DIRTY_SYNC | no write *BUG*
...
<bdi flush>          | 0            |
<timestamp update>   | I_DIRTY_TIME | no write (lazytime)
<fsync>              | I_DIRTY_SYNC | write timestamp update

We should set I_DIRTY_TIME on the second timestamp update to let fsync()
notice there's a timestamp update after the last inode writeout.

After this patch, we can see rapid trace of journal commit:
$ trace jbd2:jbd2_end_commit
    jbd2/loop0-8-1879  [002] ....   208.275057: jbd2_end_commit: dev 7,0 transaction 5364 sync 0 head 3343
    jbd2/loop0-8-1879  [000] ....   208.302539: jbd2_end_commit: dev 7,0 transaction 5365 sync 0 head 3343
    jbd2/loop0-8-1879  [000] ....   208.327238: jbd2_end_commit: dev 7,0 transaction 5366 sync 0 head 3343
    jbd2/loop0-8-1879  [003] ....   208.347618: jbd2_end_commit: dev 7,0 transaction 5367 sync 0 head 3343
...

Reported-by: Asraa Ali Mardan <asraaiteng@...il.com>
Signed-off-by: Naohiro Aota <naota@...sp.net>
---

 fs/fs-writeback.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 05713a5..ace628c 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -2100,16 +2100,17 @@ void __mark_inode_dirty(struct inode *inode, int flags)
 	 */
 	smp_mb();
 
-	if (((inode->i_state & flags) == flags) ||
-	    (dirtytime && (inode->i_state & I_DIRTY_INODE)))
+	if ((inode->i_state & flags) == flags)
 		return;
 
 	if (unlikely(block_dump))
 		block_dump___mark_inode_dirty(inode);
 
 	spin_lock(&inode->i_lock);
-	if (dirtytime && (inode->i_state & I_DIRTY_INODE))
+	if (dirtytime && (inode->i_state & I_DIRTY_INODE)) {
+		inode->i_state |= I_DIRTY_TIME;
 		goto out_unlock_inode;
+	}
 	if ((inode->i_state & flags) != flags) {
 		const int was_dirty = inode->i_state & I_DIRTY;
 
-- 
2.8.2