Re: [PATCH v16 11/12] vfio/pci: Add TPH_ST_CONFIG for PCIe TPH ST configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Lockless modification of the `tph_permit` bitfield races with updates to adjacent bitfields, risking data corruption of critical device state flags.
- [Medium] The `tph_permit` flag is not reset when the VFIO device is closed, leaking state between user sessions.
--

commit 3866f62cd649303d4e63fcdfae7f9e49073c8b9c
Author: Chengwen Feng <fengchengwen@huawei.com>
Subject: vfio/pci: Add TPH_ST_CONFIG for PCIe TPH ST configuration

This patch adds the VFIO_DEVICE_FEATURE_TPH_ST_CONFIG feature to allow
userspace to configure PCIe TPH Steering Tag table entries. It uses a shadow
table to cache entries, paired with a per-device mutex for concurrent access
protection.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 7c6e3e1213d3e..e20c92a3dfcb1 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
[ ... ]
> @@ -496,6 +496,50 @@ static const struct dev_pm_ops vfio_pci_core_pm_ops = {
[ ... ]
> +static void vfio_pci_tph_deinit(struct vfio_pci_core_device *vdev)
> +{
> +	kfree(vdev->tph_st_shadow);
> +	vdev->tph_st_shadow = NULL;
> +	vdev->tph_st_entries = 0;
> +}

[Severity: Medium]
When the VFIO device is closed, should tph_permit be reset to 0 here or in
vfio_pci_core_disable()?

Because struct vfio_pci_core_device persists across opens, a subsequent
user opening the same device will inherit tph_permit = 1.

Could this allow the new user to bypass the explicit feature opt-in sequence
required by the API and cause cross-session state leakage?

[ ... ]
> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> index 5f32830766a0f..9cac61c63ead2 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -143,6 +143,9 @@ struct vfio_pci_core_device {
>  	struct notifier_block	nb;
>  	struct rw_semaphore	memory_lock;
>  	struct list_head	dmabufs;
> +	struct mutex		tph_lock;
> +	u16			*tph_st_shadow;
> +	u16			tph_st_entries;
>  };

[Severity: High]
Is it safe to modify tph_permit locklessly?

In struct vfio_pci_core_device, tph_permit is defined as a 1-bit bitfield
adjacent to other state flags like pm_runtime_engaged.

Because bitfields share the same underlying memory word, modifying
tph_permit translates to a non-atomic read-modify-write operation.

If vfio_pci_core_feature_tph_enable() sets vdev->tph_permit = 1 without a
lock, could it race with concurrent ioctls modifying adjacent bitfields
(such as pm_runtime_engaged under vdev->igate) and cause silent state
corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604132804.28678-1-fengchengwen@huawei.com?part=11