Re: [PATCH v17 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 5 potential issue(s) to consider:
- [High] Improper PM Runtime handling and missing dmabuf restoration in TPH ST configuration ioctl.
- [Medium] Missing __GFP_ACCOUNT in tph_st_shadow allocation.
- [Medium] The `tph_permit` flag is leaked across VFIO user sessions.
- [Medium] Feature PROBE for `TPH_ST_CONFIG` is broken due to an early dynamic permission check.
- [Medium] The TPH ST shadow table is not cleared when the VFIO device is reset, leading to stale data restoration.
--

commit b8ec8f816e02c6def8f7cc759648796f130131e0
Author: Chengwen Feng <fengchengwen@huawei.com>

vfio/pci: Add TPH_ST_CONFIG for PCIe TPH ST configuration

This commit adds a new VFIO device feature VFIO_DEVICE_FEATURE_TPH_ST_CONFIG
to allow userspace to configure PCIe TPH Steering Tag table entries. It
implements a shadow ST 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 ae05343701f76..cf8f6de025c9a 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c

[ ... ]

> @@ -495,6 +496,50 @@ static const struct dev_pm_ops vfio_pci_core_pm_ops = {

[ ... ]

> +static int vfio_pci_tph_init(struct vfio_pci_core_device *vdev)
> +{
> +	vdev->tph_st_entries = 0;
> +	vdev->tph_st_shadow = NULL;
> +
> +	if (!enable_unsafe_tph)
> +		return 0;
> +
> +	vdev->tph_st_entries = vfio_pci_tph_st_shadow_size(vdev);
> +	if (vdev->tph_st_entries) {
> +		vdev->tph_st_shadow = kcalloc(vdev->tph_st_entries, sizeof(u16),
> +					      GFP_KERNEL);

[Severity: Medium]
Since this per-device allocation is triggered by a userspace container
session, should it use GFP_KERNEL_ACCOUNT instead of GFP_KERNEL? Omitting
the account flag might allow a container to allocate unaccounted kernel
memory and bypass its memory cgroup limits.

> +		if (!vdev->tph_st_shadow)
> +			return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +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]
Should vdev->tph_permit be reset to 0 here during session exit?

Because vdev persists across open/close cycles until unbound from the driver,
a subsequent user session might inherit the tph_permit == 1 state,
bypassing the API requirement to explicitly enable TPH.

> +}

[ ... ]

> @@ -1535,6 +1587,74 @@ static int vfio_pci_core_feature_tph_enable(struct vfio_pci_core_device *vdev,

[ ... ]

> +static int vfio_pci_core_feature_tph_st_config(
> +			struct vfio_pci_core_device *vdev,
> +			u32 flags,
> +			struct vfio_device_feature_tph_st_config __user *arg,
> +			size_t argsz)
> +{
> +	struct vfio_device_feature_tph_st_config config;
> +	struct pci_dev *pdev = vdev->pdev;
> +	void __user *uptr;
> +	int i, idx, ret;
> +	size_t sz;
> +	u16 *sts;
> +
> +	if (!vdev->tph_permit || !vdev->tph_st_shadow)
> +		return -EOPNOTSUPP;

[Severity: Medium]
Will this early check break feature discovery?

VFIO allows discovering supported features using the
VFIO_DEVICE_FEATURE_PROBE flag. By checking !vdev->tph_permit before
calling vfio_check_feature(), any early capability PROBE request returns
-EOPNOTSUPP. This might make userspace incorrectly assume the kernel does
not support configuring the feature.

> +
> +	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET,
> +				 sizeof(config));
> +	if (ret <= 0)
> +		return ret;

[ ... ]

> +	uptr = u64_to_user_ptr(config.data_uptr);
> +	sts = memdup_array_user(uptr, config.count, sizeof(u16));
> +	sz = config.count * sizeof(u16);
> +	if (IS_ERR(sts))
> +		return PTR_ERR(sts);
> +
> +	down_write(&vdev->memory_lock);
> +	ret = vfio_pci_set_power_state(vdev, PCI_D0);

[Severity: High]
Does this bypass the driver core's power management and miss dmabuf
restoration?

If the parent bridge is in a suspended state (e.g., D3cold), calling
vfio_pci_set_power_state() without first calling pm_runtime_resume_and_get()
can lead to bus errors (Unsupported Request) or system crashes.

Additionally, if the device was previously in a suspended state, does this
path also need to call vfio_pci_dma_buf_move(vdev, false) to restore dmabufs
and prevent permanently breaking dmabuf tracking?

> +	if (ret)
> +		goto out_unlock_memory;
> +
> +	if (pcie_tph_enabled_req_type(pdev) == PCI_TPH_REQ_DISABLE)
> +		goto update_shadow;

[ ... ]

> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> index 0b411d2136ed5..d5c6f73747ad9 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -143,6 +143,8 @@ struct vfio_pci_core_device {
>  	struct notifier_block	nb;
>  	struct rw_semaphore	memory_lock;
>  	struct list_head	dmabufs;
> +	u16			*tph_st_shadow;

[Severity: Medium]
When a VFIO device is reset (e.g., via the VFIO_DEVICE_RESET ioctl or FLR),
its hardware ST table typically reverts to its default (zeroed) state.

Should the reset path (vfio_pci_ioctl_reset()) clear vdev->tph_st_shadow?

If not, a subsequent partial ST_CONFIG write or TPH enablement via config
space might incorrectly program the hardware with stale, pre-reset ST values
cached in the shadow table.

> +	u16			tph_st_entries;
>  };
>  
>  enum vfio_pci_io_width {

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