Re: [PATCH] PCI: pnv_php: Use common error handling code in pnv_php_alloc_slot()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The patch introduces spaghetti code by placing an error cleanup label inside the body of an intermediate 'if' block.

Pre-existing issues:
- [High] Calling SLOT_WARN() when alloc_workqueue() fails causes a NULL pointer dereference because php_slot->bus is uninitialized.
--

commit fc5acb6d5ef23950f12a496bbb0ba6bddebe9f87
Author: Markus Elfring <elfring@users.sourceforge.net>

PCI: pnv_php: Use common error handling code in pnv_php_alloc_slot()

This commit introduces a new goto label to reuse error handling when allocating
a PCI hotplug slot in pnv_php.

> diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
> index ff92a5c301b81..e448f18020026 100644
> --- a/drivers/pci/hotplug/pnv_php.c
> +++ b/drivers/pci/hotplug/pnv_php.c
[ ... ]
> @@ -791,16 +791,15 @@ static struct pnv_php_slot *pnv_php_alloc_slot(struct device_node *dn)
[ ... ]
>  	/* Allocate workqueue for this slot's interrupt handling */
>  	php_slot->wq = alloc_workqueue("pciehp-%s", WQ_PERCPU, 0, php_slot->name);
>  	if (!php_slot->wq) {
>  		SLOT_WARN(php_slot, "Cannot alloc workqueue\n");

[Severity: High]
This is a pre-existing issue, but can this call to SLOT_WARN() cause a NULL
pointer dereference?

Looking at pnv_php_alloc_slot(), php_slot->pdev and php_slot->bus are not
initialized until much later in the function. Since the structure is
kzalloc'd, SLOT_WARN() will evaluate with a NULL php_slot->pdev and fall back
to dev_warn(&(sl)->bus->dev, ...).

Because php_slot->bus is also NULL here, this calculates an offset from a NULL
pointer and passes it to dev_warn(), which will crash when it attempts to read
the device name.

>  		kfree(php_slot->name);
> +free_php_slot:

[Severity: Low]
Does placing a cleanup label inside an intermediate if block obscure the
control flow? 

Standard kernel coding style typically places cleanup labels sequentially at
the end of the function rather than embedding them inside failure paths.

>  		kfree(php_slot);
>  		return NULL;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5cd04727-44c5-4ace-af86-d3c227234d4b@web.de?part=1