linux-kernel - Re: [PATCH V2] PCI: AER: fix deadlock in do

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20171003080954.GA19637@lst.de>
Date:   Tue, 3 Oct 2017 10:09:54 +0200
From:   Christoph Hellwig <hch@....de>
To:     Govindarajulu Varadarajan <gvaradar@...co.com>
Cc:     Christoph Hellwig <hch@....de>, benve@...co.com,
        bhelgaas@...gle.com, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, jlbec@...lplan.org, mingo@...hat.com,
        peterz@...radead.org, okaya@...eaurora.org
Subject: Re: [PATCH V2] PCI: AER: fix deadlock in do_recovery

On Mon, Oct 02, 2017 at 05:14:09PM -0700, Govindarajulu Varadarajan wrote:
>
> I would avoid increasing the size of pci_dev. The list_head would be empty after
> we call aer_pci_walk_bus(). We already have pci_bus *subordinate to link all the
> 'device's. Making list_head available to others would require a lock. I would
> avoid that.

It does not require a new lock if you clearly document the concurrency
semantics.  And I'd much rather add a hlist_node (which should be enough
if we want to be small) to pci_dev than requiring these additional
memory allocations and boiler plate code in an error recovery situation.

>
>>> +static void aer_pci_walk_bus(struct pci_bus *top,
>>> +			     int (*cb)(struct pci_dev *, void *),
>>> +			     struct aer_broadcast_data *result)
>>> +{
>>> +	HLIST_HEAD(dev_hlist);
>>> +	struct hlist_node *tmp;
>>> +	struct aer_device_list *entry;
>>
>> Do we want to offer this as generic PCIe functionality?  If not we can
>> probably hardcode the callback and callback data to simplify this a lot.
>>
>
> I could not find any other code which aquires device_lock in pci_walk_bus cb
> function. Can you tell me how we can hardcore callback and callback data here?

The word is hardcode.  If you follow the above suggestion we won't
really need the aer_pci_walk_bus helper but could just open code most
of the code.  Or maybe just keep passing the cb for simplicity - it
was just extending on the above idea.