Re: [PATCH v16 10/12] vfio/pci: Add TPH_CPU_ST to query CPU's TPH steering tag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] VFIO feature probing is broken for `VFIO_DEVICE_FEATURE_TPH_CPU_ST` due to premature capability check.

Pre-existing issues:
- [High] Data race on bitfields in `struct vfio_pci_core_device` due to unsynchronized updates to `tph_permit`.
--

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

vfio/pci: Add TPH_CPU_ST to query CPU's TPH steering tag

This commit adds a new read-only VFIO device feature
VFIO_DEVICE_FEATURE_TPH_CPU_ST to allow userspace to query PCIe TPH
Steering Tags by CPU ID. It implements a batch query logic and is
double-gated by a module parameter and 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 ae05343701f76..7c6e3e1213d3e 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
[ ... ]
> @@ -1535,6 +1536,72 @@ static int vfio_pci_core_feature_tph_enable(struct vfio_pci_core_device *vdev,
>  	return 0;
>  }
>  
> +static int vfio_pci_core_feature_tph_cpu_st(struct vfio_pci_core_device *vdev,
> +			u32 flags,
> +			struct vfio_device_feature_tph_cpu_st __user *arg,
> +			size_t argsz)
> +{
> +	struct vfio_device_feature_tph_cpu_st cpu_st;
> +	struct pci_dev *pdev = vdev->pdev;
> +	enum tph_mem_type mtype;
> +	void __user *uptr;
> +	bool extended;
> +	int i, ret;
> +	u32 *cpus;
> +	u16 *sts;
> +	u16 st;
> +
> +	if (!vdev->tph_permit)

[Severity: High]
This is a pre-existing issue, but does the tph_permit flag suffer from a data
race due to being packed in a bitfield without locking?

The tph_permit flag is packed as a bitfield (bool tph_permit:1;) alongside
other volatile device state flags like bardirty and needs_reset in
struct vfio_pci_core_device.

The vfio_pci_core_feature_tph_enable() ioctl modifies vdev->tph_permit
without holding any device-level locks:

    if (!vdev->tph_permit)
        vdev->tph_permit = 1;

Since bitfield modifications are compiled into non-atomic read-modify-write
operations on the underlying word, executing the TPH enable ioctl
concurrently with other state changes (such as vdev->bardirty = true; in
vfio_pci_core_write(), which is executed under memory_lock read-side) could
cause a data race. Can this silently overwrite and drop state changes,
potentially causing the device to malfunction due to missed resets
or unflushed BARs?

> +		return -EOPNOTSUPP;
> +
> +	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_GET,
> +				 sizeof(cpu_st));

[Severity: Medium]
Will this early tph_permit check break VFIO feature probing for
VFIO_DEVICE_FEATURE_TPH_CPU_ST?

Because the dynamic permission check intercepts the call first, probing
TPH_CPU_ST will incorrectly fail with -EOPNOTSUPP unless the user has
already dynamically enabled TPH via VFIO_DEVICE_FEATURE_TPH_ENABLE.

vfio_check_feature() handles VFIO_DEVICE_FEATURE_PROBE operations, which
userspace uses to discover if the kernel understands a feature ioctl.
Moving the tph_permit check after vfio_check_feature() would resolve this,
as PROBE returns 0 immediately.

> +	if (ret <= 0)
> +		return ret;

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