Re: [PATCH v3 3/9] ipack: tpci200: don't store pci_device_id

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unconditional return of 1 in `ipack_unregister_bus_member` halts bus device iteration prematurely, leaving most devices orphaned but still registered.
--

commit 6023ba18e50f5578e133a944842fdee804f479b0
Author: Gary Guo <gary@garyguo.net>

    ipack: tpci200: don't store pci_device_id
    
    pci_device_id is not guaranteed to live longer than probe due to presence
    of dynamic ID. This stored ID is unused so remove it.

> diff --git a/drivers/ipack/carriers/tpci200.c b/drivers/ipack/carriers/tpci200.c
> index 05dcb6675cd66..1cf51f7632932 100644
> --- a/drivers/ipack/carriers/tpci200.c
> +++ b/drivers/ipack/carriers/tpci200.c
> @@ -562,7 +562,6 @@ static int tpci200_pci_probe(struct pci_dev *pdev,
>  
>  	/* Save struct pci_dev pointer */
>  	tpci200->info->pdev = pdev;
> -	tpci200->info->id_table = (struct pci_device_id *)id;
>  
>  	/* register the device and initialize it */
>  	ret = tpci200_install(tpci200);

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free during
bus unregistration that I wanted to ask about.

When a carrier driver is unbound and calls ipack_bus_unregister(), it uses
bus_for_each_dev() to iterate and unregister child devices:

drivers/ipack/ipack.c:ipack_bus_unregister() {
    ...
	bus_for_each_dev(&ipack_bus_type, NULL, bus,
		ipack_unregister_bus_member);
	ida_free(&ipack_ida, bus->bus_nr);
	kfree(bus);
    ...
}

However, the callback unconditionally returns 1:

drivers/ipack/ipack.c:ipack_unregister_bus_member() {
    ...
	if (idev->bus == bus)
		ipack_device_del(idev);

	return 1;
}

Since bus_for_each_dev() halts iteration on any non-zero return value, does
this stop after evaluating only the first device in the bus list?

If the iteration stops prematurely, the remaining devices would be orphaned but
stay registered. Because ipack_bus_unregister() immediately frees the bus
structure after the iteration, could this result in a use-after-free if an
ipack client driver later binds to one of these orphaned devices or their sysfs
attributes are accessed?

> diff --git a/drivers/ipack/carriers/tpci200.h b/drivers/ipack/carriers/tpci200.h
> index e79ac64abcffa..a2bf3125794b7 100644
> --- a/drivers/ipack/carriers/tpci200.h
> +++ b/drivers/ipack/carriers/tpci200.h
> @@ -145,7 +145,6 @@ struct tpci200_slot {
>   */
>  struct tpci200_infos {
>  	struct pci_dev			*pdev;
> -	struct pci_device_id		*id_table;
>  	struct tpci200_regs __iomem	*interface_regs;
>  	void __iomem			*cfg_regs;
>  	struct ipack_bus_device		*ipack_bus;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-pci_id_fix-v3-0-2d48fc025acc@garyguo.net?part=3