Re: [PATCH 4/5] cxl/port: Fix use-after-free, permit out-of-order decoder shutdown

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

Zijun Hu wrote:
> On 2024/10/15 03:32, Dan Williams wrote:
> > Zijun Hu wrote:
> >> On 2024/10/13 06:16, Dan Williams wrote:
> >>> Zijun Hu wrote:
> >>> [..]
> >>>>>> it does NOT deserve, also does NOT need to introduce a new core driver
> >>>>>> API device_for_each_child_reverse_from(). existing
> >>>>>> device_for_each_child_reverse() can do what the _from() wants to do.
> >>>>>>
> 
> [snip]
> 
> >>> Introduce new superset calls with the additional parameter and then
> >>> rewrite the old routines to just have a simple wrapper that passes a
> >>> NULL @start parameter.
> >>>
> >>> Now, if Greg has the appetite to go touch all ~370 existing callers, so
> >>> be it, but introducing a superset-iterator-helper and a compat-wrapper
> >>> for legacy is the path I would take.
> >>>
> >>
> >> current kernel tree ONLY has 15 usages of
> >> device_for_each_child_reverse(), i would like to
> >> add an extra parameter @start as existing
> >> (class|driver)_for_each_device() and bus_for_each_(dev|drv)() do
> >> if it is required.
> > 
> > A new parameter to a new wrapper symbol sounds fine to me. Otherwise,
> > please do not go thrash all the call sites to pass an unused NULL @start
> > parameter. Just accept that device_for_each_* did not follow the
> > {class,driver,bus}_for_each_* example and instead introduce a new symbol
> > to wrap the new functionality that so far only has the single CXL user.
> > 
> 
> you maybe regard my idea as a alternative proposal if Greg dislike
> introducing a new core driver API. (^^)

If the proposal is to add an unwanted parameter to existing call sites
then yes, I would NAK that.

> > [..]
> >>> If I understand your question correctly you are asking how does
> >>> device_for_each_child_reverse_from() get used in
> >>> cxl_region_find_decoder() to enforce in-order allocation?
> >>>
> >>
> >> yes. your recommendation may help me understand it.
> >>
> 
> below simple solution should have same effect as your recommendation.
> also have below optimizations:
> 
> 1) it don't need new core API.
> 2) it is more efficient since it has minimal iterating.
> 
> i will submit it if you like it. (^^)

See the patch I just submitted, it does not handle the case of competing
allocations. The cxld->region check is not sufficient for determining
that the decoder is committed.