Re: [PATCH v5 03/11] PCI: liveupdate: Track incoming preserved PCI devices

[David Matlack <dmatlack@google.com>]

On 2026-05-14 01:05 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] Integer truncation in `pci_ser_xa_key()` drops the upper 16 bits of the PCI domain.
> - [High] Memory leak of KHO-preserved physical pages in the error path of `pci_flb_retrieve()`.
> - [High] Subsystem guide violation: `pci_flb_preserve()` calls a KHO serialization API without checking `kho_is_enabled()`.
> - [High] Data corruption in `pci_liveupdate_unpreserve()` due to decrementing `ser->nr_devices` without compacting the array.
> - [High] Use-after-free risk because `pci_flb_unpreserve()` leaves dangling pointers in `dev->liveupdate.outgoing`.
> - [High] `pci_liveupdate_finish()` calls `pci_liveupdate_flb_put_incoming()` under a write lock, risking a sleep-in-atomic bug.
> --
> 
> commit 46fdddc110c525ef02301643989348b7f03a10c0
> Author: David Matlack <dmatlack@google.com>
> 
> PCI: liveupdate: Track incoming preserved PCI devices
> 
> This commit adds support for tracking pci devices preserved across a kexec
> via live update. It introduces setup and cleanup hooks for the pci core to
> manage the incoming live update state of these devices.
> 
> > diff --git a/drivers/pci/liveupdate.c b/drivers/pci/liveupdate.c
> > index 9c4582ecd55cc..f14396dd14773 100644
> > --- a/drivers/pci/liveupdate.c
> > +++ b/drivers/pci/liveupdate.c
> > @@ -85,6 +101,21 @@ struct pci_flb_outgoing {
> >  	struct mutex lock;
> >  };
> >  
> > +/**
> > + * struct pci_flb_incoming - Incoming PCI FLB object
> > + * @ser: The incoming struct pci_ser from the previous kernel.
> > + * @xa: Xarray used to quickly lookup devices in @ser.
> > + */
> > +struct pci_flb_incoming {
> > +	struct pci_ser *ser;
> > +	struct xarray xa;
> > +};
> > +
> > +static unsigned long pci_ser_xa_key(u32 domain, u16 bdf)
> > +{
> > +	return domain << 16 | bdf;
> > +}
> 
> Can this integer shift truncate the upper 16 bits of the pci domain?
> 
> Because domain is a u32, the shift expression evaluates as a 32-bit integer.
> This discards the upper 16 bits before the result is promoted to the 64-bit
> unsigned long return type.
> 
> If devices are in domains >= 0x10000, this will produce colliding keys,
> causing them to overwrite each other in the xarray.
> 
> Could this be cast before the shift, such as
> ((unsigned long)domain << 16) | bdf?

Ah I thought I had fixed that in v5. Will fix in v6.

> > +
> >  static int pci_flb_preserve(struct liveupdate_flb_op_args *args)
> >  {
> >  	struct pci_flb_outgoing *outgoing;
> 
> Does this function need to check kho_is_enabled() before proceeding?
> 
> According to the kho subsystem guidelines, all callers of serialization-side
> kho APIs (like kho_alloc_preserve() used later in this function) must
> gate their usage on kho_is_enabled().
> 
> Failing to check the enabled state can cause null pointer dereferences or
> silently add useless tracking state that will never be used.
> 
> [ ... ]
> 
> > @@ -140,13 +171,44 @@ static void pci_flb_unpreserve(struct liveupdate_flb_op_args *args)
> 
> Can pci_flb_unpreserve() cause a use-after-free risk by leaving dangling
> pointers?
> 
> When an outgoing live update is aborted, this function frees the outgoing->ser
> memory via kho_unpreserve_free(). However, it does not clear the
> dev->liveupdate.outgoing pointers in the pci devices that were already
> preserved.
> 
> If a new live update is initiated and pci_liveupdate_unpreserve() is called
> on those devices, it will execute memset(dev_ser, 0, ...) on the dangling
> dev->liveupdate.outgoing pointer, corrupting memory.

This was addressed in the patch 2 thread.

> 
> >  
> >  static int pci_flb_retrieve(struct liveupdate_flb_op_args *args)
> >  {
> > -	args->obj = phys_to_virt(args->data);
> > +	struct pci_flb_incoming *incoming;
> > +	int i, ret;
> > +
> > +	incoming = kmalloc_obj(*incoming);
> > +	if (!incoming)
> > +		return -ENOMEM;
> > +
> > +	incoming->ser = phys_to_virt(args->data);
> > +
> > +	xa_init(&incoming->xa);
> > +
> > +	for (i = 0; i < incoming->ser->max_nr_devices; i++) {
> > +		struct pci_dev_ser *dev_ser = &incoming->ser->devices[i];
> > +		unsigned long key;
> > +
> > +		if (!dev_ser->refcount)
> > +			continue;
> > +
> > +		key = pci_ser_xa_key(dev_ser->domain, dev_ser->bdf);
> > +		ret = xa_err(xa_store(&incoming->xa, key, dev_ser, GFP_KERNEL));
> > +		if (ret) {
> > +			xa_destroy(&incoming->xa);
> > +			kfree(incoming);
> > +			return ret;
> > +		}
> 
> Does this error path leak the preserved physical pages?
> 
> If xa_store() fails, this returns an error code without calling
> kho_restore_free(incoming->ser). Because the function returns an error and
> does not populate args->obj, the live update core will not call the .finish
> callback to clean up the memory.

Yes if retrieve fails then the preserved struct pci_ser gets leaked. The
user is likely going to reboot the system anyway since the preserved
devices will not able to be retrieved, so I don't think leaking is a
problem in practice. But I also don't see any reason why we have to leak
it. I will free it in v6.

> 
> [ ... ]
> 
> > @@ -268,6 +330,163 @@ void pci_liveupdate_unpreserve(struct pci_dev *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(pci_liveupdate_unpreserve);
> 
> Can pci_liveupdate_unpreserve() corrupt the live update tracking state by
> leaving holes in the array?
> 
> Looking at the body of pci_liveupdate_unpreserve():
> 
> drivers/pci/liveupdate.c:pci_liveupdate_unpreserve() {
>     ...
>     ser->nr_devices--;
>     memset(dev_ser, 0, sizeof(*dev_ser));
>     ...
> }
> 
> If a device other than the most recently preserved one is unpreserved, this
> leaves a hole in the array and moves the allocation cursor backward.
> Subsequent device preservations will overwrite currently preserved, valid
> devices at that index.

Leaving a hole in the array is the intention. ser->nr_devices is not a
cursor, it is the number of valid (non-hole) elements of the devices
array. pci_liveupdate_preserve() specifically walks through the entire
array to find the first non-hole to use for the next device.

> [ ... ]
> 
> > +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");
> > +		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;
> > +
> > +	/* Drop this device's reference on the incoming FLB. */
> > +	pci_liveupdate_flb_put_incoming();
> > +}
> 
> Can calling pci_liveupdate_flb_put_incoming() under a write lock cause a
> sleep-in-atomic bug?
> 
> This function calls pci_liveupdate_flb_put_incoming() while holding
> dev->liveupdate.lock (an rwlock) for writing. If the put function acquires a
> sleepable lock internally, calling it from atomic context will trigger a bug.
> 
> This contrasts with pci_liveupdate_cleanup_device() which drops the lock
> first using scoped_guard().

Ah I fixed this in a later patch in the series but forgot to also
address it in this intermediate state. Will fix in v6.

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