Re: [PATCH v7 0/7] cxl: Sashiko bug fixes

[Alison Schofield]

On Wed, Sep 02, 2026 at 01:38:32PM +0800, Richard Cheng wrote:
> Seven independent, pre-existing bugs in the CXL core, reported by
> sashiko.

Hi Richard,

I finally got around to looking into all this poisonous code ;)
because I had an inkling that they couldn't be independent nor
coincidence based on that common Fixes Tag. That actually is
the issue, aka the finding.

be5cbd084027 is described as an enum removal. Mostly it did that,
CXL_DECODER_PMEM became CXL_PARTMODE_PMEM and nothing else moves. 
But three functions were not renames, they were rewritten.

  cxl_get_poison_by_memdev()	two if blocks became one loop
  cxl_get_poison_unmapped()	rewritten, no line survives
  poison_by_decoder()		gained an unguarded part[] index

If you line up your patches against the pre-rewrite code you'll see
that each one is putting back a statement the rewrite dropped:

  patch 2   'if (!length) return 0' was correct because pmem was the
            last partition.  It became a mid-loop break.
  patch 3   the RAM branch cleared rc on a tolerated -EFAULT.  The
            rewrite continues without clearing it.
  patch 4   the function consumed ctx->offset.  The rewrite derives
            the resume point from the resource child list instead --
            note poison_by_decoder() still computes ctx->offset and
            nothing reads it.  That dead store is the tell.
  patch 5   'if (rc) return rc' lived inside the pmem if block.  When
            the two blocks were merged into a loop it went with the
            block and was never re-added.
  patch 6   be5cbd084027 added six new cxlds->part[cxled->part]
            dereferences and guarded five of them against part < 0.
            In cxl_region_attach() it reordered the existing checks to
            get the guard ahead of the index.  poison_by_decoder() is
            the one site it missed.

BTW - I did confirm it was not a merge issue.

Here's what I'm thinking. DaveJ can pluck patch 1 and 7 separately and
you can take another pass at this as a 'Poison Repair Set'. 

I walked cxl_get_poison_unmapped() against the pre-be5cbd084027 form, but
not the other 2 rewritten functions. It would be good if you can do that.
so we can be sure nothing else is missing. Doing that, along with
addressing Sashiko citings, will get us to the finish line on this one.

I'm going to reply inline to Patches 5 and 6, but here is the
highlights:

- Patches 2, 3 and 4 together restore cxl_get_poison_unmapped()
  and improve upon it. It is partition-generic now and no longer issues
  a zero-length Get Poison List. 

- Patch 5 fixes the masked error but loses the poison records that the
  pre-rewrite code collected before it reported the failure.

- Patch 6 fixes the out-of-bounds read by failing the whole scan for
  the memdev, where the pre-rewrite code scanned that decoder and
  carried on.

-- Alison

> 
> Patch 1: Get/Set Feature derive each mailbox command's offset from the
> starting offset plus the amount of data already transferred, then store
> it in a 16-bit field. A large offset/count supplied through fwctl can
> cause a later offset to exceed the representable feature extent and be
> truncated by cpu_to_le16(), targeting the wrong feature data. Reject
> invalid ranges up front.
> 
> Change cxl_get_feature() to return ssize_t so invalid input and mailbox
> failures are reported as negative errno instead of being conflated with
> a zero-byte result. Update all EDAC callers for the signed return
> contract while preserving the existing fwctl RPC response behavior.
> 
> Patch 2: cxl_get_poison_unmapped() aborted its whole partition sweep on
> the first fully-mapped partition, silently skipping unmapped poison in
> all later partitions. Skip that partition instead.
> 
> Patch 3: the same function tolerated the -EFAULT a RAM partition returns
> for Get Poison List but left it in rc, so a benign fault on the last
> scanned partition surfaced as a spurious read failure. Clear rc, as
> poison_by_decoder() already does.
> 
> Patch 4: the same function also ignored the ctx->offset handoff from
> poison_by_decoder() and derived its scan start from the highest DPA
> allocation, so the DPA of allocated-but-uncommitted decoders was never
> scanned by either phase. Resume the sweep at ctx->offset.
> 
> Patch 5: cxl_get_poison_by_memdev() overwrote rc on each partition
> query, so an earlier partition's failure was masked by a later success
> and unscanned poison was reported as a clean list. Stop on any error
> not tolerated as a RAM -EFAULT.
> 
> Patch 6: poison_by_decoder() assumed every decoder with a DPA reservation
> was assigned to a partition. Malformed device DPA metadata can leave
> dpa_res set while part remains -1, causing a poison scan to access
> before the partition array. Reject such decoders before the lookup.
> 
> Patch 7: the Get and Set Feature fwctl handlers converted all helper
> failures into normal RPC responses, sometimes with a SUCCESS device
> status. Propagate delivery failures as ioctl errors while continuing to
> report actual device errors through rpc_out->retval.
> 
> A nonzero short Get Feature response is valid when Offset + Count runs
> past the end of the feature. Preserve the returned bytes as a successful
> partial transfer, reject unexpected zero-length success responses, and
> require fixed-format EDAC callers to receive their complete attribute
> structures before consuming them.
> 
> Changes since v6 [1]:
> - Patch 7: Remove redundant braces. (Dave Jiang)
> 
> [1]:
> https://lore.kernel.org/linux-cxl/20260826014508.9989-1-icheng@xxxxxxxxxx/
> 
> 
> Richard Cheng (7):
>   cxl/features: Reject feature offset that overflows 16-bit field
>   cxl/region: Scan all partitions for unmapped poison
>   cxl/region: Don't leak tolerated RAM -EFAULT from unmapped poison scan
>   cxl/region: Start unmapped poison scan at the committed decoder
>     boundary
>   cxl/memdev: Don't overwrite the error from an earlier partition poison
>     query
>   cxl/region: Reject poison scan for decoder without a partition
>   cxl/fwctl: Propagate feature RPC delivery errors
> 
>  drivers/cxl/core/core.h     |  8 +++---
>  drivers/cxl/core/edac.c     | 30 +++++++++++++-------
>  drivers/cxl/core/features.c | 56 ++++++++++++++++++++++++-------------
>  drivers/cxl/core/memdev.c   |  2 ++
>  drivers/cxl/core/region.c   | 15 +++++-----
>  5 files changed, 71 insertions(+), 40 deletions(-)
> 
> 
> base-commit: 7098e9cd98a05c0c5de2fae0c2465f9d966fdd07
> -- 
> 2.53.0
>