| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0is40HhOwPyKe2_K6qcqVE0iNwUW3q746vW+fnRgzA9YQ@mail.gmail.com>
Date: Wed, 17 May 2023 13:32:06 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: linux-pci@...r.kernel.org, Bjorn Helgaas <bhelgaas@...gle.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Rob Herring <robh@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
Emmanuel Grumbach <emmanuel.grumbach@...el.com>,
"Rafael J . Wysocki" <rafael@...nel.org>,
Heiner Kallweit <hkallweit1@...il.com>,
Lukas Wunner <lukas@...ner.de>,
Jesse Barnes <jbarnes@...tuousgeek.org>,
"Rafael J. Wysocki" <rjw@...ysocki.net>,
Jiang Liu <jiang.liu@...wei.com>,
Yijing Wang <wangyijing@...wei.com>,
Greg Kroah-Hartman <gregkh@...e.de>,
Shaohua Li <shaohua.li@...el.com>,
Mike Marciniszyn <mike.marciniszyn@...el.com>,
Ashutosh Dixit <ashutosh.dixit@...el.com>,
Dean Luick <dean.luick@...el.com>,
Doug Ledford <dledford@...hat.com>,
Ira Weiny <ira.weiny@...el.com>,
Ricky Wu <ricky_wu@...ltek.com>,
Rui Feng <rui_feng@...lsil.com.cn>,
Micky Ching <micky_ching@...lsil.com.cn>,
Lee Jones <lee.jones@...aro.org>,
Samuel Ortiz <sameo@...ux.intel.com>,
Wei WANG <wei_wang@...lsil.com.cn>,
Daniel Bristot de Oliveira <bristot@...hat.com>,
Matt Carlson <mcarlson@...adcom.com>,
Michael Chan <mchan@...adcom.com>,
"David S. Miller" <davem@...emloft.net>,
Francois Romieu <romieu@...zoreil.com>,
Sven Peter <sven@...npeter.dev>,
Luiz Augusto von Dentz <luiz.von.dentz@...el.com>,
Bruce Allan <bruce.w.allan@...el.com>,
Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
Jeff Garzik <jeff@...zik.org>,
Auke Kok <auke-jan.h.kok@...el.com>,
"John W. Linville" <linville@...driver.com>,
Vasanthakumar Thiagarajan <vasanth@...eros.com>,
Stanislaw Gruszka <sgruszka@...hat.com>,
Kalle Valo <kvalo@...nel.org>,
Lorenzo Bianconi <lorenzo@...nel.org>,
Felix Fietkau <nbd@....name>, linux-kernel@...r.kernel.org,
Dean Luick <dean.luick@...nelisnetworks.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
stable@...r.kernel.org
Subject: Re: [PATCH v2 1/9] PCI: Add locking to RMW PCI Express Capability
Register accessors
On Wed, May 17, 2023 at 12:53 PM Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com> wrote:
>
> Many places in the kernel write the Link Control and Root Control PCI
> Express Capability Registers without proper concurrency control and
> this could result in losing the changes one of the writers intended to
> make.
>
> Add pcie_cap_lock spinlock into the struct pci_dev and use it to
> protect bit changes made in the RMW capability accessors. Protect only
> a selected set of registers by differentiating the RMW accessor
> internally to locked/unlocked variants using a wrapper which has the
> same signature as pcie_capability_clear_and_set_word(). As the
> Capability Register (pos) given to the wrapper is always a constant,
> the compiler should be able to simplify all the dead-code away.
>
> The RMW locking is only added to pcie_capability_clear_and_set_word()
> because so far only the Link Control Register (ASPM, hotplug, various
> drivers) and the Root Control Register (AER & PME) require RMW locking.
>
> Fixes: c7f486567c1d ("PCI PM: PCIe PME root port service driver")
> Fixes: f12eb72a268b ("PCI/ASPM: Use PCI Express Capability accessors")
> Fixes: 7d715a6c1ae5 ("PCI: add PCI Express ASPM support")
> Fixes: affa48de8417 ("staging/rdma/hfi1: Add support for enabling/disabling PCIe ASPM")
> Fixes: 849a9366cba9 ("misc: rtsx: Add support new chip rts5228 mmc: rtsx: Add support MMC_CAP2_NO_MMC")
> Fixes: 3d1e7aa80d1c ("misc: rtsx: Use pcie_capability_clear_and_set_word() for PCI_EXP_LNKCTL")
> Fixes: c0e5f4e73a71 ("misc: rtsx: Add support for RTS5261")
> Fixes: 3df4fce739e2 ("misc: rtsx: separate aspm mode into MODE_REG and MODE_CFG")
> Fixes: 121e9c6b5c4c ("misc: rtsx: modify and fix init_hw function")
> Fixes: 19f3bd548f27 ("mfd: rtsx: Remove LCTLR defination")
> Fixes: 773ccdfd9cc6 ("mfd: rtsx: Read vendor setting from config space")
> Fixes: 8275b77a1513 ("mfd: rts5249: Add support for RTS5250S power saving")
> Fixes: 5da4e04ae480 ("misc: rtsx: Add support for RTS5260")
> Fixes: 0f49bfbd0f2e ("tg3: Use PCI Express Capability accessors")
> Fixes: 5e7dfd0fb94a ("tg3: Prevent corruption at 10 / 100Mbps w CLKREQ")
> Fixes: b726e493e8dc ("r8169: sync existing 8168 device hardware start sequences with vendor driver")
> Fixes: e6de30d63eb1 ("r8169: more 8168dp support.")
> Fixes: 8a06127602de ("Bluetooth: hci_bcm4377: Add new driver for BCM4377 PCIe boards")
> Fixes: 6f461f6c7c96 ("e1000e: enable/disable ASPM L0s and L1 and ERT according to hardware errata")
> Fixes: 1eae4eb2a1c7 ("e1000e: Disable L1 ASPM power savings for 82573 mobile variants")
> Fixes: 8060e169e02f ("ath9k: Enable extended synch for AR9485 to fix L0s recovery issue")
> Fixes: 69ce674bfa69 ("ath9k: do btcoex ASPM disabling at initialization time")
> Fixes: f37f05503575 ("mt76: mt76x2e: disable pcie_aspm by default")
> Suggested-by: Lukas Wunner <lukas@...ner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> Cc: stable@...r.kernel.org
No objections, so
Reviewed-by: Rafael J. Wysocki <rafael@...nel.org>
> ---
> drivers/pci/access.c | 20 +++++++++++++++++---
> drivers/pci/probe.c | 1 +
> include/linux/pci.h | 34 ++++++++++++++++++++++++++++++++--
> 3 files changed, 50 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 3c230ca3de58..0b2e90d2f04f 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -497,8 +497,8 @@ int pcie_capability_write_dword(struct pci_dev *dev, int pos, u32 val)
> }
> EXPORT_SYMBOL(pcie_capability_write_dword);
>
> -int pcie_capability_clear_and_set_word(struct pci_dev *dev, int pos,
> - u16 clear, u16 set)
> +int pcie_capability_clear_and_set_word_unlocked(struct pci_dev *dev, int pos,
> + u16 clear, u16 set)
> {
> int ret;
> u16 val;
> @@ -512,7 +512,21 @@ int pcie_capability_clear_and_set_word(struct pci_dev *dev, int pos,
>
> return ret;
> }
> -EXPORT_SYMBOL(pcie_capability_clear_and_set_word);
> +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_unlocked);
> +
> +int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
> + u16 clear, u16 set)
> +{
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&dev->pcie_cap_lock, flags);
> + ret = pcie_capability_clear_and_set_word_unlocked(dev, pos, clear, set);
> + spin_unlock_irqrestore(&dev->pcie_cap_lock, flags);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_locked);
>
> int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
> u32 clear, u32 set)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 0b2826c4a832..53ac0d3287a8 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2318,6 +2318,7 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
> .end = -1,
> };
>
> + spin_lock_init(&dev->pcie_cap_lock);
> #ifdef CONFIG_PCI_MSI
> raw_spin_lock_init(&dev->msi_lock);
> #endif
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 60b8772b5bd4..ab7682ed172f 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -467,6 +467,7 @@ struct pci_dev {
> pci_dev_flags_t dev_flags;
> atomic_t enable_cnt; /* pci_enable_device has been called */
>
> + spinlock_t pcie_cap_lock; /* Protects RMW ops in capability accessors */
> u32 saved_config_space[16]; /* Config space saved at suspend time */
> struct hlist_head saved_cap_space;
> int rom_attr_enabled; /* Display of ROM attribute enabled? */
> @@ -1217,11 +1218,40 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val);
> int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val);
> int pcie_capability_write_word(struct pci_dev *dev, int pos, u16 val);
> int pcie_capability_write_dword(struct pci_dev *dev, int pos, u32 val);
> -int pcie_capability_clear_and_set_word(struct pci_dev *dev, int pos,
> - u16 clear, u16 set);
> +int pcie_capability_clear_and_set_word_unlocked(struct pci_dev *dev, int pos,
> + u16 clear, u16 set);
> +int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
> + u16 clear, u16 set);
> int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
> u32 clear, u32 set);
>
> +/**
> + * pcie_capability_clear_and_set_word - RMW accessor for PCI Express Capability Registers
> + * @dev: PCI device structure of the PCI Express device
> + * @pos: PCI Express Capability Register
> + * @clear: Clear bitmask
> + * @set: Set bitmask
> + *
> + * Perform a Read-Modify-Write (RMW) operation using @clear and @set
> + * bitmasks on PCI Express Capability Register at @pos. Certain PCI Express
> + * Capability Registers are accessed concurrently in RMW fashion, hence
> + * require locking which is handled transparently to the caller.
> + */
> +static inline int pcie_capability_clear_and_set_word(struct pci_dev *dev,
> + int pos,
> + u16 clear, u16 set)
> +{
> + switch (pos) {
> + case PCI_EXP_LNKCTL:
> + case PCI_EXP_RTCTL:
> + return pcie_capability_clear_and_set_word_locked(dev, pos,
> + clear, set);
> + default:
> + return pcie_capability_clear_and_set_word_unlocked(dev, pos,
> + clear, set);
> + }
> +}
> +
> static inline int pcie_capability_set_word(struct pci_dev *dev, int pos,
> u16 set)
> {
> --
> 2.30.2
>
Powered by blists - more mailing lists