linux-ext4 - Re: [PATCH, RFC] fs: only call sync_filesystem() when remounting read-only

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: <20140313125535.GB11752@thunk.org>
Date:	Thu, 13 Mar 2014 08:55:35 -0400
From:	Theodore Ts'o <tytso@....edu>
To:	Dave Chinner <david@...morbit.com>
Cc:	Lucas Nussbaum <lucas.nussbaum@...ia.fr>,
	linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	Emmanuel Jeanvoine <emmanuel.jeanvoine@...ia.fr>,
	Dmitry Monakhov <dmonakhov@...nvz.org>
Subject: Re: [PATCH, RFC] fs: only call sync_filesystem() when remounting
 read-only

On Thu, Mar 13, 2014 at 05:04:44PM +1100, Dave Chinner wrote:
> XFS most definitely considered sync_filesystem(sb) to be a data
> integrity operation. xfs_fs_sync_fs() calls this:
> 
> 	xfs_log_force(mp, XFS_LOG_SYNC);
> 
> Which will issue a blocking journal commit which will uses
> REQ_FLUSH|REQ_FUA for the journal writes. Hence if there was
> anything dirty in the filesystem that sync_filesystem wrote to disk,
> it will issue a cache flush just like ext4 does.

Sorry, I didn't quite do my experimets right, apparetly.

So I did the following test:

dd if=/etc/motd of=/mnt/test conv=notrunc ; mount -o remount /mnt ; mount -o remount /mnt ; mount -o remount /mnt

This is what XFS does:

252,2    0        1     0.000000000 15453  Q   W 88 + 8 [kworker/u16:1]
252,2    0        2     0.000230348 15453  C   W 88 + 8 [0]
252,2    7        1     0.000334892 15544  Q FWFSM 5243112 + 8 [mount]
252,2    0        3     0.251131828     0  C WFSM 5243112 + 8 [0]
252,2    0        4     0.251495890 15087  Q  WM 96 + 16 [xfsaild/dm-2]
252,2    0        5     0.251729470     0  C  WM 96 + 16 [0]
252,2    4        1     0.263317936 11070  Q FWFSM 5243120 + 8 [kworker/4:2]
252,2    0        6     0.273394400     0  C WFSM 5243120 + 8 [0]
252,2    0        7     0.273678692 15087  Q  WM 0 + 8 [xfsaild/dm-2]
252,2    0        8     0.273902550     0  C  WM 0 + 8 [0]
252,2    0        9     0.295673237     0  C WFSM 5243128 + 8 [0]
252,2    0       10     0.296035803 15087  Q  WM 0 + 8 [xfsaild/dm-2]
252,2    0       11     0.296266732     0  C  WM 0 + 8 [0]
252,2    7        2     0.286271844 24012  Q FWFSM 5243128 + 8 [kworker/7:0]

... and this is what ext4 does:

252,4    7        8    10.973326622 15512  Q  WM 78 + 2 [mount]
252,4    7        9    10.973576941 15512  Q FWS [mount]
252,4    2        1    10.973141488 15271  Q   W 108 + 2 [kworker/u16:3]
252,4    0       24    10.973390538     0  C   W 108 + 2 [0]
252,4    0       25    10.973548736     0  C  WM 78 + 2 [0]
252,4    7       10    11.244052462 15513  Q FWS [mount]
252,4    0       26    11.231292967     0  C FWS 0 [0]
252,4    0       27    11.231452643 15512  Q  WS 2 + 2 [mount]
252,4    0       28    11.231686794     0  C  WS 2 + 2 [0]
252,4    7       11    11.266337812 15514  Q FWS [mount]
252,4    0       29    11.253022650     0  C FWS 0 [0]
252,4    0       30    11.253135113 15513  Q  WS 2 + 2 [mount]
252,4    0       31    11.253376707     0  C  WS 2 + 2 [0]
252,4    0       32    11.266640135     0  C FWS 0 [0]
252,4    0       33    11.266727461 15514  Q  WS 2 + 2 [mount]
252,4    0       34    11.266954710     0  C  WS 2 + 2 [0]

> > 1)  Nowhere in the remount system call is it stated that it has
> >     ***any*** data integrity implications.   If you are making the rw->ro
> >     transition, sure, you'll need to flush out any pending changes.  But there
> >     doesn't seem to be any justification for requiring this this if the
> >     remount is a no-op.   So I think changing the remount code path as I
> >     suggested is a valid option.
> 
> What the man page says doesn't change the fact we need to audit all
> the existing filesystems before such a change is made.

Fair enough, I'll do what Christoph suggested and move the
sync_filesystems() call into all of the existing file systems.

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