From: Rob Clark <[email protected]> [ Upstream commit ae88499d71ce80ecd25c055eddddace108266321 ]
Don't rely on store ordering to protect us from caller seeing a partially initialized vm. Reported-by: Sashiko <[email protected]> Signed-off-by: Rob Clark <[email protected]> Patchwork: https://patchwork.freedesktop.org/patch/743079/ Message-ID: <[email protected]> Signed-off-by: Sasha Levin <[email protected]> --- LLM Generated explanations, may be completely bogus: The local tree is **v7.3-rc4** (mainline `7.3.0-rc4`). This commit is a real ARM memory-ordering fix in lazy GPU VM creation: without acquire/release, a concurrent ioctl can observe a partially initialized VM. The change is small, already in this 7.3 lineage, and is appropriate to keep here. --- ## PHASE 1: COMMIT MESSAGE FORENSICS **Step 1.1: PARSE THE SUBJECT LINE** Record: **[drm/msm] [Fix] [barriers when publishing the lazily created context VM]** **Step 1.2: PARSE ALL COMMIT MESSAGE TAGS** Record: - `Reported-by: Sashiko <[email protected]>` — AI review bot (sashiko.dev), **not** syzbot; static/review finding, no reproducer - `Signed-off-by: Rob Clark` — MSM Adreno GPU maintainer (ignore any Sasha Levin SOB) - `Patchwork: https://patchwork.freedesktop.org/patch/743079/` - `Message-ID: <[email protected]>` — **patch 01/17** of a series (msgid `-2` after cover `-1`) - **No** `Fixes:`, **no** `Cc: stable`, **no** `Reviewed-by` / `Tested- by` / `Acked-by` on the committed message (absence of stable tags is expected) **Step 1.3: ANALYZE THE COMMIT BODY TEXT** Record: - **Bug:** lazy `ctx->vm` publication used a plain store; a concurrent reader could see a non-NULL pointer before VM fields were visible - **Symptom:** caller observes a **partially initialized VM** (GPU IOVA/page-table garbage, kernel deref of uninitialized `drm_gpuvm` fields) - **Version info:** none in the message - **Root cause (author):** “Don't rely on store ordering” — classic double-checked locking without acquire/release. MSM runs on ARM64, where that ordering is **not** guaranteed **Step 1.4: DETECT HIDDEN BUG FIXES** Record: **Not hidden.** Explicit “Fix” plus a documented memory-ordering hole. The `mutex_lock` → `guard(mutex)` swap is incidental cleanup around the real barrier change. --- ## PHASE 2: DIFF ANALYSIS **Step 2.1: INVENTORY THE CHANGES** Record: - File: `drivers/gpu/drm/msm/msm_drv.c` — **+7 / -6** (13-line hunk) - Function: `msm_context_vm()` - Scope: **single-file surgical fix** **Step 2.2: UNDERSTAND THE CODE FLOW CHANGE** Record (before → after): - Fast path: plain `if (ctx->vm) return ctx->vm` → `vm = smp_load_acquire(&ctx->vm); if (vm) return vm` - Slow path: `mutex_lock` / assign `ctx->vm` / `mutex_unlock` → `guard(mutex)` then `smp_store_release(&ctx->vm, vm)` - Path: concurrent first-use of a DRM context (ioctl), not init-only and not an error path **Step 2.3: IDENTIFY THE BUG MECHANISM** Record: **[Synchronization / memory ordering + KCSAN-class data race]** - Double-checked locking: unlocked read of `ctx->vm` vs locked write - Broken mechanism: pointer published with a plain store; reader can use the object before initialization is visible - Fix: standard publication pattern (`smp_store_release` / `smp_load_acquire`) **Step 2.4: ASSESS THE FIX QUALITY** Record: - **Obviously correct** DCL publication - Mutex still serializes creation; acquire/release only publishes the pointer - `guard(mutex)` is equivalent to lock/unlock (cleanup.h) - Regression risk: **low**. Inner `if (!ctx->vm)` remains a plain load **under the mutex** (fine). Return after the slow path is also under the mutex, so mutex acquire pairs with the other thread’s `smp_store_release` - Pre-existing issue **not** introduced here: `msm_gpu_create_private_vm()` can return `ERR_PTR`/`NULL` and that value was already stored; later `00dfa76bdfc2f` added `IS_ERR_OR_NULL` before the release store --- ## PHASE 3: GIT HISTORY INVESTIGATION **Step 3.1: BLAME THE CHANGED LINES** Record: - Lazy VM helper: `6bf32afd37eb6` (“drm/msm: Lazily create context VM”, 2025-06-29) - Unlocked DCL + `init_lock`: `feb8ef4636a457` (“drm/msm: Add opt-in for VM_BIND”, same day) - **First appeared:** **v6.17-rc1** (`git describe --contains` / `git merge-base --is-ancestor 6bf32afd37eb6 v6.17`) - Present for many releases before 7.3 **Step 3.2: FOLLOW THE FIXES: TAG** Record: **No `Fixes:` tag.** Buggy code is the lazy-VM DCL above. Confirmed ancestor of this HEAD. **Step 3.3: CHECK FILE HISTORY FOR RELATED CHANGES** Record (same series, after this commit): - `ea69d489d3a68` — queuelock → `ctxlock` (patch 02/17) - `2c26f9e46d395` — VM creation on `ctxlock` instead of static `init_lock` (patch 03/17); **keeps** the acquire/release from this patch - `00dfa76bdfc2f` — don’t cache `ERR_PTR` in `ctx->vm` (patch 14/17) This patch is **standalone**. Later patches refine locking/errors; they are not prerequisites for the barriers. **Step 3.4: CHECK THE AUTHOR'S OTHER COMMITS** Record: **Rob Clark is the Qualcomm Adreno GPU maintainer** (`MAINTAINERS`: “DRM DRIVER for Qualcomm Adreno GPUs”). He authored the buggy lazy-VM code and this fix. **Step 3.5: CHECK FOR DEPENDENT/PREREQUISITE COMMITS** Record: Needs `msm_context_vm()` / lazy `ctx->vm` (in this tree since 6.17). Uses `guard(mutex)` and `smp_load_acquire`/`smp_store_release` (present in 7.3). **Can apply standalone.** --- ## PHASE 4: MAILING LIST AND EXTERNAL RESEARCH **Step 4.1: FIND THE ORIGINAL PATCH DISCUSSION** Record: - `b4 dig -c ae88499d71ce8` matched patch-id; lore URL `https://patch.ms gid.link/[email protected]` - `b4 dig -a` revisions: v1 (different subject) → **v2–v7 this subject**; applied version is **v7 01/17** (latest) - Cover (v7 00/17): *“The VM is created lazily to give userspace a chance to opt-in to VM_BIND. But Sashiko noticed a couple paths that were not handling this properly.”* - **No NAK** in the saved thread. No human reviewer replies in that mbox (only Sashiko) - Sashiko on this patch: called remaining `ctx->vm` plain reads / `ERR_PTR` caching **pre-existing**, not regressions from this hunk - Direct lore/WebFetch: **403 (Anubis)**; used `b4 dig -m` instead **Step 4.2: CHECK WHO REVIEWED THE PATCH** Record (`b4 dig -w`): To/Cc included `dri-devel`, `linux-arm-msm`, `freedreno`, Sashiko, **Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang, Sean Paul, Marijn Suijten, David Airlie, Simona Vetter**. Right lists/maintainers; **no `Reviewed-by` on the commit**. **Step 4.3: SEARCH FOR THE BUG REPORT** Record: Sashiko is an **AI review bot**, not a fuzzer with a crash log. Cover letter confirms it flagged lazy-VM handling. Severity comes from the **code**, not a user bugzilla. **Step 4.4: CHECK FOR RELATED PATCHES AND SERIES** Record: Started as a small lazy-VM series, grew to **17 patches**. This one stayed patch 1 from v2 onward and does not depend on 02–17. **Step 4.5: CHECK STABLE MAILING LIST HISTORY** Record: lore.kernel.org/stable search **blocked (403)**. **UNVERIFIED** whether stable@ discussed this. No `Cc: stable` on the patch. --- ## PHASE 5: CODE SEMANTIC ANALYSIS **Step 5.1: IDENTIFY KEY FUNCTIONS** Record: `msm_context_vm()`; callee `msm_gpu_create_private_vm()`. **Step 5.2: TRACE CALLERS** Record (this tree): - `msm_ioctl_gem_info_iova` / `msm_ioctl_gem_info_set_iova` — GEM ioctls - `msm_ioctl_gem_submit` — GPU submit (hot path) - `msm_ioctl_vm_bind` — VM_BIND ioctl - `msm_gem_new_handle` — `MSM_BO_NO_SHARE` GEM_NEW - `msm_submitqueue_create` — VM_BIND submitqueue - `adreno_get_param` — GET_PARAM All are **userspace ioctl paths** except destroy/close helpers that read `ctx->vm` after lifetime rules. **Step 5.3: TRACE CALLEES** Record: `msm_gpu_create_private_vm()` may call `gpu->funcs->create_private_vm`, take a pid ref, or fall back to the global VM. Returned object is expected to be fully set up **before** the pointer is published — that is exactly what `smp_store_release` is for. **Step 5.4: FOLLOW THE CALL CHAIN** Record: Unprivileged GPU client (Mesa/VK) opens `/dev/dri/renderD*`, then two threads hit GEM_SUBMIT/GEM_NEW/GET_IOVA before VM exists. **Reachable from userspace. Common** for multi-threaded GL/Vulkan. **Step 5.5: SEARCH FOR SIMILAR PATTERNS** Record: Other `ctx->vm` reads still exist (`msm_gem_close`, `a6xx_set_pagetable`, `msm_submitqueue` destroy, `EN_VM_BIND`). Close/destroy/submit-pagetable run after VM exists or on teardown. Sashiko noted `msm_gem_close` still uses a plain load; that does not make **this** publication barrier wrong. --- ## PHASE 6: CROSS-REFERENCING AGAINST THE LOCAL TREE **Step 6.1: DOES THE BUGGY CODE EXIST IN THIS TREE?** Record: - `git describe HEAD` → **v7.3-rc4-27-gf0100363d8c37**; `make kernelversion` → **7.3.0-rc4** - Buggy DCL **did exist** here (6.17 through 7.2); this commit is **already an ancestor of HEAD** and is in **v7.3-rc1** (`git merge- base --is-ancestor ae88499d71ce8 v7.3-rc1`; **not** in v7.2) - Current `msm_context_vm()` still uses `smp_load_acquire` / `smp_store_release` (later switched lock to `ctxlock`) **Step 6.2: CHECK FOR BACKPORT COMPLICATIONS** Record: Against **current HEAD**, this exact diff would **not** apply (lock is `ctxlock`, plus `IS_ERR_OR_NULL`). Against the **pre-fix** `msm_context_vm()` that this 7.3 series actually shipped from, it applied cleanly (that is how it landed). Difficulty for this series: **clean when needed; already present now**. **Step 6.3: CHECK IF RELATED FIXES ARE ALREADY HERE** Record: This fix plus later series commits (`2c26f9e46d395`, `00dfa76bdfc2f`, …) are already in this checkout. No alternate older barrier fix. --- ## PHASE 7: SUBSYSTEM AND MAINTAINER CONTEXT **Step 7.1: IDENTIFY THE SUBSYSTEM AND ITS CRITICALITY** Record: **drm/msm (Qualcomm Adreno GPU)** — **IMPORTANT** (phones, Chromebooks, embedded), not core mm/vfs, not staging. **Step 7.2: ASSESS SUBSYSTEM ACTIVITY** Record: **Active** (Rob Clark still landing msm fixes on this tree). Bug is in relatively new (6.17) VM_BIND/lazy-VM code, not ancient dead code. --- ## PHASE 8: IMPACT AND RISK ASSESSMENT **Step 8.1: DETERMINE WHO IS AFFECTED** Record: **Driver-specific / config-specific** (`CONFIG_DRM_MSM`), Qualcomm Adreno users. Large production population, not a toy driver. **Step 8.2: DETERMINE THE TRIGGER CONDITIONS** Record: - Trigger: two threads racing first `msm_context_vm()` on the same `drm_file` - Likelihood: **realistic** for multi-threaded GPU clients; first submit/new/iova after open - Unprivileged: **yes** (render node ioctls) **Step 8.3: DETERMINE THE FAILURE MODE SEVERITY** Record: Partially initialized `drm_gpuvm` → kernel oops on VM field access and/or bad GPU pagetable/IOVA programming. **HIGH** (crash and possible GPU mapping corruption). Not a panic in every case, but not cosmetic. **Step 8.4: CALCULATE RISK-BENEFIT RATIO** Record: - **Benefit: high** — closes a real ARM publication race on a hot ioctl path - **Risk: very low** — ~13 lines, standard barriers, no API/ABI change - **Ratio: strongly favors including the fix** --- ## PHASE 9: FINAL SYNTHESIS **Step 9.1: COMPILE THE EVIDENCE** **FOR backporting:** - Real double-checked-locking / weak-memory bug on ARM64 (this driver’s CPU) - Userspace-reachable concurrent ioctl path - Small, obviously correct acquire/release - Maintainer-authored; series reviewed on dri-devel / freedreno - Sashiko flagged the class of lazy-VM mistakes (even though it is not syzbot) - Buggy code existed in this lineage from **v6.17**; 7.3 includes that code - No new feature or uAPI **AGAINST:** - No `Reviewed-by`; no runtime reproducer (AI bot, not syzbot) - Later series patches still touch the same function (not required for this hunk) - Other `ctx->vm` readers still use plain loads (incomplete coverage, not a wrong fix) - lore.kernel.org/stable discussion **unverified** (HTTP 403) **UNRESOLVED:** - Whether anyone nominated this on stable@ (could not fetch lore/stable) - No public crash log; impact inferred from the race, not a filed user report **Step 9.2: APPLY THE STABLE RULES CHECKLIST** 1. Obviously correct and tested? **PASS** — textbook publication barriers; in mainline 7.3; maintainer-authored 2. Fixes a real bug that affects users? **PASS** — concurrent first-use of per-context GPU VM 3. Important issue (crash/security/corruption/deadlock)? **PASS** — **HIGH**: crash and/or GPU mapping corruption on ARM 4. Small and contained? **PASS** — one function, ~13 lines 5. No new features or APIs? **PASS** 6. Can apply to the local tree? **PASS** — `msm_context_vm()` exists here; this is the 7.3 tree the fix landed in **Step 9.3: CHECK FOR EXCEPTION CATEGORIES** Record: **None** (not an ID/quirk/DT/build/docs exception). It qualifies as a normal important bugfix. **Step 9.4: MAKE YOUR DECISION** This tree is **7.3.0-rc4**. The lazy VM and the DCL race are in this series. The patch is a contained memory-ordering fix on a userspace GPU ioctl path. Risk is low; skipping it would leave Qualcomm GPU users exposed to a partially initialized VM. It belongs in this tree’s stable lineage. --- ### What the commit solves for this tree `msm_context_vm()` creates the per-process GPU VM on first use so userspace can opt into VM_BIND. Two threads can hit that helper at once. The fast path used a plain load of `ctx->vm` and the creator used a plain store. On ARM64 that can make a reader see the pointer before the VM object is initialized. `smp_store_release` / `smp_load_acquire` close that hole. ### Stable rules / risk Meets the stable criteria: real, important, small, no new API. Benefit (avoid crash/bad GPU mappings) dwarfs the risk of a 13-line barrier change. ### Concerns - Follow-ups in the same series change the lock and error handling; they are **not** required for this barrier fix to be correct. - Sashiko is an AI reviewer, not syzbot; the race is still real on this hardware. - Applying **this exact diff** to current HEAD would conflict because later 7.3 commits already evolved the function — that is expected; the fix is already in v7.3-rc1. --- Verification: - [Phase 1] Parsed tags: `Reported-by: Sashiko`, Patchwork 743079, msgid `20190-2` (01/17); no Fixes/Cc:stable/Reviewed-by - [Phase 2] Diff: +7/-6 in `msm_context_vm()`; acquire on read, release on publish, `guard(mutex)` - [Phase 2] Compared `git show ae88499d71ce8^` vs `ae88499d71ce8` for that function - [Phase 3] `git blame` on parent: DCL from `feb8ef4636a457` / `6bf32afd37eb6` (2025-06-29) - [Phase 3] `git merge-base --is-ancestor 6bf32afd37eb6 v6.17` → yes; `ae88499d71ce8` in v7.3-rc1, **not** in v7.2 - [Phase 3] Follow-ups `2c26f9e46d395` / `00dfa76bdfc2f` confirmed; they keep/extend barriers, not replace the need for them - [Phase 4] `b4 dig -c ae88499d71ce8` → patch.msgid.link/20260729155609.20190-2-… - [Phase 4] `b4 dig -a`: v2–v7; applied = v7 01/17 - [Phase 4] `b4 dig -w`: MSM maintainers + dri-devel/freedreno CC’d - [Phase 4] `b4 dig -m`: cover letter + Sashiko reply; no NAK; Sashiko issues labeled pre-existing - [Phase 4] WebFetch lore/stable/patchwork: **403 Anubis** — UNVERIFIED via HTTP - [Phase 5] `git grep msm_context_vm`: ioctl callers in msm_drv, msm_gem, msm_gem_submit, msm_gem_vma, msm_submitqueue, adreno_gpu - [Phase 6] `git describe HEAD` = v7.3-rc4-27-gf0100363d8c37; `make kernelversion` = 7.3.0-rc4 - [Phase 6] Current `msm_context_vm()` still has smp_load_acquire/smp_store_release - [Phase 7] MAINTAINERS: Rob Clark maintains drm/msm Adreno - [Phase 8] Failure mode: partially initialized VM → oops / bad GPU maps; severity HIGH - UNVERIFIED: stable@ mailing list discussion (lore 403) **YES** drivers/gpu/drm/msm/msm_drv.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index c2a8aaa33f84a..8094e5e7e9a54 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -226,18 +226,19 @@ struct drm_gpuvm *msm_context_vm(struct drm_device *dev, struct msm_context *ctx { static DEFINE_MUTEX(init_lock); struct msm_drm_private *priv = dev->dev_private; + struct drm_gpuvm *vm = smp_load_acquire(&ctx->vm); /* Once ctx->vm is created it is valid for the lifetime of the context: */ - if (ctx->vm) - return ctx->vm; + if (vm) + return vm; + + guard(mutex)(&init_lock); - mutex_lock(&init_lock); if (!ctx->vm) { - ctx->vm = msm_gpu_create_private_vm( + vm = msm_gpu_create_private_vm( priv->gpu, current, !ctx->userspace_managed_vm); - + smp_store_release(&ctx->vm, vm); } - mutex_unlock(&init_lock); return ctx->vm; } -- 2.53.0
