Re: [PATCH v7 03/12] PCI: liveupdate: Track incoming preserved PCI devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] pci_liveupdate_finish_device() incorrectly aborts if refcount != 1, completely breaking the preservation lifecycle for parent bridges and leaking their FLB references.
- [Medium] `pci_liveupdate_cleanup_device()` fails to clear `dev_ser->refcount`, leading to state contamination for subsequently hot-plugged devices.
- [Medium] Memory allocated for incoming preserved PCI devices (the KHO block set) is leaked in `pci_flb_retrieve()` if `kzalloc_obj()` fails.
--

--- Patch [3]: [PATCH v7 03/12] PCI: liveupdate: Track incoming preserved PCI devices ---
commit 4fcd6c9f661309adcb1a2b891b9c5a34af9e319f
Author: David Matlack <dmatlack@google.com>

PCI: liveupdate: Track incoming preserved PCI devices

This patch adds support to the PCI core for tracking preserved PCI devices 
across Live Update. It introduces pci_liveupdate_setup_device() to fetch 
serialized state from the Live Update Orchestrator and exposes APIs for 
drivers to notify the PCI core when preservation is finished.

> diff --git a/drivers/pci/liveupdate.c b/drivers/pci/liveupdate.c
> index 03075ce06ac98..df6a02240aa47 100644
> --- a/drivers/pci/liveupdate.c
> +++ b/drivers/pci/liveupdate.c

[ ... ]

> @@ -138,13 +172,58 @@ static void pci_flb_unpreserve(struct liveupdate_flb_op_args *args)
>  
>  static int pci_flb_retrieve(struct liveupdate_flb_op_args *args)
>  {
> -	args->obj = phys_to_virt(args->data);
> +	struct pci_ser *ser = phys_to_virt(args->data);
> +	struct pci_flb_incoming *incoming;
> +	struct pci_dev_ser *dev_ser;
> +	struct kho_block_set_it it;
> +	int ret = -ENOMEM;
> +
> +	incoming = kzalloc_obj(*incoming);
> +	if (!incoming)
> +		goto err_restore_free;

[ ... ]

> +err_block_set_destroy:
> +	kho_block_set_destroy(&incoming->block_set);
> +err_free_incoming:
> +	xa_destroy(&incoming->xa);
> +	kfree(incoming);
> +err_restore_free:
> +	kho_restore_free(ser);
> +	return ret;
>  }

[Severity: Medium]
If kzalloc_obj() fails, the code jumps to err_restore_free and calls
kho_restore_free(ser).

Because the underlying memory pages backing ser->devices (the KHO block set)
have not been restored or explicitly freed, does this error path permanently
leak the memory reserved for the block set?

[ ... ]

> @@ -298,6 +377,87 @@ void pci_liveupdate_unpreserve(struct pci_dev *dev)

[ ... ]

> +void pci_liveupdate_setup_device(struct pci_dev *dev)
> +{

[ ... ]

> +	dev_ser = xa_load(&incoming->xa, key);
> +
> +	/*
> +	 * This device was not preserved across Live Update, or it was preserved
> +	 * but has already been probed and gone through pci_liveupdate_finish(),
> +	 * e.g. due to removing and re-adding the device. Either way, it's not
> +	 * treated as incoming-preserved.
> +	 */
> +	if (!dev_ser || !dev_ser->refcount) {
> +		pci_liveupdate_flb_put_incoming();
> +		return;
> +	}

[ ... ]

>  void pci_liveupdate_cleanup_device(struct pci_dev *dev)
>  {
>  	/*
> @@ -309,7 +469,83 @@ void pci_liveupdate_cleanup_device(struct pci_dev *dev)
>  	 */
>  	if (READ_ONCE(dev->liveupdate.outgoing))
>  		pci_WARN(dev, 1, "Destroying outgoing-preserved device!\n");
> +
> +	if (READ_ONCE(dev->liveupdate.incoming)) {
> +		pci_WARN(dev, 1, "Destroying incoming-preserved device!\n");
> +		pci_liveupdate_flb_put_incoming();
> +	}
>  }

[Severity: Medium]
In pci_liveupdate_cleanup_device(), the FLB reference is dropped but 
dev_ser->refcount is left unmodified.

If a preserved device is removed or destroyed before its driver calls 
pci_liveupdate_finish() (for instance, if probe fails), and a new device
is subsequently hot-plugged at the same BDF, won't pci_liveupdate_setup_device()
see the stale dev_ser with a non-zero refcount and incorrectly associate the 
new device with the stale preserved state?

> +static void pci_liveupdate_finish_device(struct pci_ser *ser, struct pci_dev *dev)
> +{
> +	if (!dev->liveupdate.incoming) {
> +		pci_warn(dev, "Cannot finish preserving an unpreserved device\n");
> +		return;
> +	}
> +
> +	if (dev->liveupdate.incoming->refcount != 1) {
> +		pci_WARN(dev, 1, "Preserved device has a corrupted refcount!\n");
> +		return;
> +	}

[Severity: High]
Is this strict refcount != 1 check correct for PCI bridges?

Looking at the outgoing path in pci_liveupdate_preserve_device_again(), a 
bridge legitimately has its refcount incremented for each preserved downstream 
device, making it greater than 1.

When the bridge driver calls pci_liveupdate_finish() on the incoming side, 
won't this check fail, abort the finish process, and permanently leak the 
FLB reference for the bridge?

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