Re: [PATCH] cxl/port: Validate decoder nr_targets against supported ways

[Alison Schofield <alison.schofield@intel.com>]

On Wed, Aug 13, 2025 at 05:57:50PM -0700, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > nr_targets is the number of downstream ports in a decoder's target
> > list. It is limited to the supported interleave ways: 1, 2, 3, 4, 6,
> > 8, 12, 16. The existing check only enforces nr_targets <=
> > CXL_DECODER_MAX_INTERLEAVE (16), allowing invalid values, like 5, 7,
> > etc, to pass through.
> > 
> > Add is_nr_targets_valid() to check for supported interleave ways using
> > ways_to_eiw(). Use it before allocating a decoder to avoid alloc/free
> > churn when nr_targets is already known to be invalid.
> > 
> > This was found by inspection and hardens the code.
> 
> Well, now seeing this and trying to answer the risk question in 1/ led
> to me to reconsider whether this is a good idea and whether we have a
> deeper confusion at play:
> 
> 1/ "Hardening" needs to come with an actual risk claim. For
>    cxl_switch_decoder_alloc() the values come from hardware or ACPI and if
>    those are buggy all bets are off. ACPI is already protected by a
>    eiw_to_ways() conversion prior to calling cxl_root_decoder_alloc(), and
>    hardware is even more accurate than this
>    approach because it considers interleave ways capability.

If all callers comes thru ACPI then agree no need. When you voiced
concern over valid host bridge interleave ways, which would be the
nr_targets of the root decoder (a switch decoder) I imagined you
were thinking of a path to root decoder alloc outside of ACPI.
Guess not.

> 
> 2/ nr_targets != ways. ways_to_eiw() is the wrong validity check for
>    switch decoders, nr_targets is a topology limitation, not a
>    interleave_ways limitation. As far as I can see nothing breaks if a
>    decoder claims to support a target count that is not a valid
>    interleave_ways number.

nr_targets does not have to equal interleave ways for the ways_to_eiw()
to be a useful helper for validating nr_targets. nr_targets has to be one
of 1,2,3,4,6,8,12,16 and if it's not ways_to_eiw() fails. I saw it as
simple code reuse.

I explored using cxl/test, changing nr_targets at decoder init time,
and predictable passes if greater than needed, and fails when less.
But it's not a thorough simulation and not worthy of a more detailed
simulation.

Thanks for the review. It was a worthwhile exploration :)

> 
> So it was useful to go through this to get that clarified, but the
> takeaway from 1/ is "there is insufficient risk" and the takeaway from
> 2/ is "nothing breaks even if switch decoder target count is 'invalid'".
> The valid interleave_ways is a separate region-only property.