Re: [PATCH v18 2/4] vfio-pci/zdev: Add a device feature for error information

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The UAPI structure `vfio_device_feature_zpci_err` uses C bitfields and contains implicit compiler padding holes, violating ABI stability guidelines.
- [Medium] User-triggerable kernel warning log due to failed `copy_to_user()`.
--

commit 91bf6efc558a8f6b752d8cbe1ecebd56aaccfea2
Author: Farhan Ali <alifm@linux.ibm.com>

vfio-pci/zdev: Add a device feature for error information

This commit adds a new device feature to expose zPCI platform specific
error information to userspace. This allows userspace to recover from
these errors.

> diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
> index 78a28db00c6d6..c0d6362cc4eb0 100644
> --- a/drivers/vfio/pci/vfio_pci_zdev.c
> +++ b/drivers/vfio/pci/vfio_pci_zdev.c
[ ... ]
> @@ -141,6 +141,49 @@ int vfio_pci_info_zdev_add_caps(struct vfio_pci_core_device *vdev,
[ ... ]
> +	if (copy_to_user(arg, &err, sizeof(err))) {
> +		dev_warn_ratelimited(device->dev,
> +				     "Failed to handle PCI error event for PCI function 0x%x",
> +				     zdev->fid);
> +		return -EFAULT;
> +	}

[Severity: Medium]
Is it intended to log a kernel warning when copy_to_user() fails?

Because copy_to_user() failures are caused directly by user space providing
an invalid, unmapped, or read-only memory address in the arg pointer,
could an unprivileged user intentionally pass invalid pointers to trigger
these warnings and spam the host kernel logs?

> +
> +	return 0;
> +}
[ ... ]
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 5de618a3a5eee..3eead86a00ab2 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
[ ... ]
> @@ -1534,6 +1534,36 @@ struct vfio_device_feature_dma_buf {
[ ... ]
> +struct vfio_device_feature_zpci_err {
> +	__u32 feature_flags;		/* Indicate future features */
> +	__u32 reserved1;
> +	__u32 fh;			/* function handle */
> +	__u32 fid;			/* function id */
> +	__u32 ett		:  4;	/* expected table type */
> +	__u32 mvn		: 12;	/* MSI vector number */
> +	__u32 dmaas		:  8;	/* DMA address space */
> +	__u32 reserved2		:  6;
> +	__u32 q			:  1;	/* event qualifier */
> +	__u32 rw		:  1;	/* read/write */
> +	__u64 faddr;			/* failing address */
> +	__u32 reserved3;
> +	__u16 reserved4;
> +	__u16 pec;			/* PCI event code */
> +	__u8 reserved5[28];		/* Allow for future expansion */
> +};

[Severity: High]
Does this struct layout rely on C bitfields and implicit compiler padding in
a UAPI header?

Bitfields are highly compiler- and architecture-dependent, which can break
cross-architecture compatibility.

Additionally, is there a 4-byte implicit padding hole between the bitfields
(ending at offset 20) and faddr (which must start at offset 24 due to
8-byte alignment on 64-bit platforms)? Is there another 4-byte implicit
padding hole at the end of the struct to satisfy the overall 8-byte
alignment requirement?

Should explicit padding fields and bitmask macros be used instead of C
bitfields to ensure ABI stability?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603182415.2324-1-alifm@linux.ibm.com?part=2