linux-kernel - Re: [PATCH v2] fusion-mptbase: handle failed allocation for workqueue

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] [day] [month] [year] [list]

Date:	Fri, 19 Feb 2016 14:46:25 +0100
From:	Tomas Henzl <thenzl@...hat.com>
To:	emilne@...hat.com, Johannes Thumshirn <jthumshirn@...e.de>
Cc:	Insu Yun <wuninsu@...il.com>, nagalakshmi.nandigama@...gotech.com,
	praveen.krishnamoorthy@...gotech.com,
	sreekanth.reddy@...gotech.com, abhijit.mahajan@...gotech.com,
	MPT-FusionLinux.pdl@...gotech.com, linux-scsi@...r.kernel.org,
	linux-kernel@...r.kernel.org, taesoo@...ech.edu,
	yeongjin.jang@...ech.edu, insu@...ech.edu, changwoo@...ech.edu
Subject: Re: [PATCH v2] fusion-mptbase: handle failed allocation for workqueue

On 18.2.2016 17:55, Ewan Milne wrote:
> On Thu, 2016-02-18 at 11:40 -0500, Ewan Milne wrote:
>> It also appears to me upon further inspection that the existing code has
>> other problems.  In particular, once mpt_mapresources() has returned
>> with a nonzero error code, it looks like iounmap() should be called, but
>> it is not in the case of a failed allocation of reset_work_q.  I'm also
>> not sure why pci_release_selected_regions() is only called for the case
>> of mpt_do_ioc_recovery() returning != -5 when it is called whenever
>> there is a failed allocation of reset_work_q.
>>
>> Consider the attached patch (untested, because I don't have hardware):
>> It shows what I would do for labels & error handling.  If the rc != -5
>> case of return from mpt_do_ioc_recovery() could be eliminated, then
>> another label "out_free_fw_event_q:" could be added prior to the other
>> error cases at the end, and all the code after the printk() in that path
>> could be replaced by "goto out_free_fw_event_q;"
>>
>> 	if ((r = mpt_do_ioc_recovery(ioc, MPT_HOSTEVENT_IOC_BRINGUP,
>> 	    CAN_SLEEP)) != 0){
>> 		printk(MYIOC_s_ERR_FMT "didn't initialize properly! (%d)\n",
>> 		    ioc->name, r);
>> 		goto out_free_fw_event_q;
>> 	}
>> ...
>>
>> out_free_fw_event_q:
>> 	destroy_workqueue(ioc->fw_event_q);
>> 	ioc->fw_event_q = NULL;
>>
>> out_remove_ioc:
>> ...
>>
>> However I do not know if that change is legitimate.
>>
>> -Ewan
> There is an error in the patch attached in my previous mail.
> Please refer to the attached PATCH v2 RFC.

Looks fine to me, do you we could add this change to your patch ?


diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index a4f362b3b4..4a42c1534c 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -2012,6 +2012,8 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
                if (ioc->alt_ioc)
                        ioc->alt_ioc->alt_ioc = NULL;
                iounmap(ioc->memmap);
+               if (pci_is_enabled(pdev)
+                       pci_disable_device(pdev);
                if (r != -5)
                        pci_release_selected_regions(pdev, ioc->bars);

@@ -2019,7 +2021,6 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
                ioc->reset_work_q = NULL;

                kfree(ioc);
-               pci_set_drvdata(pdev, NULL);
                return r;
        }

@@ -2052,13 +2053,13 @@ out_remove_ioc:
        list_del(&ioc->list);
        if (ioc->alt_ioc)
                ioc->alt_ioc->alt_ioc = NULL;
-       pci_set_drvdata(ioc->pcidev, NULL);

        destroy_workqueue(ioc->reset_work_q);
        ioc->reset_work_q = NULL;

 out_unmap_resources:
        iounmap(ioc->memmap);
+       pci_disable_device(pdev);
        pci_release_selected_regions(pdev, ioc->bars);

 out_free_ioc:

-tomash

>
> -Ewan
>