Re: [PATCH v16 09/12] vfio/pci: Add TPH_ENABLE feature skeleton and unsafe module parameter

[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 causes a C11 data race with adjacent bitfields, potentially corrupting device state.
- [Medium] The `tph_permit` flag is not cleared when the device is closed, leaking the enabled state to subsequent users of the device.
--

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

vfio/pci: Add TPH_ENABLE feature skeleton and unsafe module parameter

This commit introduces the framework for PCIe TPH (Transaction Processing
Hints) support in VFIO. It adds a global enable_unsafe_tph module parameter
and a new VFIO_DEVICE_FEATURE_TPH_ENABLE uapi to allow userspace to set a
per-device tph_permit flag.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 3f8d093aacf8a..ae05343701f76 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c

[ ... ]

> @@ -1516,6 +1517,24 @@ static int vfio_pci_core_feature_token(struct vfio_pci_core_device *vdev,
>  	return 0;
>  }
>  
> +static int vfio_pci_core_feature_tph_enable(struct vfio_pci_core_device *vdev,
> +					    u32 flags, size_t argsz)
> +{
> +	int ret;
> +
> +	if (!enable_unsafe_tph)
> +		return -EOPNOTSUPP;
> +
> +	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET, 0);
> +	if (ret <= 0)
> +		return ret;
> +
> +	if (!vdev->tph_permit)
> +		vdev->tph_permit = 1;

[Severity: High]
Does setting this bitfield without a lock cause a read-modify-write data
race with adjacent bitfields?

Looking at struct vfio_pci_core_device, tph_permit shares the same memory
word as pm_intx_masked and pm_runtime_engaged. Since C compilers implement
bitfield assignments via read-modify-write operations, could a concurrent
lockless update here overwrite a state change made elsewhere?

For example, vfio_pci_core_runtime_suspend() updates vdev->pm_intx_masked
without holding any locks. If the ioctl and runtime PM suspend execute
concurrently, it seems one could overwrite the other's state change, which
could cause the kernel to fail to unmask INTx interrupts during resume.

[Severity: Medium]
Is there a mechanism to clear this flag when the device is closed?

Because struct vfio_pci_core_device is tied to the PCI device lifetime and
is not reallocated between sessions, it appears a subsequent user opening
the device will inherit tph_permit = 1.

While hardware state might be cleaned up on close in functions like
vfio_pci_core_disable() or vfio_pci_tph_deinit(), could failing to clear
vdev->tph_permit bypass the userspace opt-in requirement for future sessions,
allowing a VMM that never called VFIO_DEVICE_FEATURE_TPH_ENABLE to configure
ST tables?

> +
> +	return 0;
> +}
> +
>  int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
>  				void __user *arg, size_t argsz)
>  {

[ ... ]

> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> index 2ebba746c18f7..5f32830766a0f 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h

[ ... ]

> @@ -127,6 +127,7 @@ struct vfio_pci_core_device {
>  	bool			needs_pm_restore:1;
>  	bool			pm_intx_masked:1;
>  	bool			pm_runtime_engaged:1;
> +	bool			tph_permit:1;
>  	struct pci_saved_state	*pci_saved_state;
>  	struct pci_saved_state	*pm_save;
>  	int			ioeventfds_nr;

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