From patchwork Mon Sep 18 19:17:14 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Meelis Roos X-Patchwork-Id: 9957475 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id AFE2060385 for ; Mon, 18 Sep 2017 19:17:34 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A1EC6204C2 for ; Mon, 18 Sep 2017 19:17:34 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 96BB522638; Mon, 18 Sep 2017 19:17:34 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 194B6204C2 for ; Mon, 18 Sep 2017 19:17:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750772AbdIRTRU (ORCPT ); Mon, 18 Sep 2017 15:17:20 -0400 Received: from smtp2.it.da.ut.ee ([193.40.5.67]:45492 "EHLO smtp2.it.da.ut.ee" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750733AbdIRTRS (ORCPT ); Mon, 18 Sep 2017 15:17:18 -0400 Received: from math.ut.ee (unknown [IPv6:2001:bb8:2002:2400:5054:ff:fe3b:8db9]) by smtp2.it.da.ut.ee (Postfix) with ESMTP id 8B28B73D88A; Mon, 18 Sep 2017 22:17:16 +0300 (EEST) Received: by math.ut.ee (Postfix, from userid 1014) id 58FDD221C3C; Mon, 18 Sep 2017 22:17:14 +0300 (EEST) Received: from localhost (localhost [127.0.0.1]) by math.ut.ee (Postfix) with ESMTP id 74AF2221C37; Mon, 18 Sep 2017 22:17:14 +0300 (EEST) Date: Mon, 18 Sep 2017 22:17:14 +0300 (EEST) From: Meelis Roos To: linux-scsi@vger.kernel.org, Linux Kernel list , Hannes Reinecke , "James E.J. Bottomley" , "Martin K. Petersen" Subject: Re: Another UBSAN warning from aic7xxx In-Reply-To: Message-ID: References: User-Agent: Alpine 2.21 (LRH 202 2017-01-01) MIME-Version: 1.0 Sender: linux-scsi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP > Despite the first one being a false positive, her I am reporting another > one, from a older dual P2 server with Adaptec aic7896/97 Ultra2 SCSI > adapter and QUANTUM VIKING II 4.5WSE HDD-s. The same happens in other machines, like IBM xSeries 305, and is still present in 2017, in 4.14-rc1. I debugged it a little. The warning comes from ahc_init(): /* * Only allow target mode features if this unit has them enabled. */ if ((AHC_TMODE_ENABLE & (0x1 << ahc->unit)) == 0) ahc->features &= ~AHC_TARGETMODE; Here, ahc->unit seems to be -1. ahc->unit is initialized to -1 in ahc_alloc() and can be later set by ahc_set_unit(). Stacktrace shows we come from ahc_linux_pci_dev_probe(). This function does ahc_alloc() and sets up PCI-specific configuration of the ahc structure. Then it calls ahc_pci_config() that in turn calls ahc_init() with ahc->unit still set to -1. ahc->unit initialized later: ahc_linux_pci_dev_probe() finishes with call to ahc_linux_register_host(). ahc_linux_register_host() does among other things ahc_set_unit(ahc, ahc_linux_unit++); where ahc_linux_unit is defined as static int ahc_linux_unit; so it seems uninitialized in addition to being assigned too late. With that in mind, if ((AHC_TMODE_ENABLE & (0x1 << ahc->unit)) == 0) is clearly doing its check too early, or the number allocation is done too late. AHC_TMODE_ENABLE is defined 0 in a header, but is seems to be a compile-time bitmask of what AHC cards are to be used in target mode (as in BSD). Perhaps we should do the ahc->unit number allocation in ahc_linux_pci_dev_probe(). I also can not understand the logic in ahc_lock() and ahc_unlock() around ahc_set_unit() - they use &s for lock, and s is defined locally in the ahc_linux_register_host() function as u_long s (uninitialized). Happens to work by luck? Below is a patch that seems to cure it for me - moving the unit initialization earlier, and into another file, and removing the lock around it. Will this work or should we fix it differently? Fixing this reveals another UBSAN warning from the same driver, will fix that too. > [ 1.730498] ================================================================================ > [ 1.730693] UBSAN: Undefined behaviour in drivers/scsi/aic7xxx/aic7xxx_core.c:5325:31 > [ 1.730872] shift exponent -1 is negative > [ 1.730993] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.6.0 #14 > [ 1.731110] Hardware name: To be Filled To be Filled/Intel 440BX/GX, BIOS 063101 07/15/99 > [ 1.731287] 00000000 c135ccec 00000046 f6461c50 00000001 c13974db f6461c44 ffffffff > [ 1.731757] c1397a66 c1775bd0 f6461c54 c10f28c1 f7269638 00000202 0000312d f72695f8 > [ 1.732026] 00000004 f7269618 00000058 f7269638 00000056 00ff473c 80000500 00000000 > [ 1.732026] Call Trace: > [ 1.732026] [] ? dump_stack+0x45/0x69 > [ 1.732026] [] ? ubsan_epilogue+0xb/0x40 > [ 1.732026] [] ? __ubsan_handle_shift_out_of_bounds+0xd6/0x120 > [ 1.732026] [] ? hrtimer_interrupt+0x101/0x390 > [ 1.732026] [] ? scrdown+0x30/0x1bb > [ 1.732026] [] ? pci_conf1_read+0x90/0x150 > [ 1.732026] [] ? ahc_init+0xb2b/0xd90 > [ 1.732026] [] ? pci_read+0x37/0x70 > [ 1.732026] [] ? pci_bus_read_config_byte+0x68/0xb0 > [ 1.732026] [] ? ahc_pci_read_config+0x40/0xd0 > [ 1.732026] [] ? ahc_pci_config+0x4a4/0x10f0 > [ 1.732026] [] ? rgb_foreground+0xdc/0xdd > [ 1.732026] [] ? ahc_linux_pci_dev_probe+0xed/0x320 > [ 1.732026] [] ? kernfs_add_one+0x147/0x1b0 > [ 1.732026] [] ? kernfs_new_node+0x36/0x80 > [ 1.732026] [] ? __pm_runtime_resume+0x3c/0x60 > [ 1.732026] [] ? pci_device_probe+0x91/0x130 > [ 1.732026] [] ? driver_probe_device+0xc8/0x330 > [ 1.732026] [] ? driver_probe_device+0x330/0x330 > [ 1.732026] [] ? driver_probe_device+0x330/0x330 > [ 1.732026] [] ? __driver_attach+0x99/0xd0 > [ 1.732026] [] ? bus_for_each_dev+0x4c/0x90 > [ 1.732026] [] ? driver_attach+0x1a/0x40 > [ 1.732026] [] ? driver_probe_device+0x330/0x330 > [ 1.732026] [] ? bus_add_driver+0x127/0x290 > [ 1.732026] [] ? spi_transport_init+0x1aa/0x1aa > [ 1.732026] [] ? driver_register+0x67/0x120 > [ 1.732026] [] ? ahc_linux_init+0x82/0x8f > [ 1.732026] [] ? do_one_initcall+0x8a/0x260 > [ 1.732026] [] ? spi_transport_init+0x1aa/0x1aa > [ 1.732026] [] ? parameq+0xf/0xb0 > [ 1.732026] [] ? parse_args+0x205/0x510 > [ 1.732026] [] ? kernel_init_freeable+0x153/0x25c > [ 1.732026] [] ? kernel_init_freeable+0x1d0/0x25c > [ 1.732026] [] ? kernel_init+0x9/0x120 > [ 1.732026] [] ? schedule_tail+0xb/0xa0 > [ 1.732026] [] ? ret_from_kernel_thread+0x21/0x38 > [ 1.732026] [] ? rest_init+0x60/0x60 > [ 1.732026] ================================================================================ diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm.c b/drivers/scsi/aic7xxx/aic7xxx_osm.c index acd687f4554e..436b8e0462dd 100644 --- a/drivers/scsi/aic7xxx/aic7xxx_osm.c +++ b/drivers/scsi/aic7xxx/aic7xxx_osm.c @@ -376,8 +376,6 @@ static int ahc_linux_run_command(struct ahc_softc*, static void ahc_linux_setup_tag_info_global(char *p); static int aic7xxx_setup(char *s); -static int ahc_linux_unit; - /************************** OS Utility Wrappers *******************************/ void @@ -1090,7 +1088,6 @@ ahc_linux_register_host(struct ahc_softc *ahc, struct scsi_host_template *templa char buf[80]; struct Scsi_Host *host; char *new_name; - u_long s; int retval; template->name = ahc->description; @@ -1109,9 +1106,6 @@ ahc_linux_register_host(struct ahc_softc *ahc, struct scsi_host_template *templa host->max_lun = AHC_NUM_LUNS; host->max_channel = (ahc->features & AHC_TWIN) ? 1 : 0; host->sg_tablesize = AHC_NSEG; - ahc_lock(ahc, &s); - ahc_set_unit(ahc, ahc_linux_unit++); - ahc_unlock(ahc, &s); sprintf(buf, "scsi%d", host->host_no); new_name = kmalloc(strlen(buf) + 1, GFP_ATOMIC); if (new_name != NULL) { diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c index 0fc14dac7070..0c1231f7610f 100644 --- a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c +++ b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c @@ -42,6 +42,8 @@ #include "aic7xxx_osm.h" #include "aic7xxx_pci.h" +static int ahc_linux_unit = 0; + /* Define the macro locally since it's different for different class of chips. */ #define ID(x) ID_C(x, PCI_CLASS_STORAGE_SCSI) @@ -249,10 +251,12 @@ ahc_linux_pci_dev_probe(struct pci_dev *pdev, const struct pci_device_id *ent) return (-ENODEV); } } + ahc_set_unit(ahc, ahc_linux_unit++); ahc->dev_softc = pci; error = ahc_pci_config(ahc, entry); if (error != 0) { ahc_free(ahc); + ahc_linux_unit--; return (-error); }