Re: [PATCH v2 12/13] PCI/CXL: Add a sysfs entry to unmask SBR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Concurrent writes to the `cxl_unmask_sbr` bitfield via the sysfs store function can corrupt adjacent bitfields in `struct pci_dev` due to non-atomic read-modify-write operations.
--

--- Patch [12]: [PATCH v2 12/13] PCI/CXL: Add a sysfs entry to unmask SBR ---
commit d915ae90bb0b4f81c466e6e9be4cc5b40f737fbb
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

PCI/CXL: Add a sysfs entry to unmask SBR

A Secondary Bus Reset of a CXL Downstream Port only reaches the component
below it when the Port's Unmask SBR bit is set. This commit adds a
cxl_unmask_sbr sysfs attribute to allow the kernel to trigger an SBR of
CXL DPorts.

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
[ ... ]
> @@ -573,6 +573,33 @@ static ssize_t reset_subordinate_store(struct device *dev,
>  }
>  static DEVICE_ATTR_WO(reset_subordinate);
>  
> +static ssize_t cxl_unmask_sbr_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return sysfs_emit(buf, "%u\n", pdev->cxl_unmask_sbr);
> +}
> +
> +static ssize_t cxl_unmask_sbr_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	unsigned long val;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (kstrtoul(buf, 0, &val) < 0)
> +		return -EINVAL;
> +
> +	pdev->cxl_unmask_sbr = !!val;

[Severity: High]
Does writing to this bitfield without synchronization risk corrupting adjacent
state flags in struct pci_dev?

Modifying a C bitfield is a non-atomic read-modify-write operation on the
containing 32-bit word.

If a user writes to this sysfs file while the kernel concurrently modifies
other flags sharing this word (like is_busmaster, msi_enabled, or
state_saved), could this silently overwrite those updates?

> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(cxl_unmask_sbr);

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
[ ... ]
> @@ -485,6 +485,7 @@ struct pci_dev {
>  	unsigned int	shpc_managed:1;		/* SHPC owned by shpchp */
>  	unsigned int	is_thunderbolt:1;	/* Thunderbolt controller */
>  	unsigned int	is_cxl:1;               /* Compute Express Link (CXL) */
> +	unsigned int	cxl_unmask_sbr:1;	/* SBR unmask allowed by user */
>  	/*
>  	 * Devices marked being untrusted are the ones that can potentially
>  	 * execute DMA attacks and similar. They are typically connected

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=12