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

[Alison Schofield]

On Tue, Sep 15, 2026 at 04:11:27PM +0800, Richard Cheng wrote:
> On Thu, Sep 03, 2026 at 10:20:07PM +0800, Alison Schofield wrote:
> > 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
> >
> 
> Hi Alison,
> 
> Thanks a lot for your detailed reviewed.
> 
> I have a few questions to discuss with you before I sent another patch series
> for poison rework.
> 
> So for patch 5's issue, if we get RAM query error and PMEM query error at the same
> time, which return value would you prefer ? I think retained all the record in
> some data structure wouldn't be an issue, just the return error, you would like to see
> the first error or the last ?

Hi Richard,

Neither! Changed my mind after a closer look and seeing that all the query sites
are stopping on first error, so your patch, as is, is inline with other queries.

However, I do think we could do better than that, but that would be beyond the
scope of this fixup patch set. We could change our policy to always query both
partitions and return the first error rather than stopping on it.

So I think you are heading to leaving Patch 5 as is, fixing up Patch 6 and taking
a done on this Sashiko poison 'fixups' set.

-- Alison




> 
> and for patch 6, I see it and will addressed it in the new series.
> 
> Best regards,
> Richard Cheng.
>  
> > > 
> > > 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
> > >