xfs: data races on xfs_params and xfs_globals tunables

[徐晗翔 <xuhx@hust.edu.cn>]

The global XFS tunables in xfs_params and xfs_globals are written through
sysctl/sysfs handlers and read on normal filesystem paths with no common
lock and without a consistent READ_ONCE()/WRITE_ONCE() protocol.  For the
most concrete case, _xfs_alert_tag() reads xfs_panic_mask twice:

    if (xfs_panic_mask && (xfs_panic_mask & panic_tag)) {
            xfs_alert(mp, "Transforming an alert into a BUG.");
            do_panic = 1;
    }

A concurrent write to /proc/sys/fs/xfs/panic_mask can change the value
between the two loads, so the first test can see a non-zero mask and the
second a zero (or a different) mask.  The result is either a missed
panic or a panic the administrator had just disabled.  The same pattern
exists for xfs_error_level and for the timer/rotorstep/inherit knobs; the
DEBUG-only xfs_globals sysfs attributes have the same plain-read/plain-
write pairing.  This is a policy/diagnostic data race, not memory
corruption, and I am reporting it separately from the stats_clear UAF.

The detailed description:

* Test environment / affected version

  Linux 7.3.0 snapshot (the tree used for this audit; Makefile reports
  VERSION=7 PATCHLEVEL=3 SUBLEVEL=0), x86_64.  The report is based on
  source inspection, not on a KCSAN run: the data race is visible in the
  C access pattern, but the package does not claim a dynamic splat.
  xfs_globals sysfs attributes exist only in DEBUG builds.

* Mechanism

  The storage is:

      fs/xfs/xfs_sysctl.h:15-34       struct xfs_params
      fs/xfs/xfs_globals.c:15-29      struct xfs_globals xfs_globals
      fs/xfs/xfs_platform.h:104-115   xfs_panic_mask, xfs_error_level, ...

  The sysctl table is fs/xfs/xfs_sysctl.c:69-182.  Most entries use
  proc_dointvec_minmax(); its proc_int_conv() in kernel/sysctl.c uses
  WRITE_ONCE(*k_ptr, tmp_k).  But xfs_panic_mask_proc_handler() then
  does a plain assignment and a plain OR:

      fs/xfs/xfs_sysctl.c:33-49
      ret = proc_dointvec_minmax(ctl, write, buffer, lenp, ppos);
      if (!ret && write) {
              xfs_panic_mask = *valp;
      #ifdef DEBUG
              xfs_panic_mask |= (XFS_PTAG_SHUTDOWN_CORRUPT | XFS_PTAG_LOGRES);
      #endif
      }

  The read side is plain C everywhere:

      fs/xfs/xfs_message.c:64          xfs_panic_mask (two reads)
      fs/xfs/xfs_message.c:50          xfs_error_level
      fs/xfs/xfs_error.c:249,269,295,326,333,376,383
      fs/xfs/xfs_fsops.c:533
      fs/xfs/xfs_log.c:3408
      fs/xfs/xfs_da_btree.c:2781
      fs/xfs/xfs_icache.c:209,230      syncd/blockgc timers
      fs/xfs/xfs_filestream.c:281,388 rotorstep/fstrm timer
      fs/xfs/libxfs/xfs_inode_util.c:177-189  inherit_*

  There is no xfs_params lock, no seqlock, and no READ_ONCE() on these
  reads.  A write-side WRITE_ONCE() is not sufficient by itself: the two
  reads in _xfs_alert_tag() can still be separated, and the compiler can
  merge or reorder plain loads.

  The DEBUG-only sysfs attributes in fs/xfs/xfs_sysfs.c:84-314 are the
  same pattern: bug_on_assert_store() writes a bool at :98/:100,
  bug_on_assert_show() reads it at :112, and assfail() reads it at
  fs/xfs/xfs_message.c:101; log_recovery_delay, mount_delay, always_cow,
  pwork_threads, larp, and bload_* have analogous store/consumer pairs.

* Trigger model

  - Root writes one of /proc/sys/fs/xfs/* (or a DEBUG sysfs attribute)
    while another CPU is in _xfs_alert_tag(), xfs_error_report(), a
    mount/recovery path, or an xattr path that reads the same tunable.
  - No module unload and no memory reclaim are required.
  - The visible effect is a changed policy decision: a missing or extra
    BUG(), different stack-trace verbosity, a timer interval sampled old
    or new, etc.  It is not a memory-safety violation and does not cross
    a privilege boundary by itself.
  - KCSAN would report the plain read/write pairs; this package does not
    include such a run, so the report should be treated as a static
    data-race finding and a cleanup proposal, not as a security bug.



* Suggested direction 

  - Use READ_ONCE() for every read of xfs_params.<member>.val and
    xfs_globals.<member>; use WRITE_ONCE() for the write side, including
    the plain assignment/OR in xfs_panic_mask_proc_handler().
  - In _xfs_alert_tag(), take one local snapshot and use it for both
    tests:

        uint32_t mask = READ_ONCE(xfs_panic_mask);

        if (mask && (mask & panic_tag)) {
                xfs_alert(mp, "Transforming an alert into a BUG.");
                do_panic = 1;
        }

  - For booleans that must take effect immediately (bug_on_assert), a
    static_key or a WRITE_ONCE() plus local snapshot is preferable to a
    plain bool.  For the timer/rotorstep knobs, READ_ONCE() is enough to
    remove the undefined behavior and KCSAN report; multi-field policy
    consistency is not required.

  This is intentionally a low-priority report.  If the maintainers prefer
  to fold it into a general tunables cleanup rather than treat it as a
  standalone bug, that is fine; the important part is the access-pattern
  fix and the two-read snapshot in _xfs_alert_tag().


  Please keep me in Cc; I am not subscribed to the lists.

Regards,
Hanxiang Xu
xuhx@hust.edu.cn