[PATCH v4 0/4] ntfs: fix volume flag update races and persist the recorded error state

[Hongling Zeng]

Hi all,

v4 of the series, incorporating Baolin Wang's review of v3.  The 3/3
hygiene patch ("ntfs: NULL vol->vol_ino in the load_system_files()
error teardown") has been applied separately, so this respin carries
the remaining work as four patches.  1/4 and 3/4 are unchanged from v3
(formerly 1/3 and 2/3); 2/4 and 4/4 are new and close the two races
Baolin spotted.

Changes since v3:

  2/4 (new): the callers in file.c and namei.c checked VOLUME_IS_DIRTY
      without any lock before calling ntfs_set_volume_flags(), so the
      check could race with the ntfs_sync_fs() clear and leave the
      volume clean on disk despite a metadata modification.  They now
      call ntfs_set_volume_flags() unconditionally; with the
      unchanged-value check under the mrec_lock from 1/4 this is a
      cheap no-op in the write itself when the bit is already set, at
      the cost of taking the lock on every call - see 2/4's commit
      message for why that cannot be avoided.

  4/4 (new): ntfs_put_super() persisted the dirty state before the
      just-in-case mftmirr/mft commits and the final write_inode_now(),
      which can themselves record NVolErrors(); errors from those
      points would leave the volume unmounted with a clean on-disk
      dirty bit, contradicting the "cannot unmount clean" guarantee.
      The persistence now happens after the last of them, with the
      iput(vol->vol_ino) moved to the end of ntfs_put_super() so
      vol->vol_ino is still available.

To recap the discussion that shaped v3:

The original patch (now 1/4) fixed the lost-update race where
ntfs_set_volume_flags() and ntfs_clear_volume_flags() computed the new
value from vol->vol_flags outside any lock.  The review correctly
pointed out that a race remained in ntfs_sync_fs(): it checked
NVolErrors() outside the critical section, so a concurrent error path
could still record an error after the check and before the clear, and
the volume would end up persisted as clean despite the recorded error.
1/4 fixes both: the read-modify-write and the NVolErrors()-checked
clearing of VOLUME_IS_DIRTY happen inside the same mrec_lock critical
section.

3/4 closes the writer-side half of the window: the runtime
metadata-corruption paths in fs/ntfs recorded only the in-memory
NVolErrors() flag and never persisted VOLUME_IS_DIRTY at all, so a
volume could unmount with a clean on-disk flag despite recorded
corruption.  Persisting from the error paths themselves is not an
option: they run under a wide variety of ntfs locks (runlist locks,
vol->lcnbmp_lock, vol->mftbmp_lock, mrec_locks) while the dirty-bit
write needs the $Volume mrec_lock and can take the $MFT runlist lock,
which self-deadlocks or forms ABBA cycles.  Instead, 3/4 makes
persistence a property of the lock-free sync contexts: the new
ntfs_sync_volume_dirty_state() sets VOLUME_IS_DIRTY when NVolErrors()
is recorded and clears it otherwise, evaluating the error flag under
the $Volume mrec_lock.  It is called from ntfs_sync_fs(), the
remount-to-read-only path and ntfs_put_super(), with NV_Hibernated
gating so a hibernated volume is never written by these paths.  The
guarantee is eventual rather than instantaneous (a crash between the
error record and the next persistence point remains a window); with
4/4, ntfs_put_super() persists after the last commit that can record
errors, so a volume that is read-write at unmount time cannot unmount
clean.

One housekeeping note: this series was rebased onto the current ntfs
tree after the first posting failed to apply there.  Dennis Tighe's
"ntfs: do not mark the volume clean in sync_fs when errors were
recorded" landed in the meantime and fixed part of the same sync_fs
problem this series addresses; 3/4 supersedes that fix with the
ntfs_sync_volume_dirty_state() helper, which guards the clear in every
persistence context, not just sync_fs.  No conflict with its intent.

Patch layout:

  1/4 ntfs: fix volume flag update races
      Move the read-modify-write of vol->vol_flags inside the
      $Volume mrec_lock; ntfs_sync_fs() clears the dirty bit under
      the lock with the NVolErrors() check.

  2/4 ntfs: set the volume dirty bit unconditionally on metadata
      changes
      Drop the racy caller-side VOLUME_IS_DIRTY checks in file.c and
      namei.c; the locked unchanged-value check makes the unconditional
      call skip the write when the bit is already set.

  3/4 ntfs: sync the volume dirty bit with the recorded error state
      New ntfs_sync_volume_dirty_state(), called from the lock-free
      sync contexts (sync_fs / remount-ro / put_super); NV_Hibernated
      gating.

  4/4 ntfs: persist the dirty state after the final put_super()
      commits
      Move the persistence after the last commits that can record
      errors, with the iput(vol->vol_ino) moved to the end.

Thanks,
Hongling Zeng (4):
  ntfs: fix volume flag update races
  ntfs: set the volume dirty bit unconditionally on metadata changes
  ntfs: sync the volume dirty bit with the recorded error state
  ntfs: persist the dirty state after the final put_super() commits

 fs/ntfs/file.c   |  14 ++--
 fs/ntfs/namei.c  |  24 +++----
 fs/ntfs/super.c  | 140 +++++++++++++++++++++++++++++++------------
 fs/ntfs/volume.h |   4 ++
 4 files changed, 130 insertions(+), 52 deletions(-)