Linux SCSI subsystem development
 help / color / mirror / Atom feed
From: Sasha Levin <sashal@kernel.org>
To: patches@lists.linux.dev, stable@vger.kernel.org
Cc: Kumar Meiyappan <kumar.meiyappan@microchip.com>,
	Sagar Biradar <sagar.biradar@microchip.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Sasha Levin <sashal@kernel.org>,
	jinpu.wang@cloud.ionos.com,
	James.Bottomley@HansenPartnership.com, mkp@kernel.org,
	linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH AUTOSEL 6.18-5.10] scsi: pm8001: Reject non-fatal dump when controller is crashed
Date: Mon, 31 Aug 2026 09:28:41 -0400	[thread overview]
Message-ID: <20260831133314.4125787-493-sashal@kernel.org> (raw)
In-Reply-To: <20260831133314.4125787-1-sashal@kernel.org>

From: Kumar Meiyappan <kumar.meiyappan@microchip.com>

[ Upstream commit aa3b8f56ef27ed72394a752820abdec4608b731c ]

pm80xx_get_non_fatal_dump() can be called even after the controller has
entered a fatal error state. In that case the forensic memory contents
are not safe to access for a non-fatal dump request, and attempting to
do so can trigger a call trace.

Check controller_fatal_error before reading the non-fatal dump buffer
and return -EINVAL when the controller is already in a crashed state.

This prevents non-fatal dump collection from running in an invalid
controller state.

Signed-off-by: Kumar Meiyappan <kumar.meiyappan@microchip.com>
Signed-off-by: Sagar Biradar <sagar.biradar@microchip.com>
Link: https://patch.msgid.link/20260416154650.415624-1-sagar.biradar@microchip.com
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Phase 1: Commit Message Forensics

### Step 1.1: Subject Line
Record: **[scsi: pm8001]** **[Reject]** — Reject non-fatal forensic dump
collection when the PMC-Sierra PM8001 controller is already in a fatal
error state.

### Step 1.2: Commit Message Tags
Record:
- **Fixes:** — not present (expected for manual review)
- **Reported-by:** — none
- **Tested-by:** — none
- **Reviewed-by:** — none
- **Acked-by:** — none
- **Link:** https://patch.msgid.link/20260416154650.415624-1-
  sagar.biradar@microchip.com
- **Cc: stable@** — present in original patch submission (verified via
  `b4 dig -w`)
- **Signed-off-by:** Kumar Meiyappan, Sagar Biradar, Martin K. Petersen
  (SCSI maintainer)

Notable: authors explicitly CC'd `stable@vger.kernel.org`; no syzbot or
user bug reports.

### Step 1.3: Commit Body Analysis
Record:
- **Bug:** `pm80xx_get_non_fatal_dump()` can run after the controller
  has entered fatal error state.
- **Symptom:** Attempting to read forensic memory in that state can
  trigger a kernel call trace.
- **Root cause:** Missing guard on `controller_fatal_error` before
  initiating non-fatal dump hardware access.
- **Fix approach:** Check `controller_fatal_error` and return `-EINVAL`
  early.
- **Version info:** none in commit message.

### Step 1.4: Hidden Bug Fix Detection
Record: **Yes, this is a straightforward bug fix** disguised as "reject"
rather than "fix", but it clearly prevents unsafe hardware access and
kernel call traces during error recovery.

---

## Phase 2: Diff Analysis

### Step 2.1: Change Inventory
Record:
- **Files:** `drivers/scsi/pm8001/pm80xx_hwi.c` (+7 / -0)
- **Function modified:** `pm80xx_get_non_fatal_dump()`
- **Scope:** Single-file, surgical fix

### Step 2.2: Code Flow Change
Record:
- **Before:** After mapping host forensic buffer pointer, function
  immediately increments `non_fatal_count` and performs MMIO
  writes/reads to the crashed controller (`pm8001_mw32`, `pm8001_mr32`,
  `pm8001_cw32`).
- **After:** If `controller_fatal_error` is true, log debug message and
  return `-EINVAL` before any controller interaction.
- **Path affected:** Sysfs read of `non_fatal_log` via
  `non_fatal_log_show()` → `pm80xx_get_non_fatal_dump()`.

### Step 2.3: Bug Mechanism
Record:
- **Category:** Logic/correctness fix — unsafe hardware access in
  invalid controller state.
- **Mechanism:** After fatal firmware error (`controller_fatal_error =
  true` set in interrupt handler at line 4084), forensic DMA/MMIO
  operations are unsafe; the function lacked the same state check used
  elsewhere in the driver.

### Step 2.4: Fix Quality
Record:
- **Quality:** Obviously correct; mirrors existing driver pattern
  (`pm8001_sas.c` task path, `pm80xx_chip_soft_rst()`).
- **Regression risk:** Very low — only rejects an operation that should
  never succeed on a dead controller.
- **Red flags:** None.

---

## Phase 3: Git History Investigation

### Step 3.1: Blame / Introduction
Record:
- Buggy function exists in local tree at
  `drivers/scsi/pm8001/pm80xx_hwi.c:387-503`.
- Non-fatal dump sysfs feature originally added in `dba2cc03b9db`
  (2020-03-16, "scsi: pm80xx: sysfs attribute for non fatal dump").
- `controller_fatal_error` exists at lines 1445, 1650, 4084 in current
  tree.
- Local autosel tree has squashed history; per-file `git blame` is not
  reliable for introduction dating.

### Step 3.2: Fixes Tag
Record: **N/A** — no `Fixes:` tag present.

### Step 3.3: Related File History
Record:
- Sibling upstream commit `2a8fbcfb04aa9` ("scsi: pm8001: Reject
  firmware update in fatal error state") from same author/date — same
  pattern, different sysfs path; also **not** in current HEAD.
- Standalone v1 patch; not part of a multi-patch series.

### Step 3.4: Author Context
Record: Kumar Meiyappan / Sagar Biradar are Microchip pm8001 driver
authors. Martin K. Petersen (SCSI maintainer) committed the fix upstream
as `aa3b8f56ef27`.

### Step 3.5: Dependencies
Record: **Standalone.** No prerequisite commits required.
`controller_fatal_error` field and `pm80xx_get_non_fatal_dump()` both
exist in this tree. Patch applies at line ~403 with clean context
(verified against upstream diff and local file).

---

## Phase 4: Mailing List and External Research

### Step 4.1: Original Discussion
Record:
- **URL:** https://patch.msgid.link/20260416154650.415624-1-
  sagar.biradar@microchip.com
- **Series:** v1 only (no v2/v3)
- **Maintainer response:** Martin K. Petersen applied to 7.2/scsi-queue
  (May 22, 2026)
- **NAKs/concerns:** None found in thread
- **Stable nomination:** Authors CC'd `stable@vger.kernel.org` in
  original submission

### Step 4.2: Reviewers
Record: CC'd Martin K. Petersen, James Bottomley, Jack Wang, linux-scsi,
stable@, Brian King, Don Brace, and other Microchip engineers.

### Step 4.3: Bug Report
Record: No external bug report, syzbot, or stack trace in commit
message. Issue appears internally discovered by driver vendor during
fatal-error handling review.

### Step 4.4: Related Patches
Record: Related upstream fix `2a8fbcfb04aa9` for
`pm8001_store_update_fw()` — same failure class, separate commit. Not a
dependency for this fix.

### Step 4.5: Stable List History
Record: Patch was posted directly to stable@ (spinics stable archive).
No rejection or prior stable discussion found beyond the submission
itself.

---

## Phase 5: Code Semantic Analysis

### Step 5.1: Key Functions
Record: `pm80xx_get_non_fatal_dump()` (modified), `non_fatal_log_show()`
(caller), fatal error ISR path setting `controller_fatal_error`.

### Step 5.2: Callers
Record:
- `non_fatal_log_show()` in `pm8001_ctl.c:588-591` directly returns
  result.
- Exposed as read-only sysfs host attribute `non_fatal_log` in
  `pm8001_host_attrs[]`.
- Called during diagnostic collection, typically by root/admin tooling
  after controller errors.

### Step 5.3: Callees
Record: `pm8001_mw32()`, `pm8001_mr32()`, `pm8001_cw32()` — MMIO
register access to potentially dead controller; forensic DMA buffer
reads from `FORENSIC_MEM`.

### Step 5.4: Reachability
Record:
- **Trigger chain:** Controller fatal firmware error → ISR sets
  `controller_fatal_error` → admin reads
  `/sys/class/scsi_host/hostN/non_fatal_log` → unsafe MMIO without
  guard.
- **Userspace reachable:** Yes, via sysfs read (requires appropriate
  permissions, typically root/CAP_SYS_ADMIN).

### Step 5.5: Similar Patterns
Record: Driver already guards fatal-error state in:
- `pm8001_sas.c:505` — task execution
- `pm80xx_hwi.c:1650` — soft reset MPI uninit
- `pm8001_sas.c:237-244` — phy control via `fatal_errors()`
- `pm8001_init.c:702-704` — NVMD read

This fix fills a gap in the same error-handling model.

---

## Phase 6: Cross-Reference Against Local Tree

### Step 6.1: Buggy Code Present?
Record: **YES.** Local tree is **v6.18.43** (`git describe HEAD` →
`v6.18.43-1-gc7f0dac02d232`, `make kernelversion` → `6.18.43`).
`pm80xx_get_non_fatal_dump()` at lines 403-404 lacks
`controller_fatal_error` check. Upstream fix commit `aa3b8f56ef27` is
**NOT** an ancestor of HEAD.

### Step 6.2: Backport Complications
Record: **Clean apply expected.** Local file matches upstream pre-fix
context at the insertion point. No conflicting changes in that function
region.

### Step 6.3: Related Fixes Already Present?
Record: **No.** Neither `aa3b8f56ef27` (this fix) nor sibling
`2a8fbcfb04aa9` (firmware update guard) are in HEAD.
`non_fatal_log_show()` directly returns `ssize_t` from dump function, so
`-EINVAL` propagates correctly to sysfs (no need for separate error-code
fix `1b6f03b`).

---

## Phase 7: Subsystem Context

### Step 7.1: Subsystem Criticality
Record: **drivers/scsi/pm8001** — PERIPHERAL driver (Microchip/PMC-
Sierra SAS HBA, `CONFIG_SCSI_PM8001`). Important for deployments using
this hardware, but not core kernel.

### Step 7.2: Subsystem Activity
Record: pm8001 driver is mature; forensic dump sysfs has existed since
2020. Recent upstream activity includes fatal-error handling hardening
from Microchip (April 2026).

---

## Phase 8: Impact and Risk Assessment

### Step 8.1: Who Is Affected
Record: **Driver-specific** — systems with PM8001/PM80xx SAS HBAs and
`CONFIG_SCSI_PM8001` enabled.

### Step 8.2: Trigger Conditions
Record:
- Controller must first enter fatal firmware error state (already a
  serious failure).
- Then sysfs read of `non_fatal_log` (diagnostic path during triage).
- **Likelihood:** Uncommon but realistic during failure investigation —
  exactly when admins collect dumps.
- **Privilege:** Typically root/admin.

### Step 8.3: Failure Mode Severity
Record:
- **Failure mode:** Kernel call trace from unsafe MMIO/DMA on dead
  controller.
- **Severity:** **MEDIUM-HIGH** — secondary kernel instability during
  already-critical failure recovery; not a normal-I/O-path crash, but
  can produce oops/warnings and complicate diagnostics.

### Step 8.4: Risk-Benefit
Record:
- **Benefit:** Prevents additional kernel call traces during HBA failure
  recovery; aligns driver error handling.
- **Risk:** Very low — 7-line guard, no API changes, no behavior change
  on healthy controllers.
- **Ratio:** Favorable for stable backport.

---

## Phase 9: Final Synthesis

### Step 9.1: Evidence Summary

**FOR backport:**
- Real bug with concrete trigger (fatal error + sysfs non_fatal dump
  read)
- Prevents kernel call trace / unsafe hardware access
- Small (7 lines), obviously correct, standalone
- Buggy code confirmed present in v6.18.43 tree
- Consistent with existing `controller_fatal_error` checks in same
  driver
- Authors CC'd stable@; SCSI maintainer merged upstream
- Clean apply expected

**AGAINST backport:**
- Narrow hardware scope (PM8001 SAS HBAs only)
- Only affects diagnostic sysfs path after controller already failed
- No syzbot report or public user bug report
- "Call trace" severity not fully documented with stack trace

**Unresolved:**
- Exact call trace type (WARN vs oops) not documented in commit message
- Introduction date of `controller_fatal_error` not determinable from
  squashed local git history (but field is present in tree)

### Step 9.2: Stable Rules Checklist
1. Obviously correct and tested? **PASS** — logic is clear; maintainer-
   applied; no Tested-by but low complexity.
2. Fixes a real bug? **PASS** — unsafe operation on crashed controller.
3. Important issue? **PASS** — kernel call trace during failure recovery
   (medium-high severity).
4. Small and contained? **PASS** — 7 lines, 1 file, 1 function.
5. No new features/APIs? **PASS** — guard only.
6. Can apply to local tree? **PASS** — code exists, patch applies
   cleanly.

### Step 9.3: Exception Categories
Record: **None** — standard driver bug fix.

### Step 9.4: Decision Rationale
This is a small, vendor-submitted fix for a real gap in fatal-error
handling. The buggy code is present in the local **6.18.43** tree, the
fix is minimal and follows established driver patterns, and it prevents
additional kernel instability during exactly the scenario where
administrators are collecting diagnostic data from a failed HBA. The
risk is negligible and the authors explicitly nominated it for stable.

---

## Verification

- [Phase 1] Parsed subject, tags; confirmed stable@ CC in original
  submission via `b4 dig -w`
- [Phase 1] No Fixes:/Reported-by/syzbot tags
- [Phase 2] Read full `pm80xx_get_non_fatal_dump()` (lines 387-503);
  confirmed MMIO access after missing guard
- [Phase 2] Diff is +7 lines adding `controller_fatal_error` check
  returning `-EINVAL`
- [Phase 3] `git describe HEAD` → v6.18.43; `make kernelversion` →
  6.18.43
- [Phase 3] `git merge-base --is-ancestor aa3b8f56ef27 HEAD` →
  NOT_IN_HEAD (fix not yet applied)
- [Phase 3] `git show aa3b8f56ef27^` at lines 400-415 confirms buggy
  pre-fix code
- [Phase 3] `dba2cc03b9db` identified as 2020 introduction of non_fatal
  dump sysfs
- [Phase 3] Sibling commit `2a8fbcfb04aa9` found (fw update guard); also
  NOT_IN_HEAD
- [Phase 4] `b4 dig -c aa3b8f56ef27` → lore URL found
- [Phase 4] `b4 dig -a` → v1 only, no later revisions
- [Phase 4] `b4 dig -w` → stable@, Martin Petersen, James Bottomley CC'd
- [Phase 4] `b4 dig -m /tmp/pm8001_thread.mbox` → Martin Petersen
  "Applied to 7.2/scsi-queue"
- [Phase 5] `grep pm80xx_get_non_fatal_dump` → caller
  `non_fatal_log_show()` in pm8001_ctl.c:591
- [Phase 5] `grep controller_fatal_error` → used in pm80xx_hwi.c,
  pm8001_ctl.c, pm8001_sas.c, pm8001_sas.h
- [Phase 5] Fatal error set at pm80xx_hwi.c:4084 in interrupt handler
- [Phase 6] Buggy code confirmed at pm80xx_hwi.c:403-404 without guard
- [Phase 6] `non_fatal_log_show()` directly returns ssize_t — `-EINVAL`
  propagation works
- [Phase 8] CONFIG_SCSI_PM8001 confirmed in drivers/scsi/Kconfig:1494
- [UNVERIFIED] Exact kernel version when `controller_fatal_error` was
  introduced (squashed local history)
- [UNVERIFIED] Full stack trace / oops type from original bug report
  (not in commit message)

**YES**The background check finished: `aa3b8f56ef27` is **not** in this
tree (`v6.18.43`), and the buggy code is still present in
`pm80xx_get_non_fatal_dump()`.

The backport analysis stands: **YES** — this is a small, low-risk guard
that prevents kernel call traces when reading `non_fatal_log` on a
controller already in fatal error state.

 drivers/scsi/pm8001/pm80xx_hwi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index 31960b72c1e92..1ce8226fcd292 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -401,6 +401,13 @@ ssize_t pm80xx_get_non_fatal_dump(struct device *cdev,
 	char *buf_copy = buf;
 
 	temp = (u32 *)pm8001_ha->memoryMap.region[FORENSIC_MEM].virt_ptr;
+
+	if (pm8001_ha->controller_fatal_error) {
+		pm8001_dbg(pm8001_ha, FAIL,
+			   "non-fatal dump not available in fatal error state\n");
+		return -EINVAL;
+	}
+
 	if (++pm8001_ha->non_fatal_count == 1) {
 		if (pm8001_ha->chip_id == chip_8001) {
 			snprintf(pm8001_ha->forensic_info.data_buf.direct_data,
-- 
2.53.0


  parent reply	other threads:[~2026-08-31 13:48 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20260831133314.4125787-1-sashal@kernel.org>
2026-08-31 13:20 ` [PATCH AUTOSEL 6.18-6.1] scsi: core: Do not block on tag allocation in scsi_eh_lock_door() Sasha Levin
2026-08-31 13:26 ` [PATCH AUTOSEL 6.18-5.10] scsi: bfa: Reduce kernel stack usage in bfa_fcs_lport_fdmi_build_portattr_block() Sasha Levin
2026-08-31 13:28 ` Sasha Levin [this message]
2026-08-31 16:47   ` [PATCH AUTOSEL 6.18-5.10] scsi: pm8001: Reject non-fatal dump when controller is crashed sashiko-bot
2026-08-31 13:29 ` [PATCH AUTOSEL 6.18-5.10] scsi: pm8001: Reject firmware update in fatal error state Sasha Levin
2026-08-31 17:05   ` sashiko-bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260831133314.4125787-493-sashal@kernel.org \
    --to=sashal@kernel.org \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=jinpu.wang@cloud.ionos.com \
    --cc=kumar.meiyappan@microchip.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=mkp@kernel.org \
    --cc=patches@lists.linux.dev \
    --cc=sagar.biradar@microchip.com \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox