netdev - Re: [PATCH net-next 3/3] mlxsw: pci: Lock configuration space of upstream bridge during reset

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240712212157.GA339030@bhelgaas>
Date: Fri, 12 Jul 2024 16:21:57 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Ido Schimmel <idosch@...dia.com>
Cc: Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	Petr Machata <petrm@...dia.com>, mlxsw@...dia.com,
	linux-pci@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH net-next 3/3] mlxsw: pci: Lock configuration space of
 upstream bridge during reset

[+cc Dan]

On Wed, Jul 03, 2024 at 05:42:05PM +0300, Ido Schimmel wrote:
> On Tue, Jul 02, 2024 at 09:35:50AM +0200, Przemek Kitszel wrote:
> > On 7/1/24 18:41, Petr Machata wrote:
> > > From: Ido Schimmel <idosch@...dia.com>
> > > 
> > > The driver triggers a "Secondary Bus Reset" (SBR) by calling
> > > __pci_reset_function_locked() which asserts the SBR bit in the "Bridge
> > > Control Register" in the configuration space of the upstream bridge for
> > > 2ms. This is done without locking the configuration space of the
> > > upstream bridge port, allowing user space to access it concurrently.
> > 
> > This means your patch is a bugfix.
> > 
> > > Linux 6.11 will start warning about such unlocked resets [1][2]:
> > > 
> > > pcieport 0000:00:01.0: unlocked secondary bus reset via: pci_reset_bus_function+0x51c/0x6a0
> > > 
> > > Avoid the warning by locking the configuration space of the upstream
> > > bridge prior to the reset and unlocking it afterwards.
> > 
> > You are not avoiding the warning but protecting concurrent access,
> > please add a Fixes tag.
> 
> The patch that added the missing lock in PCI core was posted without a
> Fixes tag and merged as part of the 6.10 PR. See commit 7e89efc6e9e4
> ("PCI: Lock upstream bridge for pci_reset_function()").
> 
> I don't see a good reason for root to poke in the configuration space of
> the upstream bridge during SBR, but AFAICT the worst that can happen is
> that reset will fail and while it is a bug, it is not a regression.
> 
> Bjorn, do you see a reason to post this as a fix?

Sorry, I was on vacation and missed this when I returned.

mlxsw is one of the few users of __pci_reset_function_locked().
Others are liquidio (octeon), VFIO, and Xen.

You need __pci_reset_function_locked() if you're already holding the
device mutex, i.e., device_lock(&pdev->dev).  I looked at the
mlxsw_pci_reset_at_pci_disable() path, and didn't see where it holds
that device lock, but I probably missed it.

The usual pci_reset_function() path, which would be preferable if you
can use it, does basically this:

  pci_dev_lock(bridge)
    device_lock(&bridge->dev)
    pci_cfg_access_lock(bridge)
  pci_dev_lock(pdev)
    device_lock(&pdev->dev)
    pci_cfg_access_lock(pdev)
  pci_dev_save_and_disable(dev)
  __pci_reset_function_locked(pdev)

This patch adds pci_cfg_access_lock(bridge), but doesn't acquire the
device_lock for the bridge.

It looks like you always reset the device at mlxsw_pci_probe()-time,
which is quite unusual in the first place, but I suppose there's some
good reason for it.

If you can use pci_reset_function() directly (or avoid the reset
altogether), it would be far preferable and would avoid potential
issues like the warning here.

Bjorn

> > > [1] https://lore.kernel.org/all/171711746953.1628941.4692125082286867825.stgit@dwillia2-xfh.jf.intel.com/
> > > [2] https://lore.kernel.org/all/20240531213150.GA610983@bhelgaas/
> > > 
> > > Cc: linux-pci@...r.kernel.org
> > > Signed-off-by: Ido Schimmel <idosch@...dia.com>
> > > Signed-off-by: Petr Machata <petrm@...dia.com>