linux-kernel - Re: [PATCH] PCI: Fix devres regression in pci

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: <dcbf9292616816bbce020994adb18e2c32597aeb.camel@redhat.com>
Date: Wed, 04 Sep 2024 15:37:25 +0200
From: Philipp Stanner <pstanner@...hat.com>
To: Damien Le Moal <dlemoal@...nel.org>, Alex Williamson
	 <alex.williamson@...hat.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, Krzysztof
 Wilczyński
	 <kwilczynski@...nel.org>, linux-pci@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI: Fix devres regression in pci_intx()

On Wed, 2024-09-04 at 17:25 +0900, Damien Le Moal wrote:
> On 2024/09/04 16:06, Philipp Stanner wrote:
> > On Tue, 2024-09-03 at 09:44 -0600, Alex Williamson wrote:
> > > On Thu, 25 Jul 2024 14:07:30 +0200
> > > Philipp Stanner <pstanner@...hat.com> wrote:
> > > 
> > > > pci_intx() is a function that becomes managed if
> > > > pcim_enable_device()
> > > > has been called in advance. Commit 25216afc9db5 ("PCI: Add
> > > > managed
> > > > pcim_intx()") changed this behavior so that pci_intx() always
> > > > leads
> > > > to
> > > > creation of a separate device resource for itself, whereas
> > > > earlier,
> > > > a
> > > > shared resource was used for all PCI devres operations.
> > > > 
> > > > Unfortunately, pci_intx() seems to be used in some drivers'
> > > > remove()
> > > > paths; in the managed case this causes a device resource to be
> > > > created
> > > > on driver detach.
> > > > 
> > > > Fix the regression by only redirecting pci_intx() to its
> > > > managed
> > > > twin
> > > > pcim_intx() if the pci_command changes.
> > > > 
> > > > Fixes: 25216afc9db5 ("PCI: Add managed pcim_intx()")
> > > 
> > > I'm seeing another issue from this, which is maybe a more general
> > > problem with managed mode.  In my case I'm using vfio-pci to
> > > assign
> > > an
> > > ahci controller to a VM.
> > 
> > "In my case" doesn't mean OOT, does it? I can't fully follow.
> > 
> > >   ahci_init_one() calls pcim_enable_device()
> > > which sets is_managed = true.  I notice that nothing ever sets
> > > is_managed to false.  Therefore now when I call pci_intx() from
> > > vfio-
> > > pci
> > > under spinlock, I get a lockdep warning
> > 
> > I suppose you see the lockdep warning because the new pcim_intx()
> > can 
> > now allocate, whereas before 25216afc9db5 it was
> > pcim_enable_device()
> > which allocated *everything* related to PCI devres.
> > 
> > >  as I no go through pcim_intx()
> > > code after 25216afc9db5 
> > 
> > You alwas went through pcim_intx()'s logic. The issue seems to be
> > that
> > the allocation step was moved.
> > 
> > > since the previous driver was managed.
> > 
> > what do you mean by "previous driver"?
> 
> The AHCI driver... When attaching a PCI dev to vfio to e.g.
> passthrough to a VM,
> the device driver must first be unbound and the device bound to vfio-
> pci. So we
> switch from ahci/libata driver to vfio. When vfio tries to enable
> intx with
> is_managed still true from the use of the device by ahci, problem
> happen.
> 
> > 
> > >   It seems
> > > like we should be setting is_managed to false is the driver
> > > release
> > > path, right?
> > 
> > So the issue seems to be that the same struct pci_dev can be used
> > by
> > different drivers, is that correct?
> > 
> > If so, I think that can be addressed trough having
> > pcim_disable_device() set is_managed to false as you suggest.
> > 
> > Another solution can could at least consider would be to use a
> > GFP_ATOMIC for allocation in get_or_create_intx_devres().
> 
> If it is allowed to call pci_intx() under a spin_lock, then we need
> GFP_ATOMIC.
> If not, then vfio-pci needs to move the call out of the spinlock.

If vfio-pci can get rid of pci_intx() alltogether, that might be a good
thing. As far as I understood Andy Shevchenko, pci_intx() is outdated.
There's only a hand full of users anyways.


Best solution would be to avoid GFP_ATOMIC and see first if setting
is_managed = false solves the reported problem for now.

Other problematic users should hopefully be found through lockdep, too.
Though I think they are unlikely to occur

> 
> Either solution must be implemented regardless of the fix to set
> is_managed to
> false.

Yes

> 
> So what context is allowed to call pci_intx() ? The current kdoc
> comment does
> not say...

the old pci_intx() did not allocate.
It only calls pci_read_config_word() and pci_write_config_word(). If
those cannot block etc. it should be save from any context.

Though I'd like to hear from one of the maintainers about it.

The new version allocates if pcim_enable_device() was called when it
runs for the first time. That first run would then be illegal in must-
not-sleep contexts.


P.