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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <alpine.LNX.2.20.1710021606440.30867@cae-iprp-alln-lb.cisco.com>
Date:   Mon, 2 Oct 2017 17:14:09 -0700
From:   Govindarajulu Varadarajan <gvaradar@...co.com>
To:     Christoph Hellwig <hch@....de>
CC:     <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 Sun, 1 Oct 2017, Christoph Hellwig wrote:

>> +struct aer_device_list {
>> +	struct device *dev;
>> +	struct hlist_node node;
>> +};
>> +
>>  extern struct bus_type pcie_port_bus_type;
>>  void aer_isr(struct work_struct *work);
>>  void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
>> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
>> index 890efcc574cb..d524f2c2c288 100644
>> --- a/drivers/pci/pcie/aer/aerdrv_core.c
>> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
>> @@ -346,6 +346,47 @@ static int report_resume(struct pci_dev *dev, void *data)
>>  	return 0;
>>  }
>>
>> +int aer_get_pci_dev(struct pci_dev *pdev, void *data)
>> +{
>> +	struct hlist_head *hhead = (struct hlist_head *)data;
>> +	struct device *dev = &pdev->dev;
>> +	struct aer_device_list *entry;
>> +
>> +	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
>> +	if (!entry)
>> +		/* continue with other devices, lets not return error */
>> +		return 0;
>> +
>> +	entry->dev = get_device(dev);
>> +	hlist_add_head(&entry->node, hhead);
>> +
>> +	return 0;
>
> Can we just embedded the list_head in the pci_dev?  Or is there a way
> we can have multiple chains like this pending at the same time?
>

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.

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