Re: [PATCH 02/15] cxl/core: Check physical address before mapping it in devm_cxl_iomap_block()

[Robert Richter <rrichter@amd.com>]

On 16.09.22 11:04:01, Dan Williams wrote:
> Robert Richter wrote:
> > On 07.09.22 22:48:57, Dan Williams wrote:
> > > Robert Richter wrote:
> > > > The physical base address of a CXL range can be invalid and is then
> > > > set to CXL_RESOURCE_NONE. Early check this case before mapping a
> > > > memory block in devm_cxl_iomap_block().
> > > > 
> > > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > > ---
> > > >  drivers/cxl/core/regs.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> > > > index 39a129c57d40..f216c017a474 100644
> > > > --- a/drivers/cxl/core/regs.c
> > > > +++ b/drivers/cxl/core/regs.c
> > > > @@ -165,6 +165,9 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> > > >  	void __iomem *ret_val;
> > > >  	struct resource *res;
> > > >  
> > > > +	if (addr == CXL_RESOURCE_NONE)
> > > > +		return NULL;
> > > > +
> > > >  	res = devm_request_mem_region(dev, addr, length, dev_name(dev));
> > > >  	if (!res) {
> > > >  		resource_size_t end = addr + length - 1;
> > > > -- 
> > > > 2.30.2
> > > > 
> > > 
> > > devm_request_mem_region() succeeds for you when this happens? More
> > > details about the failure scenario please.
> > 
> > No, CXL_RESOURCE_NONE (all FFs) is used as address. A broken range is
> > calculated that even overflows. I only vaguely remember the exact
> > error message.
> > 
> > This may happen e.g. if the Component Register Block is missing in the
> > DVSEC. cxl_find_regblock() may fail then and returns
> > CXL_RESOURCE_NONE. There are a couple of code paths there
> > component_reg_phys is set to CXL_RESOURCE_NONE without exiting
> > immediately.
> > 
> > I saw it during code development, when I pre-inititalized a port with
> > component_reg_phys set to CXL_RESOURCE_NONE. Since that case can
> > generally happen, I think it must be checked.
> 
> I think Jonathan had it right when we posited that the code should
> probably have failed before getting to this point. For example, the
> scenarios where the driver looks for a component register block via the
> register locator DVSEC are not valid for RCDs in the first instance.

I am ok having the code reviewed to prevent such situations. But the
handling of component_reg_phys is by far not trivial as there are many
locations it is initialized and others where it is used. Call chain is
across multiple functions using various data sources for
component_reg_phys, so it is hard to proof this may never happen.
E.g. in add_port_attach_ep() I found this:

	component_reg_phys = find_component_registers(uport_dev);
	port = devm_cxl_add_port(&parent_port->dev, uport_dev,
        	component_reg_phys, parent_dport);

find_component_registers() and subsequent functions (e.g.
cxl_regmap_to_base()) may return CXL_RESOURCE_NONE. But it is written
to port without any further check in cxl_port_alloc():

	port->component_reg_phys = component_reg_phys;

It is then later directly used in devm_cxl_setup_hdm() to map io
ranges with devm_cxl_iomap_block().

Just an example, I did not go through other code paths.

IMO, the check is cheap and prevents weird follow up errors and log
messages. But I would be also ok, to just drop the patch. There are no
dependencies to this change. What do you think?

Thanks,

-Robert