Re: [PATCH v2] PCI: Move pci_dev->is_busmaster into priv_flags

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unprotected C bitfield read-modify-write via sysfs corrupts PCI device state flags.
- [High] Concurrent execution of `pci_disable_device()` and `pci_set_master()` causes hardware/software state desynchronization and hardware RMW races.
--

--- Patch [1]: [PATCH v2] PCI: Move pci_dev->is_busmaster into priv_flags ---
commit 4e686d57a3e3e72847de4f3f8c7555be3dc32c14
Author: Maurice Hieronymus <mhi@mailbox.org>

PCI: Move pci_dev->is_busmaster into priv_flags

This commit moves the is_busmaster bit from the C bitfield in struct
pci_dev to the atomic priv_flags bitmap to prevent concurrent update issues.
It exposes accessor functions for outside users and prepares for
the Rust device enabling API rework.

Link: https://lore.kernel.org/rust-for-linux/DJOEYVBS17MJ.1YD3TNGQBWHNK@kernel.org/ [1]

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c

[ ... ]

> @@ -4120,7 +4147,7 @@ static void __pci_set_master(struct pci_dev *dev, bool enable)
>  			enable ? "enabling" : "disabling");
>  		pci_write_config_word(dev, PCI_COMMAND, cmd);
>  	}
> -	dev->is_busmaster = enable;
> +	pci_dev_assign_busmaster(dev, enable);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does this actually make
concurrent execution of pci_disable_device() and pci_set_master() safe?

While the software flag is now atomic, the hardware read-modify-write on
the PCI_COMMAND register appears to remain unsynchronized.

If Thread A calls do_pci_disable_device() in drivers/pci/pci.c and reads
PCI_COMMAND with MASTER=0, and concurrently Thread B calls 
__pci_set_master(true), reads PCI_COMMAND with MASTER=0, writes MASTER=1
to hardware, and sets the software flag, would Thread A skip its hardware
write but still proceed to clear the software flag?

Could this desynchronization leave bus mastering enabled in hardware while
software considers it disabled, potentially risking rogue DMA?

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -461,7 +461,6 @@ struct pci_dev {
>  	unsigned int	pref_64_window:1;	/* Pref mem window is 64-bit */
>  	unsigned int	multifunction:1;	/* Multi-function device */
>  
> -	unsigned int	is_busmaster:1;		/* Is busmaster */
>  	unsigned int	no_msi:1;		/* May not use MSI */
>  	unsigned int	block_cfg_access:1;	/* Config space access blocked */
>  	unsigned int	broken_parity_status:1;	/* Generates false positive parity */

[Severity: High]
This is a pre-existing issue, but does leaving broken_parity_status in this C
bitfield leave the remaining flags vulnerable to torn writes?

In drivers/pci/pci-sysfs.c, broken_parity_status_store() locklessly
modifies this bitfield when a privileged user writes to sysfs. If this
runs concurrently with kernel operations updating other flags in the
same memory word (like pci_cfg_access_lock() setting block_cfg_access),
could torn writes silently clear critical state flags such as
block_cfg_access or msi_enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-pci-dev-flags-v2-1-a1d7dc441cf3@mailbox.org?part=1