[PATCH v3] powerpc/pseries: use pci_host_bridge.release_fn() to kfree(phb)

[Andrew Donnellan]

On 11/08/16 16:29, Gavin Shan wrote:
> On Thu, Aug 11, 2016 at 03:01:44PM +1000, Andrew Donnellan wrote:
>> On 11/08/16 07:45, Mauricio Faria de Oliveira wrote:
>>
>> In cxl, we currently call:
>>
>> 	pci_remove_root_bus(phb->bus);
>> 	pcibios_free_controller(phb);
>>
>> which appears to break with this patch after I wire up
>> pci_set_host_bridge_release() in cxl, as phb can be freed before we call
>> pcibios_free_controller().
>>
>
> pcibios_free_controller() isn't called when bridge's release_fn() is wired
> up by pci_set_host_bridge_release(). So you can keep the code as what
> you have and no changes needed.

Sorry, I didn't mean that pci_set_host_bridge_release() calls 
pcibios_free_controller() - I meant that I added a call to 
pci_set_host_bridge_release() in the cxl vPHB setup.

The use-after-free happens after I call pci_remove_root_bus() and then 
call pcibios_free_controller() afterwards.

>
>> It seems slightly wrong for me to write:
>>
>> 	struct pci_bus *bus = phb->bus;
>> 	pcibios_free_controller(phb);
>> 	pci_remove_root_bus(bus);
>>
>
> Yeah, the sequence needs to be reversed. The released PHB instance could
> possibly be accessed inside pci_remove_root_bus().
>
> Thanks,
> Gavin
>
>>
>>>
>>> Test-case:
>>>
>>>  # ls -ld /sys/block/sd* | grep -m1 0021:01:00.0
>>>  <...> /sys/block/sdaa -> ../devices/pci0021:01/0021:01:00.0/<...>
>>>
>>>  # ls -ld /sys/block/sd* | grep -m1 0021:01:00.1
>>>  <...> /sys/block/sdab -> ../devices/pci0021:01/0021:01:00.1/<...>
>>>
>>>  # cat >/dev/sdaa & pid1=$!
>>>  # cat >/dev/sdab & pid2=$!
>>>
>>>  # drmgr -w 5 -d 1 -c phb -s 'PHB 33' -r
>>>  Validating PHB DLPAR capability...yes.
>>>  [  479.547020] pci_hp_remove_devices: PCI: Removing devices on bus 0021:01
>>>  [  479.547049] pci_hp_remove_devices:    Removing 0021:01:00.0...
>>>  ...
>>>  [  483.536303] pci_hp_remove_devices:    Removing 0021:01:00.1...
>>>  ...
>>>  [  497.072130] pci_bus 0021:01: busn_res: [bus 01-ff] is released
>>>  [  497.072209] rpadlpar_io: slot PHB 33 removed
>>>
>>>  # kill -9 $pid1
>>>  # kill -9 $pid2
>>>  [  506.604458] pcibios_host_bridge_release: domain 33, dynamic 1
>>>
>>> Suggested-By: Gavin Shan <gwshan at linux.vnet.ibm.com>
>>> Signed-off-by: Mauricio Faria de Oliveira <mauricfo at linux.vnet.ibm.com>
>>>
>>
>> Missing a '---' here :)
>>
>>> Changelog:
>>> - v3: different approach: struct pci_host_bridge.release_fn()
>>> - v2: different approach: struct pci_controller.refcount
>>> ---
>>> arch/powerpc/include/asm/pci-bridge.h |  2 ++
>>> arch/powerpc/kernel/pci-common.c      | 15 ++++++++++++++-
>>> arch/powerpc/platforms/pseries/pci.c  |  3 +++
>>> 3 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
>>> index b5e88e4..9b11631 100644
>>> --- a/arch/powerpc/include/asm/pci-bridge.h
>>> +++ b/arch/powerpc/include/asm/pci-bridge.h
>>> @@ -54,6 +54,7 @@ struct pci_controller_ops {
>>>  */
>>> struct pci_controller {
>>> 	struct pci_bus *bus;
>>> +	struct pci_host_bridge *bridge; /* associated 'PHB' in PCI subsystem */
>>> 	char is_dynamic;
>>> #ifdef CONFIG_PPC64
>>> 	int node;
>>> @@ -301,6 +302,7 @@ extern void pci_process_bridge_OF_ranges(struct pci_controller *hose,
>>> /* Allocate & free a PCI host bridge structure */
>>> extern struct pci_controller *pcibios_alloc_controller(struct device_node *dev);
>>> extern void pcibios_free_controller(struct pci_controller *phb);
>>> +extern void pcibios_host_bridge_release(struct pci_host_bridge *bridge);
>>>
>>> #ifdef CONFIG_PCI
>>> extern int pcibios_vaddr_is_ioport(void __iomem *address);
>>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>>> index a5c0153..c5b5f60 100644
>>> --- a/arch/powerpc/kernel/pci-common.c
>>> +++ b/arch/powerpc/kernel/pci-common.c
>>> @@ -145,11 +145,23 @@ void pcibios_free_controller(struct pci_controller *phb)
>>> 	list_del(&phb->list_node);
>>> 	spin_unlock(&hose_spinlock);
>>>
>>> -	if (phb->is_dynamic)
>>> +	/* if the associated pci_host_bridge has a release_fn(), rely on that. */
>>> +	if (!phb->bridge->release_fn && phb->is_dynamic)
>>> 		kfree(phb);
>>> }
>>> EXPORT_SYMBOL_GPL(pcibios_free_controller);
>>>
>>> +void pcibios_host_bridge_release(struct pci_host_bridge *bridge)
>>> +{
>>> +	struct pci_controller *phb = (struct pci_controller *) bridge->release_data;
>>> +
>>> +	pr_debug("domain %d, dynamic %d\n", phb->global_number, phb->is_dynamic);
>>> +
>>> +	if (phb->is_dynamic)
>>> +		kfree(phb);
>>> +}
>>> +EXPORT_SYMBOL_GPL(pcibios_host_bridge_release);
>>
>> I figure this is being exported so we can use it in cxl.
>>
>> --
>> Andrew Donnellan              OzLabs, ADL Canberra
>> andrew.donnellan at au1.ibm.com  IBM Australia Limited

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan at au1.ibm.com  IBM Australia Limited