kernfs: kernfs_drain()'s fast-path decision reads the active count without acquire ordering -- on weakly-ordered architectures the forced ->release()/unmap can be skipped

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

[-- Attachment #1: Type: text/plain, Size: 6825 bytes --]

kernfs_drain() (fs/kernfs/dir.c:508-551) decides whether it must
drain open files with the fast-path test "atomic_read(&kn->active)
== KN_DEACTIVATED_BIAS && !kernfs_should_drain_open_files(kn)" --
both a plain atomic_read (relaxed) and an unlocked read of
kn->attr.open / on->nr_mmapped / on->nr_to_release.  The writer side
(the last active holder) releases correctly -- its writes to
attr.open/counter happen under the hash lock, followed by the fully
ordered atomic_dec_return() that wakes the drain waiter
(dir.c:491-495) -- but on the READER side there is no acquire edge
on any fast path: the early-return branch (:522-524), the
wait_event's non-sleeping first check (:537-538), and the slow-loop
checks where the waker has not yet touched the queue lock.  The
phase-1 BIAS add is itself a relaxed atomic_add (:1563), and the
kernfs_rwsem write lock the remover holds has no intersection with
the putter.  On a weakly-ordered architecture (arm64, powerpc,
riscv; x86-TSO is immune because plain load-load does not reorder),
the remover can therefore observe "active drained" WITHOUT observing
the racing opener's attr.open/nr_to_release/nr_mmapped updates, skip
kernfs_drain_open_files() entirely, and leave the mandatory
->release() and unmap_mapping_range() to be deferred to the last fd
close.  Consequences under current semantics: no memory corruption
(of->kn/inode stay pinned through the dentry chain; the close path
performs the release and decrements then, and the accounting WARN
never fires) -- the observable effects are a delayed
cgroup_file_release()/put_cgroup_ns() (a cgroup-namespace reference
pinned for the lifetime of a leaked-open fd) and a skipped
unmap_mapping_range() (mmap'ed vmas survive until their next fault
takes the kernfs fault path and SIGBUSes, safely).  I verified the
RACE itself dynamically on a stock Linux 7.3 KCSAN kernel: a
cgroup2-slot hammer (attached) produced 17 captures of exactly the
target race pair -- kernfs_fop_release (the nr_to_release--
writer, inlined in the close path) vs kernfs_should_drain_open_files
(read), with the reader stack at kernfs_drain+0xba, which I mapped
back to dir.c:522-524 (the early-return decision) via objdump (the
cmp $0x80000001 / KN_DEACTIVATED_BIAS immediate).  The consequence
itself needs a weakly-ordered machine plus a microsecond window, so
I did not and could not trigger it on x86 -- the report is therefore
a memory-ordering fix with (a) the race pair proven to occur as a
matter of course on x86, (b) an LKMM litmus showing the outcome is
Allowed on linux-kernel.cat and Never on x86-TSO (available; please
run it), and (c) a one-line fix that closes all three fast paths.

The detailed description:

* The ordering gap (Linux 7.3)

  Writer (last active holder of a HAS_RELEASE file):
      mutex(attr.open = on); on->nr_to_release = 1   <- plain writes,
                                                        hash-locked
      ... ops->open() ...
      kernfs_put_active(): atomic_dec_return(active)  <- fully ordered
                                                        RMW (release
                                                        for the writer)
                                                        + wake_up_all
                                                        (dir.c:491-495)

  Reader (the remover, holding kernfs_rwsem write since phase 1 --
  itself a relaxed atomic_add(BIAS) at dir.c:1563):
      kernfs_drain():
        if (atomic_read(&kn->active) == KN_DEACTIVATED_BIAS &&   /* :522,
                                                    relaxed */
            !kernfs_should_drain_open_files(kn))     /* :784-802 plain
                                                        loads */
                return;                              /* :522-524 early
                                                        out */
        wait_event(root->deactivate_waitq,
                   atomic_read(&kn->active) == BIAS) /* :537-538 first
                                                        check also
                                                        relaxed */

  No lock edge (the fast paths never take the queue lock), no data
  or address dependency between active and attr.open/on->, and
  control dependencies do not order subsequent loads
  (Documentation/memory-barriers.txt).  Only the fully-sleeping path
  is ordered today (putter's RMW -> wake_up_all's queue lock ->
  waiter's recheck under that lock).

* Evidence (stock 7.3 KCSAN kernel; hammer attached)

  kfshammer.c drives synchronized cgroup2 slot rmdir/open/close
  traffic (the cgroup release path is the HAS_RELEASE consumer).
  300 s: 10,287 rmdirs x 185,310 open/close -> the exact target race
  pair captured 17 times:

    BUG: KCSAN: data-race in kernfs_fop_release /
         kernfs_should_drain_open_files
    reader stack at kernfs_drain+0xba   <- mapped to dir.c:522-524
      (objdump: cmp $0x80000001 == KN_DEACTIVATED_BIAS; also +0x1ab =
       dir.c:545)
    writer stack: the close path's inlined nr_to_release--
    plus 7 same-root reports (__kernfs_remove /
    kernfs_unlink_open_file family)

  Attribution controls: a sequential-cleanup control (kfs1ctl2) also
  captures the pair (plain rmdir chains reach it); a TRUE
  zero-rmdir control (kfs1ctl4, 692,469 open+close, zero rmdirs)
  shows should_drain never executing and zero target-family
  reports -- the pair is specific to rmdir-vs-close concurrency.
  The stale-READ consequence (skipped drain) is architecturally
  impossible on this x86 guest; the race itself is not.

* The fix (one line, both decision sites)

  --- a/fs/kernfs/dir.c
  +++ b/fs/kernfs/dir.c

  @@ -519
  -	if (atomic_read(&kn->active) == KN_DEACTIVATED_BIAS &&
  -	    !kernfs_should_drain_open_files(kn))
  +	/* acquire pairs with the fully-ordered dec_return in
  +	 * kernfs_put_active() so that the skip decision observes the
  +	 * racing opener's attr.open/counter updates */
  +	if (atomic_read_acquire(&kn->active) == KN_DEACTIVATED_BIAS &&
  +	    !kernfs_should_drain_open_files(kn))
   		return;
  @@ -537
  -	wait_event(root->deactivate_waitq,
  -		   atomic_read(&kn->active) == KN_DEACTIVATED_BIAS);
  +	wait_event(root->deactivate_waitq,
  +		   atomic_read_acquire(&kn->active) == KN_DEACTIVATED_BIAS);

  atomic_read_acquire() is available in this tree (include/linux/
  atomic/atomic-instrumented.h:47) and covers all three fast paths;
  the sleeping path was already ordered and is unaffected.
  smp_load_acquire() is an equivalent alternative.

  A herd7 litmus test for the pair (Expected: Allowed on
  linux-kernel.cat, Never on x86-TSO) is available and will be
  provided with the patch -- please run it, as no herd7 exists on my
  side.



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


Regards,
Hanxiang Xu
xuhx@hust.edu.cn

[-- Attachment #2: kfshammer.c --]
[-- Type: text/plain, Size: 9682 bytes --]

// SPDX-License-Identifier: GPL-2.0
/* kfshammer.c - user-space hammer for fs__kernfs audit verification (v3).
 *
 * Target: kernfs open-file accounting vs drain decision (fs/kernfs/file.c,
 * fs/kernfs/dir.c), exercised on cgroup2 attribute files.
 *
 * Synchronized slot design (structurally forces the races instead of
 * relying on timing luck):
 *
 *   slot states: 0 empty -> 1 prepared (subtree t_i/{x,y} created, 12 fds
 *   open) -> 2 removing (rmdir(2) in-kernel, recursive kernfs_remove over
 *   ~33 nodes) -> 0.
 *
 *   On seeing state 2, the prep thread closes its fds one by one with
 *   microsecond gaps, so kernfs_unlink_open_file (plain stores under the
 *   hashed per-node mutex) executes inside / right after the removal,
 *   concurrent with kernfs_drain's kernfs_should_drain_open_files loads
 *   (plain loads, no acquire: audit Case 5).  fd[0] closes last, often
 *   after the forced release, exercising kernfs_release_file exactly-once
 *   and the close-after-release WARN tripwire (file.c:586).
 *
 * Plans:
 *   kfs1 (KCSAN): expect data-race reports naming
 *        kernfs_should_drain_open_files / kernfs_unlink_open_file /
 *        kernfs_get_open_node / kernfs_drain_open_files
 *   kfs1ctl: ctrl=1, rmdir disabled -> drain never runs -> accounting is
 *            mutex-protected -> expect zero drain/accounting reports
 *   kfs2 (KASAN): heavier negative control; in-kernel tripwires
 *        (file.c:586, file.c:830, dir.c:602) must stay silent and KASAN
 *        must see no use-after-free (any hit refutes the Case 2/3 loop)
 *
 * usage: kfshammer <cg2-mount> <slots> <rmdir-threads> <seconds> [ctrl]
 */
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <fcntl.h>
#include <poll.h>
#include <pthread.h>
#include <time.h>
#include <sys/stat.h>

#define MAXSLOTS 32
#define NF_TOP	 10
#define NF_CHILD 4

static char mount_[256];
static int slots = 8, rmdirs = 3, secs = 120, ctrl = 0;
static volatile int running = 1;

/* per-slot state: 0 empty, 1 prepared (fds open), 2 removing */
static volatile int slot_state[MAXSLOTS];
static int slot_fd[MAXSLOTS][NF_TOP + 2 * NF_CHILD];
static int slot_nopen[MAXSLOTS];

static const char *files_top[NF_TOP] = {
	"cgroup.procs", "cgroup.type", "cgroup.stat", "cgroup.events",
	"cgroup.controllers", "cgroup.subtree_control", "cgroup.threads",
	"cgroup.max.descendants", "cgroup.max.depth", "cgroup.freeze",
};
static const char *files_child[NF_CHILD] = {
	"cgroup.procs", "cgroup.type", "cgroup.stat", "cgroup.events",
};

enum { E_MKDIR_OK, E_MKDIR_FAIL, E_OPEN_OK, E_OPEN_FAIL, E_CLOSE,
       E_RMDIR_OK, E_RMDIR_FAIL, E_MAX };
static long st[E_MAX];
static int last_rmdir_errno, last_open_errno;
#define ATOMIC_INC(x) __atomic_add_fetch(&(x), 1, __ATOMIC_RELAXED)

/* nanosleep with us ranges from 50us to 500ms - all < 1s, tv_nsec safe */
static int usleep_(long us)
{
	struct timespec ts = { .tv_sec = 0, .tv_nsec = us * 1000L };
	if (ts.tv_nsec >= 1000000000L) {	/* paranoia */
		ts.tv_sec += ts.tv_nsec / 1000000000L;
		ts.tv_nsec %= 1000000000L;
	}
	return nanosleep(&ts, NULL);
}

static void mkpath(char *buf, size_t len, int slot, const char *sub,
		   const char *f)
{
	snprintf(buf, len, "%s/t%d%s%s%s%s", mount_, slot, sub ? "/" : "",
		 sub ?: "", f ? "/" : "", f ?: "");
}

/* prep thread: owns one slot */
static void *th_prep(void *arg)
{
	long id = (long)(intptr_t)arg;
	char path[512];
	unsigned seed = id * 7919u + 7;
	int n = 0;

	while (running) {
		mkpath(path, sizeof(path), (int)id, NULL, NULL);
		if (mkdir(path, 0755) == 0) {
			ATOMIC_INC(st[E_MKDIR_OK]);
		} else if (!ctrl) {
			ATOMIC_INC(st[E_MKDIR_FAIL]);
			/* stale dir from a failed cycle: best-effort cleanup
			 * (bottom-up), then retry */
			mkpath(path, sizeof(path), (int)id, "x", NULL);
			rmdir(path);
			mkpath(path, sizeof(path), (int)id, "y", NULL);
			rmdir(path);
			mkpath(path, sizeof(path), (int)id, NULL, NULL);
			rmdir(path);
			usleep_(2000);
			continue;
		} else {
			/* ctrl: dir persists - churn on it, zero rmdir calls */
			ATOMIC_INC(st[E_MKDIR_FAIL]);
		}
		mkpath(path, sizeof(path), (int)id, "x", NULL);
		mkdir(path, 0755);
		mkpath(path, sizeof(path), (int)id, "y", NULL);
		mkdir(path, 0755);

		/* fd[0] = top cgroup.procs; closes LAST (after forced release) */
		n = 0;
		mkpath(path, sizeof(path), (int)id, NULL, files_top[0]);
		int fd = open(path, O_RDONLY);
		if (fd >= 0) { slot_fd[id][n++] = fd; ATOMIC_INC(st[E_OPEN_OK]); }
		else { last_open_errno = errno; ATOMIC_INC(st[E_OPEN_FAIL]); }

		for (int i = 1; i < NF_TOP; i++) {
			mkpath(path, sizeof(path), (int)id, NULL, files_top[i]);
			fd = open(path, O_RDONLY);
			if (fd < 0) { last_open_errno = errno; ATOMIC_INC(st[E_OPEN_FAIL]); continue; }
			ATOMIC_INC(st[E_OPEN_OK]);
			slot_fd[id][n++] = fd;
		}
		for (int c = 0; c < 2; c++) {
			const char *sub = c ? "y" : "x";
			for (int i = 0; i < NF_CHILD; i++) {
				mkpath(path, sizeof(path), (int)id, sub, files_child[i]);
				fd = open(path, O_RDONLY);
				if (fd < 0) { last_open_errno = errno; ATOMIC_INC(st[E_OPEN_FAIL]); continue; }
				ATOMIC_INC(st[E_OPEN_OK]);
				slot_fd[id][n++] = fd;
			}
		}
		slot_nopen[id] = n;
		if (ctrl) {
			/* control mode (churn-only): open/read/poll/close on
			 * persistent dirs, NO removal ever -> kernfs_drain /
			 * kernfs_should_drain_open_files never execute, so
			 * the target race site is unreachable by construction;
			 * accounting stays under the hashed mutex */
			for (int i = 0; i < n; i++) {
				close(slot_fd[id][i]);
				ATOMIC_INC(st[E_CLOSE]);
			}
			slot_nopen[id] = 0;
			usleep_(500);
			continue;
		}
		if (n >= 2) {
			slot_state[id] = 1;

			/* short poll on fd0: exercises kernfs_generic_poll;
			 * (kernfs releases the active ref before poll sleeps,
			 * so this does NOT lengthen the drain window) */
			if ((rand_r(&seed) & 3) == 0) {
				struct pollfd pf = { .fd = slot_fd[id][0],
						     .events = POLLPRI };
				poll(&pf, 1, 0);
			}

			/* wait for the remover to mark state 2 */
			int spins = 0;
			while (running && slot_state[id] == 1 && spins < 3000) {
				usleep_(100);
				spins++;
			}

			/* close fds INSIDE the rmdir(2) kernel window, with
			 * gaps so stores land across the whole removal */
			for (int i = 1; i < slot_nopen[id]; i++) {
				close(slot_fd[id][i]);
				ATOMIC_INC(st[E_CLOSE]);
				usleep_(100 + (long)(rand_r(&seed) % 300));
			}
			/* fd[0] closes last: often after the forced release */
			close(slot_fd[id][0]);
			ATOMIC_INC(st[E_CLOSE]);
		} else {
			for (int i = 0; i < n; i++)
				close(slot_fd[id][i]);
		}
		slot_nopen[id] = 0;
		slot_state[id] = 0;
		usleep_(500);
	}
	return NULL;
}

/* remover thread: drive slots through state 2; bottom-up (children first,
 * each child rmdir runs its own drain window, then the top removal runs
 * with the top-level fds still open) */
static void *th_rmdir(void *arg)
{
	long id = (long)(intptr_t)arg;
	char path[512];
	unsigned seed = id * 104729u + 1;
	int i = (int)id;

	while (running) {
		if (slot_state[i] == 1) {
			slot_state[i] = 2;
			int ok = 1;
			mkpath(path, sizeof(path), i, "x", NULL);
			if (rmdir(path) != 0)
				ok = 0;
			mkpath(path, sizeof(path), i, "y", NULL);
			if (rmdir(path) != 0)
				ok = 0;
			mkpath(path, sizeof(path), i, NULL, NULL);
			if (rmdir(path) != 0)
				ok = 0;
			if (ok)
				ATOMIC_INC(st[E_RMDIR_OK]);
			else {
				last_rmdir_errno = errno;
				ATOMIC_INC(st[E_RMDIR_FAIL]);
			}
		}
		i = (i + 1) % slots;
		usleep_(rand_r(&seed) % 400);
	}
	return NULL;
}

static void *th_report(void *arg)
{
	(void)arg;
	/* exactly 3 progress lines per run to avoid serial flooding */
	long wait[] = { 30, 90, 0 };
	for (int k = 0; k < 3 && running; k++) {
		struct timespec ts = { .tv_sec = wait[k] ? wait[k] : secs,
				       .tv_nsec = 0 };
		if (nanosleep(&ts, NULL) && !running)
			break;
		fprintf(stderr,
			"KFSHAMMER-PROGRESS[%d]: mkdir_ok=%ld open_ok=%ld "
			"close=%ld rmdir_ok=%ld rmdir_fail=%ld "
			"last_rmdir_errno=%d last_open_errno=%d\n",
			k + 1, st[E_MKDIR_OK], st[E_OPEN_OK], st[E_CLOSE],
			st[E_RMDIR_OK], st[E_RMDIR_FAIL],
			last_rmdir_errno, last_open_errno);
	}
	return NULL;
}

int main(int argc, char **argv)
{
	if (argc < 5) {
		fprintf(stderr,
			"usage: %s <mount> <slots> <rmdir-threads> <secs> [ctrl]\n",
			argv[0]);
		return 1;
	}
	snprintf(mount_, sizeof(mount_), "%s", argv[1]);
	slots = atoi(argv[2]);
	rmdirs = atoi(argv[3]);
	secs = atoi(argv[4]);
	if (argc > 5)
		ctrl = atoi(argv[5]);
	if (slots > MAXSLOTS)
		slots = MAXSLOTS;

	int nthreads = slots + (ctrl ? 0 : rmdirs) + 1;
	pthread_t *t = calloc(nthreads, sizeof(*t));
	for (long i = 0; i < slots; i++)
		pthread_create(&t[i], NULL, th_prep, (void *)i);
	for (long i = 0; i < (long)(ctrl ? 0u : (unsigned)rmdirs); i++)
		pthread_create(&t[slots + i], NULL, th_rmdir, (void *)(long)i);
	pthread_create(&t[nthreads - 1], NULL, th_report, NULL);

	struct timespec ts = { .tv_sec = secs, .tv_nsec = 0 };
	nanosleep(&ts, &ts);
	running = 0;
	for (int i = 0; i < nthreads; i++)
		pthread_join(t[i], NULL);

	printf("KFSHAMMER-FINAL: mkdir_ok=%ld mkdir_fail=%ld open_ok=%ld "
	       "open_fail=%ld close=%ld rmdir_ok=%ld rmdir_fail=%ld "
	       "last_rmdir_errno=%d last_open_errno=%d\n",
	       st[E_MKDIR_OK], st[E_MKDIR_FAIL], st[E_OPEN_OK],
	       st[E_OPEN_FAIL], st[E_CLOSE], st[E_RMDIR_OK],
	       st[E_RMDIR_FAIL], last_rmdir_errno, last_open_errno);
	fflush(stdout);
	printf("KFSHAMMER-FINAL2: rmdir_ok=%ld rmdir_fail=%ld close=%ld "
	       "open_ok=%ld mkdir_ok=%ld\n",
	       st[E_RMDIR_OK], st[E_RMDIR_FAIL], st[E_CLOSE], st[E_OPEN_OK],
	       st[E_MKDIR_OK]);
	fflush(stdout);
	return 0;
}