perf/uncore: activeboxes count TOCTOU lets PCI probe/remove run perf_pmu_register() and perf_pmu_unregister() concurrently on the same struct pmu

perf/uncore: activeboxes count TOCTOU lets PCI probe/remove run perf_pmu_register() and perf_pmu_unregister() concurrently on the same struct pmu

[徐晗翔]

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

The uncore PCI driver decides "first active box registers the PMU" and
"last active box unregisters the PMU" with nothing but the
pmu->activeboxes counter -- the decision and the perf_pmu_register()/
perf_pmu_unregister() execution are not serialized -- so on a multi-die
platform whose same-typed boxes live on different dies, concurrently
removing the last box of one die and probing a box of another die runs
register and unregister on the SAME struct pmu at the same time; the
unregister side's unlocked tail (two grace periods, then idr_remove()
re-reading pmu->type and perf_pmu_free() reading pmu->dev and
pmu->cpu_pmu_context) then consumes whatever the concurrent register
just published, and the kernel oopses or leaves a PMU in the list
holding freed per-CPU contexts.  The perf-core half of this is
confirmed dynamically on mainline 7.3 (first concurrency round:
page fault oops inside perf_pmu_unregister); the uncore-side trigger
is source-proven on 7.3's current (recently refactored) code, which
added box refcounting and a PMU_BROKEN state but still no
serialization of this decision path.

Both decision sites are counter-only on 7.3 (arch/x86/events/intel/
uncore.c): uncore_box_setup() at :1153 skips registration when
atomic_inc_return(&pmu->activeboxes) > 1 (:1166), otherwise calls
uncore_pmu_register() (:926); uncore_pci_pmu_unregister() at :1283
calls uncore_pmu_unregister() (:960) when atomic_dec_return(...) == 0
(:1291).  Meanwhile the perf core is asymmetric: perf_pmu_register()
(kernel/events/core.c:12836) holds pmus_lock for its whole body
(guard at :12841), while perf_pmu_unregister() (:13023) holds it only
for the idr_cmpxchg/list_del section (:13025-13029) and for the final
idr_remove (:13056-13057) -- between and after them the two
synchronize_srcu()/synchronize_rcu() grace periods (:13042-13043),
the idr_remove() that re-reads pmu->type, and perf_pmu_free()
(:13068, reading pmu->dev/pmu->cpu_pmu_context and freeing them) all
run unlocked for tens of milliseconds.  Triggering needs a multi-die
host with id-table uncore PCI drivers (snbep/skx families) and
concurrent sysfs unbind/rescan (or a hotplug/DPC event racing an
admin operation) -- root actions, but each entirely legitimate.

The detailed description:

* Test environment / affected version

  Linux version 7.3.0 (gcc 13.3.0, GNU ld 2.42) #1 SMP PREEMPT_DYNAMIC
  -- vanilla mainline 7.3 release tarball (kernel.org, unmodified),
  rebuilt with CONFIG_PERF_EVENTS=y, CONFIG_PERF_EVENTS_INTEL_UNCORE=y,
  KASAN enabled, QEMU/KVM x86_64 guest.  The perf-core functions
  cited are byte-identical in 7.1.5 and 7.3; the uncore functions were
  refactored during 7.3 (box refcounting, PMU_BROKEN stickiness,
  WARN_ON_ONCE against double probe at :1206) -- the refactor did NOT
  add any lock (the file's lock inventory is unchanged), and the
  counter-only decision shape survives it.  Line numbers below are
  from 7.3.

* Mechanism

  1. Counter-only decisions, no lock between decision and execution:

         uncore_box_setup()              /* uncore.c:1153 */
             ...
             if (atomic_inc_return(&pmu->activeboxes) > 1)   /* :1166 */
                     return 0;
             ret = uncore_pmu_register(pmu);                 /* first
                                                                box */

         uncore_pci_pmu_unregister()    /* uncore.c:1283 */
             pmu->boxes[die] = NULL;
             if (atomic_dec_return(&pmu->activeboxes) == 0)  /* :1291 */
                     uncore_pmu_unregister(pmu);             /* last
                                                                box */

     Two different PCI devices' probe and remove callbacks (per-device
     locks only; sysfs bind/unbind/rescan are not mutually exclusive)
     can hold both decisions simultaneously: remove drops die0's last
     box to 0 and decides "unregister" while probe brings die1's box
     from 0 to 1 and decides "register" -- same struct intel_uncore_pmu
     (type->pmus[] is shared across dies for id-table types).

  2. The register side rewrites the shared struct pmu from scratch:
     uncore_pmu_register() (:926) assigns the whole template
     (clearing ->dev, ->type, ->cpu_pmu_context) before calling
     perf_pmu_register(), which then creates a NEW device, allocates a
     NEW type id and a NEW percpu context array, and publishes them.

  3. The unregister side's unlocked tail reads what register just
     published.  With A = remove(die0) and B = probe(die1):

       A: perf_pmu_unregister() locks pmus_lock, idr_cmpxchg + list_del
          (:13025-13029), UNLOCKS, enters two grace periods
          (:13042-13043)                            -- tens of ms
       B: uncore_pmu_register() resets the struct, perf_pmu_register()
          (holding pmus_lock the whole time) creates device D2, type
          T2, contexts P2; INIT event list; publishes; returns 0
       A: resumes: idr_remove(&pmu_idr, pmu->type)  /* :13056-13057,
          re-reads pmu->type == T2 -> deletes B's fresh entry */
          perf_pmu_free(pmu)                        /* :13068: reads
          pmu->dev == D2 -> device_del/put; reads pmu->cpu_pmu_context
          == P2 -> kfrees B's fresh contexts + free_percpu */

     End states: a PMU still on the pmus list whose IDR entry is gone
     and whose contexts are freed (later perf_event_open() reaching
     find_get_pmu_context() walks freed memory), or the mirror
     interleaving where A's cmpxchg consumes B's registration wholesale
     (the active PMU vanishes; event creation fails with ENODEV until
     rebind).  uncore_pmu_unregister() (:960-967) also ignores
     perf_pmu_unregister()'s return value, so the -EINVAL variant
     leaves a half-torn state machine in place.

* Evidence (7.3)

  - Source-level, uncore side: the decision paths above contain no
    serialization on current 7.3 (verified function-by-function; the
    7.3 refactor touched exactly these functions without adding a
    lock).  Real-hardware trigger: concurrent
      echo <bdf0> > /sys/bus/pci/drivers/skx_uncore/unbind
      echo 1     > /sys/bus/pci/rescan
    on a multi-die host.
  - Dynamic, perf-core half (reproducer attached, pmu_race.c): its
    "race" action starts two kthreads faithfully mirroring both sides
    -- an unregister loop and a register loop that performs the same
    whole-struct reset uncore_pmu_register() does before each
    perf_pmu_register(), on the same struct pmu.  On the 4-vCPU KASAN
    VM the FIRST concurrency round (seconds into the run) ends in:

        CPU: 0 ... Comm: pmu_race_unreg
        RIP: 0010:perf_pmu_unregister+0x1cb/0x620
        #PF: supervisor read access ... address fffffffffffffe08

    reading a pointer derived from the concurrently rewritten pmu
    fields; the unregister thread dies and the race never converges.
    (Same signature on 7.1.5 at +0x1d3; the code between is
    identical.)  Usage:

      insmod pmu_race.ko
      echo "race 2000" > /sys/module/pmu_race/parameters/action
      echo status > /sys/module/pmu_race/parameters/action

    QEMU provides no uncore PCI devices, which is why the uncore-side
    trigger is presented source-level and the perf-core concurrency
    (the half both sides funnel into) is what the reproducer drives.


Thanks for taking a look.


Regards,
Hanxiang Xu
xuhx@hust.edu.cn


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

// SPDX-License-Identifier: GPL-2.0
/*
 * pmu_race.c - verification driver for perf/PMU race-condition audit cases
 * (R4/R6/R8/R9) against the KASAN verification kernel.
 *
 * Exposes an "action" module parameter that drives perf_pmu_register /
 * perf_pmu_unregister / perf_pmu_migrate_context / kernel-counter creation
 * on a fake PMU named "racepmu":
 *
 *   echo register        > action   : one perf_pmu_register
 *   echo unregister      > action   : one perf_pmu_unregister
 *   echo regloop N       > action   : N sequential register+unregister rounds
 *                                     (natural-window hammer, R4-A / baseline)
 *   echo race N          > action   : 2 kthreads: A=unregister loop,
 *                                     B=register loop on the same struct pmu
 *                                     (R8: activeboxes TOCTOU core, uncore-style
 *                                     template reset before each register)
 *   echo churn N         > action   : 3 kthreads: reg/unreg churn + migrate loop
 *                                     + open/close churn (R9 perf-core overlap)
 *   echo open N          > action   : N kernel counter create/release attempts
 *                                     (poisoned-state consumption)
 *   echo status          > action   : log registered state and stats
 *
 * The kernel source is NOT modified: everything is driven through exported
 * perf core APIs (perf_pmu_register/perf_pmu_unregister are EXPORT_SYMBOL_GPL,
 * perf_pmu_migrate_context and perf_event_create_kernel_counter likewise).
 */
#define pr_fmt(fmt) "PMURACE: " fmt

#include <linux/module.h>
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/kthread.h>
#include <linux/sched.h>
#include <linux/delay.h>
#include <linux/workqueue.h>
#include <linux/perf_event.h>
#include <linux/err.h>
#include <linux/string.h>

static struct pmu test_pmu;
static int my_registered;
static int test_type = -1;

/* statistics */
static unsigned long stat_reg_ok, stat_reg_fail;
static unsigned long stat_unreg_ok, stat_unreg_fail;
static unsigned long stat_open_ok, stat_open_fail;
static int open_err0, open_err1;

static int race_event_init(struct perf_event *event);
static void race_event_read(struct perf_event *event);
static int race_event_add(struct perf_event *event, int flags);
static void race_event_del(struct perf_event *event, int flags);
static void race_event_start(struct perf_event *event, int flags);
static void race_event_stop(struct perf_event *event, int flags);
static int open_thread(void *data);

/*
 * Template reset mirrors uncore_pmu_register()'s
 *     pmu->pmu = (struct pmu){ ... };
 * which clears ->dev / ->cpu_pmu_context / ->type etc. before re-registering.
 */
static void pmu_reset(void)
{
	static const struct pmu tmpl = {
		.task_ctx_nr	= perf_invalid_context,
		.module		= THIS_MODULE,
		.capabilities	= PERF_PMU_CAP_NO_INTERRUPT |
				  PERF_PMU_CAP_NO_EXCLUDE,
		.event_init	= race_event_init,
		.add		= race_event_add,
		.del		= race_event_del,
		.start		= race_event_start,
		.stop		= race_event_stop,
		.read		= race_event_read,
	};

	test_pmu = tmpl;
}

static int race_event_init(struct perf_event *event)
{
	if (event->attr.type != test_type)
		return -ENOENT;
	return 0;
}
static void race_event_read(struct perf_event *event) { }
static int race_event_add(struct perf_event *event, int flags) { return 0; }
static void race_event_del(struct perf_event *event, int flags) { }
static void race_event_start(struct perf_event *event, int flags) { }
static void race_event_stop(struct perf_event *event, int flags) { }

static int do_register(void)
{
	int ret;

	pmu_reset();
	ret = perf_pmu_register(&test_pmu, "racepmu", -1);
	if (ret) {
		stat_reg_fail++;
		my_registered = 0;
	} else {
		stat_reg_ok++;
		my_registered = 1;
		test_type = test_pmu.type;
	}
	return ret;
}

static int do_unregister(void)
{
	int ret;

	if (!my_registered) {
		stat_unreg_fail++;
		return -EINVAL;
	}
	ret = perf_pmu_unregister(&test_pmu);
	if (ret) {
		stat_unreg_fail++;
	} else {
		stat_unreg_ok++;
		my_registered = 0;
	}
	return ret;
}

static int do_open_one(int cpu)
{
	struct perf_event_attr attr = {};
	struct perf_event *ev;

	if (!my_registered || test_type < 0)
		return -ENODEV;

	attr.size = sizeof(attr);
	attr.type = test_type;
	attr.config = 0;
	attr.disabled = 1;

	ev = perf_event_create_kernel_counter(&attr, cpu % nr_cpu_ids, NULL,
					      NULL, NULL);
	if (IS_ERR(ev)) {
		stat_open_fail++;
		if (open_err0 == 0) open_err0 = (int)PTR_ERR(ev);
		else if (open_err1 == 0 && (int)PTR_ERR(ev) != open_err0) open_err1 = (int)PTR_ERR(ev);
		return PTR_ERR(ev);
	}
	stat_open_ok++;
	perf_event_release_kernel(ev);
	return 0;
}

/*
 * Ungated open attempt: like a real in-flight perf_event_open() that
 * already passed perf_init_event() under SRCU while the PMU is being
 * torn down. No my_registered gate: failures are expected and ignored;
 * the goal is to reach find_get_pmu_context() while/after
 * pmu->cpu_pmu_context is freed.
 */
static void do_open_raw(int cpu)
{
	struct perf_event_attr attr = {};
	struct perf_event *ev;

	if (test_type < 0)
		return;

	attr.size = sizeof(attr);
	attr.type = test_type;
	attr.config = 0;
	attr.disabled = 1;

	ev = perf_event_create_kernel_counter(&attr, cpu % nr_cpu_ids, NULL,
					      NULL, NULL);
	if (!IS_ERR(ev))
		perf_event_release_kernel(ev);
}

struct loop_ctx {
	long n;
	struct completion done;
};

/* R8: thread A - unregister hammer (mirrors uncore_pci_pmu_unregister) */
static int race_unreg_thread(void *data)
{
	struct loop_ctx *c = data;
	long i;

	for (i = 0; i < c->n && !kthread_should_stop(); i++) {
		if (my_registered)
			do_unregister();
		usleep_range(0, 300);
	}
	complete(&c->done);
	return 0;
}

/* R8: thread B - register hammer with template reset (mirrors uncore_pmu_register) */
static int race_reg_thread(void *data)
{
	struct loop_ctx *c = data;
	long i;

	for (i = 0; i < c->n && !kthread_should_stop(); i++) {
		do_register();
		usleep_range(0, 300);
	}
	complete(&c->done);
	return 0;
}

/* R9 core: migrate hammer (mirrors uncore_change_type_ctx) */
static int migrate_thread(void *data)
{
	struct loop_ctx *c = data;
	long i;

	for (i = 0; i < c->n && !kthread_should_stop(); i++) {
		if (my_registered)
			perf_pmu_migrate_context(&test_pmu, 0, 1 % nr_cpu_ids);
		usleep_range(0, 200);
	}
	complete(&c->done);
	return 0;
}

/*
 * R9-specific: migrate hammer kthread racing a SEQUENTIAL
 * unregister/register loop (no concurrent register||unregister, so the
 * R8 tear at perf_pmu_unregister+0x1d3 cannot fire first). The migrate
 * thread keeps calling perf_pmu_migrate_context() while the loop frees
 * and re-creates pmu->cpu_pmu_context.
 */
static int mig_hammer(void *unused)
{
	while (!kthread_should_stop()) {
		perf_pmu_migrate_context(&test_pmu, 0, 1 % nr_cpu_ids);
		cond_resched();
	}
	return 0;
}

static void run_migunreg(long n)
{
	struct task_struct *mig;
	long i;

	if (!my_registered)
		do_register();

	mig = kthread_run(mig_hammer, NULL, "pmu_mig_hammer");
	if (IS_ERR(mig)) {
		pr_err("kthread_run failed\n");
		return;
	}

	for (i = 0; i < n; i++) {
		if (my_registered)
			do_unregister();
		do_register();
		cond_resched();
	}

	kthread_stop(mig);
	/* consume any poisoned end state */
	do_open_one(0);
	do_open_one(1 % nr_cpu_ids);
	pr_info("MIGUNREG_DONE n=%ld reg_ok=%lu reg_fail=%lu unreg_ok=%lu unreg_fail=%lu open_ok=%lu open_fail=%lu\n",
		n, stat_reg_ok, stat_reg_fail, stat_unreg_ok, stat_unreg_fail,
		stat_open_ok, stat_open_fail);
}

/*
 * R9 with live events: a migrate hammer and an open/close hammer run
 * continuously while the main thread does sequential unregister/register
 * rounds. Opened events live long enough for perf_pmu_migrate_context()
 * to take the __perf_pmu_install_event() path, which dereferences
 * pmu->cpu_pmu_context through find_get_pmu_context().
 */
static void run_migopen(long n)
{
	struct loop_ctx o = { .n = 4 * n + 64 };
	struct task_struct *mig, *op;
	long i;

	init_completion(&o.done);
	if (!my_registered)
		do_register();

	mig = kthread_run(mig_hammer, NULL, "pmu_mig_hammer");
	op = kthread_run(open_thread, &o, "pmu_open_hammer");
	if (IS_ERR(mig) || IS_ERR(op)) {
		pr_err("kthread_run failed\n");
		return;
	}

	for (i = 0; i < n; i++) {
		if (my_registered)
			do_unregister();
		do_register();
		/* give the open/migrate hammers a stable registered window */
		usleep_range(4000, 8000);
		cond_resched();
	}

	kthread_stop(mig);
	if (!IS_ERR(op)) {
		/* let the open hammer drain, then stop */
		wait_for_completion(&o.done);
	}
	pr_info("MIGOPEN_DONE n=%ld reg_ok=%lu reg_fail=%lu unreg_ok=%lu unreg_fail=%lu open_ok=%lu open_fail=%lu err0=%d err1=%d\n",
		n, stat_reg_ok, stat_reg_fail, stat_unreg_ok, stat_unreg_fail,
		stat_open_ok, stat_open_fail, open_err0, open_err1);
}

/* R9 core: open/close churn (exercises find_get_pmu_context) */

static int open_thread(void *data)
{
	struct loop_ctx *c = data;
	long i;

	for (i = 0; i < c->n && !kthread_should_stop(); i++) {
		do_open_raw((int)(i % nr_cpu_ids));
		usleep_range(0, 200);
	}
	complete(&c->done);
	return 0;
}

static void run_race(long n)
{
	struct loop_ctx a = { .n = n }, b = { .n = n };
	struct task_struct *ta, *tb;

	init_completion(&a.done);
	init_completion(&b.done);

	/* start from registered state like the real scenario */
	if (!my_registered)
		do_register();

	ta = kthread_run(race_unreg_thread, &a, "pmu_race_unreg");
	tb = kthread_run(race_reg_thread, &b, "pmu_race_reg");
	if (!IS_ERR(ta) && !IS_ERR(tb)) {
		wait_for_completion(&a.done);
		wait_for_completion(&b.done);
	} else
		pr_err("kthread_run failed\n");
	if (!IS_ERR(ta))
		kthread_stop(ta);
	if (!IS_ERR(tb))
		kthread_stop(tb);

	pr_info("RACE_DONE n=%ld reg_ok=%lu reg_fail=%lu unreg_ok=%lu unreg_fail=%lu\n",
		n, stat_reg_ok, stat_reg_fail, stat_unreg_ok, stat_unreg_fail);
}

static void run_churn(long n)
{
	struct loop_ctx a = { .n = n }, b = { .n = n }, c = { .n = n };
	struct loop_ctx d = { .n = n };
	struct task_struct *ta, *tb, *tc, *td;

	init_completion(&a.done);
	init_completion(&b.done);
	init_completion(&c.done);
	init_completion(&d.done);

	if (!my_registered)
		do_register();

	ta = kthread_run(race_unreg_thread, &a, "pmu_churn_unreg");
	tb = kthread_run(race_reg_thread, &b, "pmu_churn_reg");
	tc = kthread_run(migrate_thread, &c, "pmu_churn_mig");
	td = kthread_run(open_thread, &d, "pmu_churn_open");
	if (!IS_ERR(ta) && !IS_ERR(tb) && !IS_ERR(tc) && !IS_ERR(td)) {
		wait_for_completion(&a.done);
		wait_for_completion(&b.done);
		wait_for_completion(&c.done);
		/* consume any poisoned end state */
		do_open_one(0);
		do_open_one(1 % nr_cpu_ids);
	} else
		pr_err("kthread_run failed\n");

	pr_info("CHURN_DONE n=%ld reg_ok=%lu reg_fail=%lu unreg_ok=%lu unreg_fail=%lu open_ok=%lu open_fail=%lu\n",
		n, stat_reg_ok, stat_reg_fail, stat_unreg_ok, stat_unreg_fail,
		stat_open_ok, stat_open_fail);
}

static void run_regloop(long n)
{
	long i;

	for (i = 0; i < n; i++) {
		do_register();
		do_unregister();
		usleep_range(0, 50);
	}
	pr_info("REGLOOP_DONE n=%ld reg_ok=%lu reg_fail=%lu unreg_ok=%lu unreg_fail=%lu\n",
		n, stat_reg_ok, stat_reg_fail, stat_unreg_ok, stat_unreg_fail);
}

static void run_open(long n)
{
	long i;

	for (i = 0; i < n; i++)
		do_open_raw((int)(i % nr_cpu_ids));
	pr_info("OPEN_DONE open_ok=%lu open_fail=%lu\n",
		stat_open_ok, stat_open_fail);
}

static char action_cmd[64];

static void action_workfn(struct work_struct *w)
{
	char cmd[64];
	long n;
	char *p, *cp;

	strscpy(cmd, strstrip(action_cmd), sizeof(cmd));
	cp = cmd;
	p = strsep(&cp, " ");
	n = 0;
	if (cp && cp[0])
		if (kstrtol(cp, 0, &n))
			n = 0;

	if (!strcmp(p, "register"))
		pr_info("register rc=%d type=%d\n", do_register(), test_type);
	else if (!strcmp(p, "unregister"))
		pr_info("unregister rc=%d\n", do_unregister());
	else if (!strcmp(p, "regloop"))
		run_regloop(n);
	else if (!strcmp(p, "race"))
		run_race(n);
	else if (!strcmp(p, "churn"))
		run_churn(n);
	else if (!strcmp(p, "migunreg"))
		run_migunreg(n);
	else if (!strcmp(p, "migopen"))
		run_migopen(n);
	else if (!strcmp(p, "open"))
		run_open(n);
	else if (!strcmp(p, "status"))
		pr_info("status registered=%d type=%d reg_ok=%lu reg_fail=%lu unreg_ok=%lu unreg_fail=%lu open_ok=%lu open_fail=%lu\n",
			my_registered, test_type, stat_reg_ok, stat_reg_fail,
			stat_unreg_ok, stat_unreg_fail, stat_open_ok,
			stat_open_fail);
	else
		pr_info("unknown action '%s'\n", p);

	pr_info("ACTION_COMPLETE\n");
}

static DECLARE_WORK(action_work, action_workfn);

static char action_cmd[64];

static int action_set(const char *val, const struct kernel_param *kp)
{
	int ret = param_set_copystring(val, kp);

	if (ret == 0)
		schedule_work(&action_work);
	return ret;
}

static const struct kernel_param_ops action_ops = {
	.set = action_set,
	.get = param_get_string,
};
static struct kparam_string action_param = {
	.maxlen = sizeof(action_cmd),
	.string = action_cmd,
};
module_param_cb(action, &action_ops, &action_param, 0200);
MODULE_PARM_DESC(action, "register|unregister|regloop N|race N|churn N|open N|status");

static int __init pmu_race_init(void)
{
	pmu_reset();
	pr_info("loaded, nr_cpu_ids=%d\n", nr_cpu_ids);
	return 0;
}

static void __exit pmu_race_exit(void)
{
	cancel_work_sync(&action_work);
	if (my_registered)
		do_unregister();
	pr_info("unloaded reg_ok=%lu reg_fail=%lu unreg_ok=%lu unreg_fail=%lu\n",
		stat_reg_ok, stat_reg_fail, stat_unreg_ok, stat_unreg_fail);
}

module_init(pmu_race_init);
module_exit(pmu_race_exit);
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("perf/PMU race-condition verification driver (R4/R6/R8/R9)");

Re: perf/uncore: activeboxes count TOCTOU lets PCI probe/remove run perf_pmu_register() and perf_pmu_unregister() concurrently on the same struct pmu

[Chen, Zide]

Hi Hanxiang,

Thanks for bringing up this long-standing uncore issue.

Yes, I agree that a lock is needed to serialize uncore PMU registration
and teardown.

It seems perf_pmu_{register,unregister}() expects individual PMU drivers
to manage the lifecycle of struct pmu and won't call
perf_pmu_{register,unregister}() concurrently on the same object.

During unregister's unlocked grace window, a concurrent
perf_pmu_register() reinitializes the PMU context, which the resuming
unregister tail then frees, corrupting the core state.

Intel Uncore is unique in implementing an N-to-1 model where multiple
PMON boxes across different dies share a single struct pmu instance.
Without a lock, concurrent probe and remove races across dies can happen.

I have been working on a series of uncore bug fixes, and this patch will
be included in the patch series that I will post to LKML shortly.

Thanks,
Zide


On 10/22/2026 8:58 AM, 徐晗翔 wrote:
> The uncore PCI driver decides "first active box registers the PMU" and
> "last active box unregisters the PMU" with nothing but the
> pmu->activeboxes counter -- the decision and the perf_pmu_register()/
> perf_pmu_unregister() execution are not serialized -- so on a multi-die
> platform whose same-typed boxes live on different dies, concurrently
> removing the last box of one die and probing a box of another die runs
> register and unregister on the SAME struct pmu at the same time; the
> unregister side's unlocked tail (two grace periods, then idr_remove()
> re-reading pmu->type and perf_pmu_free() reading pmu->dev and
> pmu->cpu_pmu_context) then consumes whatever the concurrent register
> just published, and the kernel oopses or leaves a PMU in the list
> holding freed per-CPU contexts.  The perf-core half of this is
> confirmed dynamically on mainline 7.3 (first concurrency round:
> page fault oops inside perf_pmu_unregister); the uncore-side trigger
> is source-proven on 7.3's current (recently refactored) code, which
> added box refcounting and a PMU_BROKEN state but still no
> serialization of this decision path.
> 
> Both decision sites are counter-only on 7.3 (arch/x86/events/intel/
> uncore.c): uncore_box_setup() at :1153 skips registration when
> atomic_inc_return(&pmu->activeboxes) > 1 (:1166), otherwise calls
> uncore_pmu_register() (:926); uncore_pci_pmu_unregister() at :1283
> calls uncore_pmu_unregister() (:960) when atomic_dec_return(...) == 0
> (:1291).  Meanwhile the perf core is asymmetric: perf_pmu_register()
> (kernel/events/core.c:12836) holds pmus_lock for its whole body
> (guard at :12841), while perf_pmu_unregister() (:13023) holds it only
> for the idr_cmpxchg/list_del section (:13025-13029) and for the final
> idr_remove (:13056-13057) -- between and after them the two
> synchronize_srcu()/synchronize_rcu() grace periods (:13042-13043),
> the idr_remove() that re-reads pmu->type, and perf_pmu_free()
> (:13068, reading pmu->dev/pmu->cpu_pmu_context and freeing them) all
> run unlocked for tens of milliseconds.  Triggering needs a multi-die
> host with id-table uncore PCI drivers (snbep/skx families) and
> concurrent sysfs unbind/rescan (or a hotplug/DPC event racing an
> admin operation) -- root actions, but each entirely legitimate.
> 
> The detailed description:
> 
> * Test environment / affected version
> 
>   Linux version 7.3.0 (gcc 13.3.0, GNU ld 2.42) #1 SMP PREEMPT_DYNAMIC
>   -- vanilla mainline 7.3 release tarball (kernel.org, unmodified),
>   rebuilt with CONFIG_PERF_EVENTS=y, CONFIG_PERF_EVENTS_INTEL_UNCORE=y,
>   KASAN enabled, QEMU/KVM x86_64 guest.  The perf-core functions
>   cited are byte-identical in 7.1.5 and 7.3; the uncore functions were
>   refactored during 7.3 (box refcounting, PMU_BROKEN stickiness,
>   WARN_ON_ONCE against double probe at :1206) -- the refactor did NOT
>   add any lock (the file's lock inventory is unchanged), and the
>   counter-only decision shape survives it.  Line numbers below are
>   from 7.3.
> 
> * Mechanism
> 
>   1. Counter-only decisions, no lock between decision and execution:
> 
>          uncore_box_setup()              /* uncore.c:1153 */
>              ...
>              if (atomic_inc_return(&pmu->activeboxes) > 1)   /* :1166 */
>                      return 0;
>              ret = uncore_pmu_register(pmu);                 /* first
>                                                                 box */
> 
>          uncore_pci_pmu_unregister()    /* uncore.c:1283 */
>              pmu->boxes[die] = NULL;
>              if (atomic_dec_return(&pmu->activeboxes) == 0)  /* :1291 */
>                      uncore_pmu_unregister(pmu);             /* last
>                                                                 box */
> 
>      Two different PCI devices' probe and remove callbacks (per-device
>      locks only; sysfs bind/unbind/rescan are not mutually exclusive)
>      can hold both decisions simultaneously: remove drops die0's last
>      box to 0 and decides "unregister" while probe brings die1's box
>      from 0 to 1 and decides "register" -- same struct intel_uncore_pmu
>      (type->pmus[] is shared across dies for id-table types).
> 
>   2. The register side rewrites the shared struct pmu from scratch:
>      uncore_pmu_register() (:926) assigns the whole template
>      (clearing ->dev, ->type, ->cpu_pmu_context) before calling
>      perf_pmu_register(), which then creates a NEW device, allocates a
>      NEW type id and a NEW percpu context array, and publishes them.
> 
>   3. The unregister side's unlocked tail reads what register just
>      published.  With A = remove(die0) and B = probe(die1):
> 
>        A: perf_pmu_unregister() locks pmus_lock, idr_cmpxchg + list_del
>           (:13025-13029), UNLOCKS, enters two grace periods
>           (:13042-13043)                            -- tens of ms
>        B: uncore_pmu_register() resets the struct, perf_pmu_register()
>           (holding pmus_lock the whole time) creates device D2, type
>           T2, contexts P2; INIT event list; publishes; returns 0
>        A: resumes: idr_remove(&pmu_idr, pmu->type)  /* :13056-13057,
>           re-reads pmu->type == T2 -> deletes B's fresh entry */
>           perf_pmu_free(pmu)                        /* :13068: reads
>           pmu->dev == D2 -> device_del/put; reads pmu->cpu_pmu_context
>           == P2 -> kfrees B's fresh contexts + free_percpu */
> 
>      End states: a PMU still on the pmus list whose IDR entry is gone
>      and whose contexts are freed (later perf_event_open() reaching
>      find_get_pmu_context() walks freed memory), or the mirror
>      interleaving where A's cmpxchg consumes B's registration wholesale
>      (the active PMU vanishes; event creation fails with ENODEV until
>      rebind).  uncore_pmu_unregister() (:960-967) also ignores
>      perf_pmu_unregister()'s return value, so the -EINVAL variant
>      leaves a half-torn state machine in place.
> 
> * Evidence (7.3)
> 
>   - Source-level, uncore side: the decision paths above contain no
>     serialization on current 7.3 (verified function-by-function; the
>     7.3 refactor touched exactly these functions without adding a
>     lock).  Real-hardware trigger: concurrent
>       echo <bdf0> > /sys/bus/pci/drivers/skx_uncore/unbind
>       echo 1     > /sys/bus/pci/rescan
>     on a multi-die host.
>   - Dynamic, perf-core half (reproducer attached, pmu_race.c): its
>     "race" action starts two kthreads faithfully mirroring both sides
>     -- an unregister loop and a register loop that performs the same
>     whole-struct reset uncore_pmu_register() does before each
>     perf_pmu_register(), on the same struct pmu.  On the 4-vCPU KASAN
>     VM the FIRST concurrency round (seconds into the run) ends in:
> 
>         CPU: 0 ... Comm: pmu_race_unreg
>         RIP: 0010:perf_pmu_unregister+0x1cb/0x620
>         #PF: supervisor read access ... address fffffffffffffe08
> 
>     reading a pointer derived from the concurrently rewritten pmu
>     fields; the unregister thread dies and the race never converges.
>     (Same signature on 7.1.5 at +0x1d3; the code between is
>     identical.)  Usage:
> 
>       insmod pmu_race.ko
>       echo "race 2000" > /sys/module/pmu_race/parameters/action
>       echo status > /sys/module/pmu_race/parameters/action
> 
>     QEMU provides no uncore PCI devices, which is why the uncore-side
>     trigger is presented source-level and the perf-core concurrency
>     (the half both sides funnel into) is what the reproducer drives.
> 
> 
> Thanks for taking a look.
> 
> 
> Regards,
> Hanxiang Xu
> xuhx@hust.edu.cn