Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753043AbaAPNQz (ORCPT ); Thu, 16 Jan 2014 08:16:55 -0500 Received: from mga09.intel.com ([134.134.136.24]:55134 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752954AbaAPNQ2 (ORCPT ); Thu, 16 Jan 2014 08:16:28 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.95,667,1384329600"; d="scan'208";a="467646891" Subject: [PATCH] isci: reformulate for_each_isci_host macro to fix an oops To: JBottomley@Parallels.com From: Lukasz Dorau Cc: pawel.baldysiak@intel.com, dave.jiang@intel.com, maciej.patelczyk@intel.com, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org Date: Thu, 16 Jan 2014 14:16:24 +0100 Message-ID: <20140116131624.24112.8745.stgit@gklab-154-244.igk.intel.com> User-Agent: StGit/0.16 MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org The loop 'for' in macro 'for_each_isci_host' (drivers/scsi/isci/init.c:717) is executed more times than it can be. Regardless the condition: 'id < ARRAY_SIZE(to_pci_info(pdev)->hosts)' (drivers/scsi/isci/host.h:315) it is executed when id equals ARRAY_SIZE(to_pci_info(pdev)->hosts) too. (Remark: ARRAY_SIZE(to_pci_info(pdev)->hosts) always equals SCI_MAX_CONTROLLERS = 2) It sounds crazy, but it is truth. I have checked it in the following way: I have added the line: printk(KERN_ERR ">>> (%d < %d) == %d \n", \ i, SCI_MAX_CONTROLLERS, (i < SCI_MAX_CONTROLLERS)); after the 'for_each_isci_host' macro in drivers/scsi/isci/init.c:701 and received the following output: >>> (0 < 2) == 1 >>> (1 < 2) == 1 >>> (2 < 2) == 1 after issuing 'modprobe isci' command on platform with two SCU controllers (Patsburg D or T chipset required). The kernel was compiled using gcc version 4.8.2. This patch does not introduce any functional changes. It only reformulates the 'for_each_isci_host' macro and the relevant code in the drivers/scsi/isci/init.c file and fixes the following oops after 'rmmod isci': BUG: unable to handle kernel NULL pointer dereference at (null) IP: [] __list_add+0x1b/0xc0 Oops: 0000 [#1] SMP RIP: 0010:[] [] __list_add+0x1b/0xc0 Call Trace: [] __mutex_lock_slowpath+0x114/0x1b0 [] mutex_lock+0x1f/0x30 [] sas_disable_events+0x1b/0x50 [libsas] [] sas_unregister_ha+0x18/0x60 [libsas] [] isci_unregister+0x1e/0x40 [isci] [] isci_pci_remove+0x5d/0x100 [isci] [] pci_device_remove+0x3b/0xb0 [] __device_release_driver+0x7f/0xf0 [] driver_detach+0xa8/0xb0 [] bus_remove_driver+0x9b/0x120 [] driver_unregister+0x2c/0x50 [] pci_unregister_driver+0x23/0x80 [] isci_exit+0x10/0x1e [isci] [] SyS_delete_module+0x16b/0x2d0 [] ? do_notify_resume+0x61/0xa0 [] system_call_fastpath+0x16/0x1b Signed-off-by: Lukasz Dorau Tested-by: Pawel Baldysiak Cc: Maciej Patelczyk Cc: Dave Jiang Cc: --- drivers/scsi/isci/host.h | 8 ++++---- drivers/scsi/isci/init.c | 20 ++++++++++++++------ 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/drivers/scsi/isci/host.h b/drivers/scsi/isci/host.h index 4911310..2002fdf 100644 --- a/drivers/scsi/isci/host.h +++ b/drivers/scsi/isci/host.h @@ -310,10 +310,10 @@ static inline struct Scsi_Host *to_shost(struct isci_host *ihost) return ihost->sas_ha.core.shost; } -#define for_each_isci_host(id, ihost, pdev) \ - for (id = 0, ihost = to_pci_info(pdev)->hosts[id]; \ - id < ARRAY_SIZE(to_pci_info(pdev)->hosts) && ihost; \ - ihost = to_pci_info(pdev)->hosts[++id]) +#define for_each_isci_host(id, ihost, hosts) \ + for (id = 0, ihost = hosts[0]; \ + (id < SCI_MAX_CONTROLLERS) && ihost; \ + ihost = hosts[++id]) static inline void wait_for_start(struct isci_host *ihost) { diff --git a/drivers/scsi/isci/init.c b/drivers/scsi/isci/init.c index d25d0d8..5a07b7b 100644 --- a/drivers/scsi/isci/init.c +++ b/drivers/scsi/isci/init.c @@ -346,6 +346,7 @@ static int isci_setup_interrupts(struct pci_dev *pdev) int err, i, num_msix; struct isci_host *ihost; struct isci_pci_info *pci_info = to_pci_info(pdev); + struct isci_host **hosts; /* * Determine the number of vectors associated with this @@ -390,7 +391,8 @@ static int isci_setup_interrupts(struct pci_dev *pdev) return 0; intx: - for_each_isci_host(i, ihost, pdev) { + hosts = pci_info->hosts; + for_each_isci_host(i, ihost, hosts) { err = devm_request_irq(&pdev->dev, pdev->irq, isci_intx_isr, IRQF_SHARED, DRV_NAME"-intx", ihost); if (err) @@ -621,6 +623,7 @@ static int isci_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) struct isci_pci_info *pci_info; int err, i; struct isci_host *isci_host; + struct isci_host **hosts; const struct firmware *fw = NULL; struct isci_orom *orom = NULL; char *source = "(platform)"; @@ -698,13 +701,15 @@ static int isci_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (err) goto err_host_alloc; - for_each_isci_host(i, isci_host, pdev) + hosts = pci_info->hosts; + for_each_isci_host(i, isci_host, hosts) scsi_scan_host(to_shost(isci_host)); return 0; err_host_alloc: - for_each_isci_host(i, isci_host, pdev) + hosts = pci_info->hosts; + for_each_isci_host(i, isci_host, hosts) isci_unregister(isci_host); return err; } @@ -712,9 +717,10 @@ static int isci_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) static void isci_pci_remove(struct pci_dev *pdev) { struct isci_host *ihost; + struct isci_host **hosts = to_pci_info(pdev)->hosts; int i; - for_each_isci_host(i, ihost, pdev) { + for_each_isci_host(i, ihost, hosts) { wait_for_start(ihost); isci_unregister(ihost); isci_host_deinit(ihost); @@ -726,9 +732,10 @@ static int isci_suspend(struct device *dev) { struct pci_dev *pdev = to_pci_dev(dev); struct isci_host *ihost; + struct isci_host **hosts = to_pci_info(pdev)->hosts; int i; - for_each_isci_host(i, ihost, pdev) { + for_each_isci_host(i, ihost, hosts) { sas_suspend_ha(&ihost->sas_ha); isci_host_deinit(ihost); } @@ -744,6 +751,7 @@ static int isci_resume(struct device *dev) { struct pci_dev *pdev = to_pci_dev(dev); struct isci_host *ihost; + struct isci_host **hosts = to_pci_info(pdev)->hosts; int rc, i; pci_set_power_state(pdev, PCI_D0); @@ -758,7 +766,7 @@ static int isci_resume(struct device *dev) pci_set_master(pdev); - for_each_isci_host(i, ihost, pdev) { + for_each_isci_host(i, ihost, hosts) { sas_prep_resume_ha(&ihost->sas_ha); isci_host_init(ihost); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/