Re: [PATCH v5 06/11] PCI: liveupdate: Auto-preserve upstream bridges across Live Update

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Kernel stack overflow via deep recursion in `pci_liveupdate_preserve_path()`.
--

commit 9dcae64241d06ee18edeb26c25b9a7b263aa05a4
Author: David Matlack <dmatlack@google.com>

PCI: liveupdate: Auto-preserve upstream bridges across Live Update

This commit introduces tracking to auto-preserve upstream PCI bridges when a
device is preserved across a Live Update. It adds functions to preserve,
unpreserve, and finish the preservation of devices and recursively applies
these to upstream bridges up to the root port.

> diff --git a/drivers/pci/liveupdate.c b/drivers/pci/liveupdate.c
> index 558fbaec8dddd..d8e06afde2c76 100644
> --- a/drivers/pci/liveupdate.c
> +++ b/drivers/pci/liveupdate.c

[ ... ]

> @@ -363,8 +389,82 @@ int pci_liveupdate_preserve(struct pci_dev *dev)

[ ... ]

> +static int pci_liveupdate_preserve_path(struct pci_ser *ser, struct pci_dev *dev)
> +{
> +	int ret;
> +
> +	if (!dev)
> +		return 0;
> +
> +	ret = pci_liveupdate_preserve_device(ser, dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = pci_liveupdate_preserve_path(ser, dev->bus->self);

Can this recursion cause a kernel stack overflow?

The PCI specification allows up to 256 nested buses in a single domain, which
can occur with deep Thunderbolt or USB4 topologies.

Because the recursive call to pci_liveupdate_preserve_path() is followed by a
potential call to pci_liveupdate_unpreserve_device(), it cannot be optimized
as a tail call. This forces a new stack frame for every level of the PCI
hierarchy, which could overflow the standard kernel stack, especially with
KASAN enabled, and cause a crash.

Could this be rewritten to use an iterative approach instead?

> +	if (ret) {
> +		pci_liveupdate_unpreserve_device(ser, dev);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static void pci_liveupdate_unpreserve_path(struct pci_ser *ser, struct pci_dev *dev)
> +{
> +	if (!dev)
> +		return;
> +
> +	if (pci_liveupdate_unpreserve_device(ser, dev))
> +		return;
> +
> +	pci_liveupdate_unpreserve_path(ser, dev->bus->self);

Does this code have the same recursion issue as pci_liveupdate_preserve_path()
and require an iterative approach?

> +}

[ ... ]

> @@ -519,28 +656,18 @@ void pci_liveupdate_cleanup_device(struct pci_dev *dev)

[ ... ]

>  void pci_liveupdate_finish(struct pci_dev *dev)
>  {
> -	guard(write_lock)(&dev->liveupdate.lock);
> -
> -	if (!dev->liveupdate.incoming) {
> -		pci_warn(dev, "Cannot finish preserving an unpreserved device\n");
> +	if (!dev)
>  		return;
> -	}
> -
> -	pci_info(dev, "Device is finished participating in Live Update\n");
>  
> -	/*
> -	 * Drop the refcount so this device does not get treated as an incoming
> -	 * device again, e.g. in case pci_liveupdate_setup_device() gets called
> -	 * again because the device is hot-plugged.
> -	 */
> -	dev->liveupdate.incoming->refcount = 0;
> -	dev->liveupdate.incoming = NULL;
> +	if (pci_liveupdate_finish_device(dev))
> +		return;
>  
> -	/* Drop this device's reference on the incoming FLB. */
> -	pci_liveupdate_flb_put_incoming();
> +	pci_liveupdate_finish(dev->bus->self);

Does this code also have the same recursion issue?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512184846.119396-1-dmatlack@google.com?part=6