linux-kernel - Re: [PATCH 1/3] PCI: ensure the PCI device is locked over ->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: <20170613140504.GB7128@bhelgaas-glaptop.roam.corp.google.com>
Date:   Tue, 13 Jun 2017 09:05:05 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Christoph Hellwig <hch@....de>
Cc:     rakesh@...era.com, linux-pci@...r.kernel.org,
        linux-nvme@...ts.infradead.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] PCI: ensure the PCI device is locked over
 ->reset_notify calls

On Tue, Jun 13, 2017 at 09:08:10AM +0200, Christoph Hellwig wrote:
> On Mon, Jun 12, 2017 at 06:14:23PM -0500, Bjorn Helgaas wrote:
> > My main concern is being able to verify the locking.  I think that is
> > much easier if the locking is adjacent to the method invocation.  But
> > if you just add a comment at the method invocation about where the
> > locking is, that should be sufficient.
> 
> Ok.  I can add comments for all the methods as a separate patch,
> similar to Documentation/vfs/Locking
> 
> > > Yes, I mentioned this earlier, and I also vaguely remember we got
> > > bug reports from IBM on power for this a while ago.  I just don't
> > > feel confident enough to touch all these without a good test plan.
> > 
> > Hmmm.  I see your point, but I hate leaving a known bug unfixed.  I
> > wonder if some enterprising soul could tickle this bug by injecting
> > errors while removing and rescanning devices below the bridge?
> 
> I'm completely loaded up at the moment, but this sounds like a good
> idea.
> 
> In the meantime how do you want to proceed with this patch?

Can you just add comments about the locking?  I'd prefer that in the
same patch that adds the locking because that's what I had a hard time
reviewing.  I'm not thinking of anything fancy like
Documentation/filesystems/Locking; I'm just thinking of something
along the lines of "caller must hold pci_dev_lock() to protect
err_handler->reset_notify from racing ->remove()".  And the changelog
should contain the general principle about the locking strategy.

Bjorn