Re: [PATCH 5/5] PCI/sysfs: Use user config accessors for serial_number

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PCI/sysfs: serial_number_show() ignores pci_user_read_config_dword() errors
--

commit c9ac0517dd6b036d6799a82dae1da82995d72a99
Author: Matthew Wood <thepacketgeek@gmail.com>

PCI/sysfs: Use user config accessors for serial_number

This commit updates serial_number_show() to walk capabilities and read the
Device Serial Number using pci_user_read_config_dword() instead of
pci_get_dsn(), wrapped in pci_config_pm_runtime_get() and
pci_config_pm_runtime_put(), so config reads wait for in-progress device
resets.

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index a272f8583a8fe..cbdd56b92d1aa 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -696,10 +696,23 @@ static ssize_t serial_number_show(struct device *dev,
>  				  struct device_attribute *attr, char *buf)
>  {
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
> -	u64 dsn;
> +	u32 dword;
> +	u64 dsn = 0;
>  	u8 bytes[8];
> +	int pos;
> +
> +	pci_config_pm_runtime_get(pci_dev);
> +	pos = PCI_FIND_NEXT_EXT_CAP(pci_user_read_config, 0,
> +				    PCI_EXT_CAP_ID_DSN, NULL, pci_dev);
> +	if (pos) {
> +		pos += 4;
> +		pci_user_read_config_dword(pci_dev, pos, &dword);
> +		dsn = (u64)dword;
> +		pci_user_read_config_dword(pci_dev, pos + 4, &dword);
> +		dsn |= ((u64)dword) << 32;
> +	}

[Severity: Medium]
What happens if either of the pci_user_read_config_dword() calls in
serial_number_show() fails after PCI_FIND_NEXT_EXT_CAP() locates the DSN
capability?

Each pci_user_read_config_dword() call independently acquires and releases
pci_lock, so a surprise removal / hot-unplug (marking the device
disconnected) or a PCI bus read error can occur between finding the
capability and reading the two DSN dwords.

In drivers/pci/access.c, PCI_USER_READ_CONFIG() sets the output value to all
ones on failure:

drivers/pci/access.c:PCI_USER_READ_CONFIG() {
	...
	if (pci_dev_is_disconnected(dev))
		ret = PCIBIOS_DEVICE_NOT_FOUND;
	else
		ret = dev->bus->ops->read(dev->bus, dev->devfn,
					  pos, sizeof(type), &data);
	raw_spin_unlock_irq(&pci_lock);
	if (ret)
		PCI_SET_ERROR_RESPONSE(val);
	else
		*val = (type)data;

	return pcibios_err_to_errno(ret);
}

Because the return values of pci_user_read_config_dword() are not checked in
serial_number_show(), a failed read sets dword to 0xffffffff and folds it
into dsn. That makes dsn non-zero, bypassing the subsequent !dsn check and
emitting an all-ones (ff-ff-ff-ff-ff-ff-ff-ff) or partially all-ones serial
number to userspace instead of returning an error.

Should serial_number_show() check the return values of both
pci_user_read_config_dword() calls before constructing dsn?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025000836.58294-1-thepacketgeek@gmail.com?part=5