Re: [PATCH v5] cxl/region: Fix wrong logic for finding a free switch cxl decoder

[Dan Williams <dan.j.williams@intel.com>]

Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Provided that all child switch decoders are sorted by ID in ascending
> order, then it is wrong for current match_free_decoder()'s logic to
> find a free cxl decoder as explained below:
> 
> Port
>  ├── cxld A        <---->  region A
>  ├── cxld B        // no region
>  ├── cxld C        <---->  region C
> 
> Current logic will find cxld B as a free cxld, that is wrong since
> region C has not been torn down, so can not regard cxld B as free.
> 
> Fixed by finding a real free clxd by ID instead of region state.
> 
> Link: https://lore.kernel.org/all/66e08f9beb6a2_326462945d@dwillia2-xfh.jf.intel.com.notmuch/
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>

Thanks for this.

Now, on the way to recommending this I had the thought that another way
to do this would be to assume that the next decoder to allocate is
"port->commit_end + 1" and then double check that no previous decoder is
disabled.

I put that idea aside because I did not want to depend on new child
device iteration infrastructure. However, it turns out that since this
suggestion, I have found that the current behavior of
cxl_region_detach(), where it early exits upon detecting out-of-order
shutdown, leads to use after free bugs.

Part of the fix for that is introducing a new
device_for_each_child_reverse_from() helper that is identical to
device_for_each_child_reverse(), but takes a starting child device for
the iteration. With that, this allocator would not need to walk forward
through the list, it could just start at port->commit_end + 1, and then
walk backward to make sure no decoders are idle.

Let me post that fix, Cc: you, and then you can fix this allocation
order validation to use device_for_each_child_reverse_from() rather than
add more decoder_alloc tracking logic to 'struct cxl_port' since
port->commit_end is already sufficient.