Re: [PATCH] PCI: iproc: fix resource allocation for BCMA PCIe

[Bjorn Helgaas]

On Wed, Feb 08, 2017 at 05:07:27PM -0500, Abylay Ospan wrote:
> Hi Bjorn,
> 
> I have checked first listed driver
> (drivers/pci/host/pcie-designware.c). Seems like into
> 'devm_request_pci_bus_resources' we supply same stack allocated 'res'
> (actual insert of this pointer to 'iomem_resource' was done inside
> '__request_resource'). This 'res' is not changed inside
> 'of_pci_get_host_bridge_resources'.
> I don't have this platforms on hand and cannot test it on real
> hadrware (to 100% verify). But investigating this code I see that the
> problem exist.
> 
> Here is a summary of flow for 'res' to show the problem:
> 
> pcie-designware.c:
>   LIST_HEAD(res);
>   ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res,
> &pp->io_base); <--- 'res' not changing here
>   ret = devm_request_pci_bus_resources(&pdev->dev, &res);
> 
>   drivers/pci/bus.c:
>     err = devm_request_resource(dev, parent, res);
> 
>     kernel/resource.c:
>       conflict = request_resource_conflict(root, new);
>         conflict = __request_resource(root, new);
>           *p = new;  <--- here we introduce stack allocated res into
> global 'iomem_resource'
> 
> 
> Please check and correct me if i'm wrong ?

The "res" in dw_pcie_host_init() is a list_head (not a struct
resource) and is on the stack.

When we call of_pci_get_host_bridge_resources(), we pass a pointer
("&res") to the empty list.  It kzallocs a struct resource for the bus
range and more for any bridge windows, and adds them to the list.

When we call devm_request_pci_bus_resources(), we pass a pointer
("&res") to the list, which is no longer empty.  It iterates through
the list and calls devm_request_resource() for each resource.  Inside
devm_request_pci_bus_resources(), "res" is the pointer to the resource
(not the list_head), and this resource is the one we kzalloc'd above.

When devm_request_resource() calls request_resource_conflict(), it
passes that pointer to the kzalloc'd resource (the pointer is called
"new" in devm_request_resource()).)

So when __request_resource() assigns "*p = new", it is copying a
pointer to a kzalloc'd struct resource.

This is certainly a twisty maze of similar names for different things,
but I think it is OK if the list_head is on the stack as long as the
struct resources are kzalloc'd.

> >   dw_pcie_host_init
> >     LIST_HEAD(res)                            # on stack
> >     of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &pp->io_base)
> >       res = kzalloc()                         # different "res" from above!
> >       pci_add_resource_offset(resources, res, ...)
> >     devm_request_pci_bus_resources(&pdev->dev, &res)
> >     pci_scan_root_bus(pp->dev, pp->root_bus_nr, &dw_pcie_ops, pp, &res)
> >     error:
> >       pci_free_resource_list(&res)
> 
> > This looks good to me, but I don't think it's necessary to keep the
> > list_head in the struct iproc_pcie.  It should be safe to use
> > "LIST_HEAD(res)" on the stack like the other drivers do.  Can you
> > verify that and get an ack from Ray, Scott, or Jon?
> 
> if my investigation above is true then we need to keep 'res' all the
> time we working with the driver (or find another way to fix this
> issue).
> 
> -- 
> Abylay Ospan,
> NetUP Inc.
> http://www.netup.tv