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: <2887936e2d655834ea28e07957b1c1ccd9e68e27.camel@redhat.com>
Date: Wed, 04 Sep 2024 09:06:37 +0200
From: Philipp Stanner <pstanner@...hat.com>
To: 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, Damien Le Moal <dlemoal@...nel.org>
Subject: Re: [PATCH] PCI: Fix devres regression in pci_intx()

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"?

>   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().

I suppose your solution is the better one, though.


P.

>   Thanks,
> 
> Alex
>