* [PATCH AUTOSEL 7.0-6.12] 9p/trans_xen: make cleanup idempotent after dataring alloc errors
[not found] <20260428104133.2858589-1-sashal@kernel.org>
@ 2026-04-28 10:41 ` Sasha Levin
0 siblings, 0 replies; only message in thread
From: Sasha Levin @ 2026-04-28 10:41 UTC (permalink / raw)
To: patches, stable
Cc: Yufan Chen, Stefano Stabellini, Dominique Martinet, Sasha Levin,
ericvh, lucho, davem, edumazet, kuba, pabeni, v9fs, netdev,
linux-kernel
From: Yufan Chen <ericterminal@gmail.com>
[ Upstream commit 72cb9ee4f6d80962df17c9763b14e62e28fd85a2 ]
xen_9pfs_front_alloc_dataring() tears down resources on failure but
leaves ring fields stale. If xen_9pfs_front_init() later jumps to the
common error path, xen_9pfs_front_free() may touch the same resources
again, causing duplicate/invalid gnttab_end_foreign_access() calls and
potentially dereferencing a freed intf pointer.
Initialize dataring sentinels before allocation, gate teardown on those
sentinels, and clear ref/intf/data/irq immediately after each release.
This keeps cleanup idempotent for partially initialized rings and
prevents repeated teardown during init failure handling.
Signed-off-by: Yufan Chen <ericterminal@gmail.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Message-ID: <20260324153023.86853-2-ericterminal@gmail.com>
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
Now I have enough information to make a thorough analysis.
## Phase 1: Commit Message Forensics
**Step 1.1 — Subject line:**
- Record: subsystem=`9p/trans_xen`, action verb=`make` (with implicit
"fix"), summary=make cleanup idempotent after dataring alloc errors.
The phrase "make cleanup idempotent" is a classic disguised bug-fix
verb pattern — it's preventing duplicate teardown.
**Step 1.2 — Tags parsing:**
- Record: `Signed-off-by: Yufan Chen` (author), `Reviewed-by: Stefano
Stabellini <sstabellini@kernel.org>` (the original author/maintainer
of trans_xen.c — strong endorsement), `Message-ID:` to lore, `Signed-
off-by: Dominique Martinet <asmadeus@codewreck.org>` (9p maintainer).
No `Fixes:`, no `Cc: stable` (expected — that's why this is being
reviewed). No syzbot, no Reported-by.
**Step 1.3 — Body analysis:**
- Record: Body explains the mechanism precisely —
`xen_9pfs_front_alloc_dataring()` releases resources on failure but
leaves pointer/ref fields stale. If init then jumps to common error
path, `xen_9pfs_front_free()` re-touches them, causing
"duplicate/invalid `gnttab_end_foreign_access()` calls and potentially
dereferencing a freed `intf` pointer". Symptom = double teardown + UAF
on partially initialized rings during init failure.
**Step 1.4 — Hidden bug fix detection:**
- Record: Yes — "make cleanup idempotent" is a textbook hidden bug-fix
subject. The phrase "potentially dereferencing a freed intf pointer"
makes the use-after-free explicit. Cover letter (PATCH v3 0/2) states:
"Patch 1 fixes a potential double-free/Oops during initialization
failure" and "Tested error paths by forcing init failures on non-Xen
systems; dmesg confirms the new sentinel-based cleanup correctly
prevents Oops." So an actual Oops was observed.
## Phase 2: Diff Analysis
**Step 2.1 — Inventory:**
- Record: One file `net/9p/trans_xen.c`, +37/-14 lines, two functions
changed: `xen_9pfs_front_free()` and
`xen_9pfs_front_alloc_dataring()`. Single-file surgical fix, scope =
error path / cleanup only.
**Step 2.2 — Code flow:**
- Record (alloc_dataring): Before — fields are not initialized to
sentinels; on `out:` path, frees `bytes`/`intf` and revokes
`ring->ref` unconditionally without clearing the fields. After —
fields set to NULL/`INVALID_GRANT_REF`/-1 at the top; `out:` only
frees what's set, then clears the fields after each release.
- Record (front_free): Before — uses `if (priv->rings[i].irq > 0)` and
unconditionally calls `gnttab_end_foreign_access(ring->ref, NULL)` and
`free_page(ring->intf)`. After — uses `if (ring->irq >= 0)` then
resets to -1; checks `ring->ref != INVALID_GRANT_REF`; clears
intf/ref/data.in/data.out/irq after each release.
**Step 2.3 — Bug mechanism:**
- Record: This is BOTH (a) error path / resource leak fixes AND (d)
memory safety fixes:
- **Double-free of `ring->intf`**: `xen_9pfs_front_alloc_dataring()`
calls `free_page((unsigned long)ring->intf)` on failure but leaves
the pointer pointing to freed memory. Init then calls
`xen_9pfs_front_free()` whose check `if (!priv->rings[i].intf)
break;` does NOT trip (stale non-NULL pointer), so
`free_page((unsigned long)priv->rings[i].intf)` runs again → kernel
page double-free.
- **Double `gnttab_end_foreign_access` on `ring->ref`**: same path re-
revokes a stale grant ref.
- **Use-after-free of `ring->intf`**: if alloc failed at the
`xenbus_alloc_evtchn` stage, `ring->data.in` was set, then `bytes`
was freed by alloc_dataring's cleanup. On the second pass through
front_free, the `if (ring->data.in)` branch dereferences
`ring->intf->ring_order` and `ring->intf->ref[j]` (already-freed
page) → UAF read; then calls `gnttab_end_foreign_access` on stale
grant refs and `free_pages_exact` on already-freed `data.in`.
**Step 2.4 — Fix quality:**
- Record: Obviously correct — sentinel-based teardown is a standard
idempotent-cleanup pattern. Each release is gated by a sentinel and
the field is invalidated afterward. The change `irq > 0` → `irq >= 0`
is also a defensive correction (with explicit `-1` init, this is the
proper check). No new locking, no new APIs, no behaviour change on the
success path. Regression risk is very low.
## Phase 3: Git History Investigation
**Step 3.1 — Blame:**
- Record: The buggy alloc_dataring code came from `71ebd71921e45`
("xen/9pfs: connect to the backend"), part of v4.12-rc1 (Apr 2017).
Bug has been latent in every kernel since v4.12, so all currently-
supported LTS trees (5.4, 5.10, 5.15, 6.1, 6.6, 6.12, 6.18+) carry it.
**Step 3.2 — Fixes: target:**
- Record: No `Fixes:` tag in the commit. The introducing commit
`71ebd71921e45` is in mainline since v4.12, so it definitely exists in
every active stable tree.
**Step 3.3 — File history:**
- Record: Recent related fixes on this file that are already in stable:
`e43c608f40c06` ("9p/xen: fix release of IRQ"), `7ef3ae82a6ebb`
("9p/xen: fix init sequence"), `ea4f1009408ef` ("9p/xen: Fix UAF in
xen_9pfs_front_remove"), `ce8ded2e61f47` ("9p/xen: protect
xen_9pfs_front_free against concurrent calls"). All are small
stability fixes. The current patch is standalone and not part of a
multi-patch dependent series; series cover letter shows it splits into
2/2 patches but patch 2 (parser cleanup with kstrtouint) is
independent.
**Step 3.4 — Author context:**
- Record: Yufan Chen is a contributor; the patch was reviewed by Stefano
Stabellini who is the original author/long-time maintainer of
`trans_xen.c` (copyright at top of file). Authoritative review.
**Step 3.5 — Dependencies:**
- Record: Uses `INVALID_GRANT_REF`, defined in
`include/xen/grant_table.h` since `bce21a2b48ede` (v5.12-rc3). This
macro is present in all current stable LTS trees (verified in 5.15 —
`#define INVALID_GRANT_REF ((grant_ref_t)-1)` at line 57). No other
dependencies. Self-contained patch.
## Phase 4: Mailing List Research
**Step 4.1 — b4 dig:**
- Record: `b4 dig -c 72cb9ee4f6d80` matched by patch-id, returned `https
://lore.kernel.org/all/20260324153023.86853-2-ericterminal@gmail.com/`
(v3 1/2).
- `b4 dig -a` showed evolution: v1 (single patch, 2026-02-25), v2 (1/4
in mixed series, 2026-02-25), v3 (1/2 in dedicated 9p/trans_xen
series, 2026-03-24). Applied version is the latest.
- v3 cover letter: "Patch 1 fixes a potential double-free/Oops during
initialization failure by making the dataring cleanup idempotent."
Confirms the author treats this as a stability/bug fix.
**Step 4.2 — Reviewers:**
- Record: Reviewed-by Stefano Stabellini (subsystem maintainer), CC'd
Eric Van Hensbergen (ericvh@kernel.org), Lucho Ionkov
(lucho@ionkov.net), and the v9fs list. The right people reviewed it.
**Step 4.3 — Bug report:**
- Record: No external bug report. Bug discovered by code inspection and
confirmed by deliberate fault injection during testing (per the v3
cover letter). No syzbot.
**Step 4.4 — Series context:**
- Record: 2-patch series. Patch 2 ("replace simple_strto* with
kstrtouint") is unrelated parser modernization and not stable
material. This patch (1/2) is fully standalone — no dependency on
patch 2.
**Step 4.5 — Stable list:**
- Record: No prior discussion on stable list found via b4 dig. Author
did not Cc stable, but recent precedent shows similar 9p/xen
idempotency-style fixes (`e43c608`, `7ef3ae82`, `ea4f1009`,
`ce8ded2e`) were backported to 5.15.y, 6.1.y, 6.6.y, 6.12.y as stable-
eligible bug fixes.
## Phase 5: Code Semantic Analysis
**Step 5.1 — Functions modified:**
- Record: `xen_9pfs_front_free()`, `xen_9pfs_front_alloc_dataring()`.
**Step 5.2 — Callers:**
- Record: `xen_9pfs_front_alloc_dataring` is called from
`xen_9pfs_front_init` (in a loop over `XEN_9PFS_NUM_RINGS`).
`xen_9pfs_front_free` is called from `xen_9pfs_front_remove` (xenbus
driver remove callback) AND from `xen_9pfs_front_init` error path.
Critical: both callers are in the device probe/teardown flow, which is
exactly the scenario the patch protects against.
**Step 5.3 — Callees:**
- Record: `gnttab_end_foreign_access`, `free_page`, `free_pages_exact`,
`unbind_from_irqhandler`, `cancel_work_sync`.
`gnttab_end_foreign_access(ref, NULL)` calls into
`gnttab_try_end_foreign_access` → `_gnttab_end_foreign_access_ref` →
indirect into the gnttab interface; reentering with stale ref produces
warnings or worse on backend interaction.
**Step 5.4 — Reachability:**
- Record: Triggered from `xenbus_driver` callback chain when a 9pfs
frontend tries to come up and any of these fails: `get_zeroed_page`
(memory pressure), `gnttab_grant_foreign_access` (grant-table
exhaustion — realistic on busy Xen guests), `alloc_pages_exact`,
`xenbus_alloc_evtchn` (event-channel exhaustion),
`bind_evtchn_to_irqhandler`. Reachable on every 9pfs frontend probe
under resource pressure or hostile/buggy backend.
**Step 5.5 — Similar patterns:**
- Record: Idempotent-cleanup-with-sentinels is the same pattern used
throughout xen frontends. The previous 9p/xen fixes (`e43c608`,
`ce8ded2e`) target the same teardown function and were backported to
stable.
## Phase 6: Cross-Referencing & Stable Tree Analysis
**Step 6.1 — Code presence:**
- Record: Verified by reading `git show
stable/linux-6.6.y:net/9p/trans_xen.c` and `git show
stable/linux-6.12.y:net/9p/trans_xen.c` — both contain the same buggy
`xen_9pfs_front_alloc_dataring()` cleanup pattern and the same
`xen_9pfs_front_free()` un-gated double-teardown. Bug present in 5.4,
5.10, 5.15, 6.1, 6.6, 6.12, 6.18 (all active LTS).
**Step 6.2 — Backport complications:**
- Record: 6.12.y file matches mainline structure almost exactly — minor
context-only deltas. 6.6.y / 6.1.y / 5.15.y use `priv->num_rings`
instead of the constant in the loop and have a slightly different
`xen_9pfs_front_free` outline (no `priv->rings` NULL check at the top
in 6.6) — those need trivial mechanical adjustment.
`INVALID_GRANT_REF` is available in all active LTS. Expected
difficulty: clean-to-minor.
**Step 6.3 — Related fixes already in stable:**
- Record: Verified — `2bb3ee1bf2375` (6.6), `b9e26059664bd` (6.1),
`4950408793b11` (5.15), `530bc9f03a102` (6.12) are the IRQ-double-free
fix; `592fb738d8682`/`91b4763da3ee6`/`db94e06c24cd4`/`e978643c4c9c0`
are the init-sequence fix; `a5d00dff97118` is the concurrent-
front_free protection. None of these address the alloc-failure
idempotency bug — this patch fills a remaining gap.
## Phase 7: Subsystem Context
**Step 7.1 — Subsystem:**
- Record: `net/9p/` — 9P virtual filesystem transport, Xen-specific.
Criticality: PERIPHERAL globally but IMPORTANT for users who actually
use 9P over Xen (e.g., Edera and other Xen-based confidential-
computing/lightweight-VM stacks who recently submitted other 9p/xen
fixes).
**Step 7.2 — Activity:**
- Record: Active subsystem with periodic stability-fix submissions in
2024–2026; multiple recent patches went to stable.
## Phase 8: Impact and Risk
**Step 8.1 — Affected population:**
- Record: Users of Xen 9pfs frontend. Niche but real (Edera, others
using 9p mounts in Xen guests).
**Step 8.2 — Trigger conditions:**
- Record: Failure during second-ring allocation in
`xen_9pfs_front_init`. Triggers include memory pressure, grant-table
exhaustion, evtchn exhaustion, malicious/buggy Xen backend. Not user-
triggerable from unprivileged userspace, but a malicious backend can
deliberately starve the frontend (Xen security model assumes the
backend is more privileged but a frontend should not crash on backend
misbehaviour).
**Step 8.3 — Severity:**
- Record: When triggered → kernel page double-free + grant ref double-
revoke + use-after-free read on a freed page. Failure mode: kernel
oops / panic / memory corruption. Severity: CRITICAL.
**Step 8.4 — Risk-benefit:**
- Record: Benefit = high (eliminates a confirmed Oops on init failure,
idempotent cleanup is universally desirable). Risk = very low — pure
error-path tightening, sentinel-based, no behaviour change on success
path, reviewed by the original author Stefano Stabellini, tested with
deliberate fault injection.
## Phase 9: Final Synthesis
**Evidence FOR backport:**
- Real bug — double-free of kernel page, use-after-free, double grant-
ref revoke during init failure (CRITICAL severity)
- Reproduced (Oops) by author with fault injection in virtme-ng
- Reviewed by the original author/maintainer of the file (Stefano
Stabellini)
- Small, surgical, single file (+37/-14)
- Bug present since v4.12 — affects every active LTS
- All dependencies (`INVALID_GRANT_REF`) present in stable trees
- Recent precedent: 4 similar 9p/xen fixes have been backported to
5.15/6.1/6.6/6.12
- Idempotent cleanup is a textbook stable-friendly pattern, no
behavioural change on success path
**Evidence AGAINST:**
- No `Fixes:` tag, no `Cc: stable` (expected, not a real negative
signal)
- No external user bug report (but author observed Oops during testing)
- Niche subsystem (9p over Xen)
**Stable rules checklist:**
1. Obviously correct and tested? Yes — sentinel pattern, reviewed by
maintainer, fault-injected by author.
2. Real bug? Yes — confirmed Oops.
3. Important issue? Yes — double-free / UAF (CRITICAL).
4. Small and contained? Yes — 51 lines, one file.
5. No new features? Correct — pure cleanup hardening.
6. Applies to stable? Yes for 6.12.y essentially clean; 6.6/6.1/5.15
need trivial loop-variable adjustment.
**Decision:** This is a small, well-reviewed, fault-injection-confirmed
fix for a memory-safety bug (double-free + UAF) that has been latent in
Xen 9p frontend code since 2017 and exists in every active LTS tree. It
matches the pattern of multiple similar 9p/xen stability fixes already
backported to stable. Backport-worthy.
## Verification
- [Phase 1] Read commit message and v3 cover letter from saved mbox
`/tmp/9pxen-thread.mbox` — confirmed "fixes a potential double-
free/Oops during initialization failure" and "Tested error paths by
forcing init failures... dmesg confirms the new sentinel-based cleanup
correctly prevents Oops".
- [Phase 1] Confirmed Reviewed-by from Stefano Stabellini in the mbox
thread.
- [Phase 2] Read full pre-fix `net/9p/trans_xen.c` and post-fix;
manually traced ring-1 alloc failure scenarios at four distinct
failure points and confirmed each leads to either double
`free_page(intf)`, double `gnttab_end_foreign_access(ref)`, or UAF
read of `ring->intf->ring_order`/`ring->intf->ref[j]`.
- [Phase 3] `git log --oneline --follow net/9p/trans_xen.c` showed
`71ebd71921e45` as origin; `git describe --contains 71ebd71921e45` →
`v4.12-rc1~103^2~31`.
- [Phase 3] `git show 71ebd71921e45` confirmed the pre-existing buggy
`xen_9pfs_front_alloc_dataring`+`xen_9pfs_front_free` cleanup pattern
was introduced in 2017.
- [Phase 3] Found prior related fixes (`e43c608`, `7ef3ae82`,
`ea4f1009`, `ce8ded2e`) on the same file with their stable tree
counterparts.
- [Phase 4] `b4 dig -c 72cb9ee4f6d80` returned the lore URL for v3 1/2.
- [Phase 4] `b4 dig -c 72cb9ee4f6d80 -a` showed v1 → v2 → v3 evolution;
applied version is latest.
- [Phase 4] `b4 dig -m /tmp/9pxen-thread.mbox` saved full thread; read
entire mbox with Read tool.
- [Phase 4] No `Cc: stable` and no NAKs in the thread; only Reviewed-by
from Stefano.
- [Phase 5] Confirmed `xen_9pfs_front_alloc_dataring` is called from
`xen_9pfs_front_init` (loop over rings) and `xen_9pfs_front_free` is
called from both `xen_9pfs_front_init` error path and
`xen_9pfs_front_remove`.
- [Phase 5] Read `drivers/xen/grant-table.c` to confirm
`gnttab_end_foreign_access` chain — invoking on a stale ref re-enters
the gnttab interface.
- [Phase 6] Verified `INVALID_GRANT_REF` exists in
`include/xen/grant_table.h` of mainline (line 57) and
`stable/linux-5.15.y` — backport-friendly.
- [Phase 6] Read `stable/linux-6.6.y:net/9p/trans_xen.c` and
`stable/linux-6.12.y:net/9p/trans_xen.c` and confirmed the buggy code
pattern is present in both.
- [Phase 6] Verified previous 9p/xen fixes were backported to
5.15/6.1/6.6/6.12 stable branches via `git log <branch> --
net/9p/trans_xen.c`.
- [Phase 8] Failure mode verified by manual trace: double-free of a
kernel page + use-after-free read on stale `ring->intf` + double
grant-revoke → CRITICAL.
- UNVERIFIED: No external bug report or syzbot reproducer; severity
rests on author's fault-injection result and direct code analysis
(both consistent with each other).
**YES**
net/9p/trans_xen.c | 51 +++++++++++++++++++++++++++++++++-------------
1 file changed, 37 insertions(+), 14 deletions(-)
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index 47af5a10e9212..85b9ebfaa17a6 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -283,25 +283,33 @@ static void xen_9pfs_front_free(struct xen_9pfs_front_priv *priv)
cancel_work_sync(&ring->work);
- if (!priv->rings[i].intf)
+ if (!ring->intf)
break;
- if (priv->rings[i].irq > 0)
- unbind_from_irqhandler(priv->rings[i].irq, ring);
- if (priv->rings[i].data.in) {
- for (j = 0;
- j < (1 << priv->rings[i].intf->ring_order);
+ if (ring->irq >= 0) {
+ unbind_from_irqhandler(ring->irq, ring);
+ ring->irq = -1;
+ }
+ if (ring->data.in) {
+ for (j = 0; j < (1 << ring->intf->ring_order);
j++) {
grant_ref_t ref;
- ref = priv->rings[i].intf->ref[j];
+ ref = ring->intf->ref[j];
gnttab_end_foreign_access(ref, NULL);
+ ring->intf->ref[j] = INVALID_GRANT_REF;
}
- free_pages_exact(priv->rings[i].data.in,
- 1UL << (priv->rings[i].intf->ring_order +
- XEN_PAGE_SHIFT));
+ free_pages_exact(ring->data.in,
+ 1UL << (ring->intf->ring_order +
+ XEN_PAGE_SHIFT));
+ ring->data.in = NULL;
+ ring->data.out = NULL;
+ }
+ if (ring->ref != INVALID_GRANT_REF) {
+ gnttab_end_foreign_access(ring->ref, NULL);
+ ring->ref = INVALID_GRANT_REF;
}
- gnttab_end_foreign_access(priv->rings[i].ref, NULL);
- free_page((unsigned long)priv->rings[i].intf);
+ free_page((unsigned long)ring->intf);
+ ring->intf = NULL;
}
kfree(priv->rings);
}
@@ -334,6 +342,12 @@ static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev,
int ret = -ENOMEM;
void *bytes = NULL;
+ ring->intf = NULL;
+ ring->data.in = NULL;
+ ring->data.out = NULL;
+ ring->ref = INVALID_GRANT_REF;
+ ring->irq = -1;
+
init_waitqueue_head(&ring->wq);
spin_lock_init(&ring->lock);
INIT_WORK(&ring->work, p9_xen_response);
@@ -379,9 +393,18 @@ static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev,
for (i--; i >= 0; i--)
gnttab_end_foreign_access(ring->intf->ref[i], NULL);
free_pages_exact(bytes, 1UL << (order + XEN_PAGE_SHIFT));
+ ring->data.in = NULL;
+ ring->data.out = NULL;
+ }
+ if (ring->ref != INVALID_GRANT_REF) {
+ gnttab_end_foreign_access(ring->ref, NULL);
+ ring->ref = INVALID_GRANT_REF;
+ }
+ if (ring->intf) {
+ free_page((unsigned long)ring->intf);
+ ring->intf = NULL;
}
- gnttab_end_foreign_access(ring->ref, NULL);
- free_page((unsigned long)ring->intf);
+ ring->irq = -1;
return ret;
}
--
2.53.0
^ permalink raw reply related [flat|nested] only message in thread