linux-kernel - Re: [PATCH V2 3/5] PCI: save and restore bus on parent bus 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: <3bca2bc1-bc7e-ad71-eb66-b74f6e0b381b@codeaurora.org>
Date:   Sun, 2 Oct 2016 23:34:13 -0400
From:   Sinan Kaya <okaya@...eaurora.org>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     linux-pci@...r.kernel.org, timur@...eaurora.org,
        cov@...eaurora.org, alex.williamson@...hat.com,
        vikrams@...eaurora.org, linux-arm-msm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 3/5] PCI: save and restore bus on parent bus reset

On 9/29/2016 7:50 PM, Sinan Kaya wrote:
> Hi Bjorn,
> 
> On 9/29/2016 5:49 PM, Bjorn Helgaas wrote:
>>> +	}
>> This pattern of "unlock, do something, relock" needs some
>> justification.  In general it's unsafe because the lock is protecting
>> *something*, and you have to assume that something can change as soon
>> as you unlock.  Maybe you know it's safe in this situation, and if so,
>> the explanation of why it's safe is what I'm looking for.
> 
> Agreed. 
> 
> The problem is that save and restore routines obtain the lock again and
> they fails as the lock is already held.
> 
> The alternative is to change the dev_locks in save and restore to try_lock
> so that it will work if locks were previously obtained or not. 
> 
>>
>> Also, you're now calling pci_reset_bridge_secondary_bus() with the dev
>> unlocked, where we called it with the dev locked before.  Some (but
>> worryingly, not all) of the other pci_reset_bridge_secondary_bus()
>> callers also have the dev locked.  I didn't look long enough to figure
>> out if there is a strategy there or if these inconsistencies are
>> latent bugs.
>>
> 
> The goal of this routine is to reset the device not the bridge and the code
> will use FLR or others if available. Therefore, it makes perfect sense to
> obtain the device lock while doing this. 
> 
> The code tries to reset the bus if none of the other resets work. This is
> where the problem is. It destroys the context for other devices.
> 
> I can fix get rid of this unlock, do something and then lock again business
> by rewriting the locks in save and restore.
> 
> Sinan
> 

I looked at the code a little bit more. I missed the fact pci_parent_bus_reset
will only work if there is only PCI device on the bus and that device is the
reset requesting device.

I'll drop this patch as well as the infiniband version for the same reason.




-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.