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

[Hongling Zeng]

Hi all,

First of all, my apologies for the many versions of this patch.  I
should have folded the reviewer feedback in and resent a single coherent
series sooner; the split across v1/v2 and follow-ups made the thread
harder to follow than it needed to be.  This v3 consolidates everything
into one series of three patches.

To recap the review discussion:

The original patch (now 1/3) 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,
meaning chkdsk would not run on the next mount.

That race is now fixed in 1/3 itself: the NVolErrors() check and the
clearing of VOLUME_IS_DIRTY both happen inside the same mrec_lock
critical section, via a dedicated ntfs_clear_volume_dirty_if_no_errors()
helper.  With that, every interleaving of the sync thread and the error
path leaves the volume dirty on disk.

Patches 2/3 then close 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, 2/3 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);
ntfs_put_super() runs on a quiesced filesystem after evict_inodes(),
so a volume that is read-write at unmount time cannot unmount clean.

Patch 3/3 is pure hygiene found during review: the load_system_files()
error unwind leaves a stale vol->vol_ino pointer in place, and carries
a dead iput() inside the IS_ERR(vol->vol_ino) branch; NULL the pointer
like ntfs_put_super() and the ntfs_fill_super() error path already do.

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; patch 2/3 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/3 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/3 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.

  3/3 ntfs: NULL vol->vol_ino in the load_system_files() error
      teardown
      Drop the stale pointer and the dead iput().

The series builds cleanly with W=1.

Thanks,
Hongling