From patchwork Tue Mar 20 21:38:47 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Hutchings X-Patchwork-Id: 10298003 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 ADED2602B3 for ; Tue, 20 Mar 2018 21:38:51 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9B63F28CC4 for ; Tue, 20 Mar 2018 21:38:51 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8E5112964A; Tue, 20 Mar 2018 21:38:51 +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=ham 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 0DF7A28CC4 for ; Tue, 20 Mar 2018 21:38:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751480AbeCTViu (ORCPT ); Tue, 20 Mar 2018 17:38:50 -0400 Received: from imap1.codethink.co.uk ([176.9.8.82]:46187 "EHLO imap1.codethink.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751456AbeCTVit (ORCPT ); Tue, 20 Mar 2018 17:38:49 -0400 Received: from [167.98.27.229] (helo=xylophone.i.decadent.org.uk) by imap1.codethink.co.uk with esmtpsa (Exim 4.84_2 #1 (Debian)) id 1eyOy4-0006Fk-JH; Tue, 20 Mar 2018 21:38:52 +0000 Date: Tue, 20 Mar 2018 21:38:47 +0000 From: Ben Hutchings To: qla2xxx-upstream@qlogic.com Cc: linux-scsi@vger.kernel.org Subject: [RFC][PATCH] qla2xxx: Fix race between iocb timeout and free Message-ID: <20180320213846.2gg7genjrsiv2tij@xylophone.i.decadent.org.uk> MIME-Version: 1.0 Content-Disposition: inline User-Agent: NeoMutt/20170113 (1.7.2) 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 qla2x00_sp_free() cancels the iocb timeout timer if it is still pending, but doesn't (and can't) wait for the timer function to complete if it is already running. Add a reference count to async iocb commands to ensure that they aren't freed too early: - One reference is held by the timer, and dropped either at the end of the timer function or after the timer is cancelled - One reference is held by the completion path, and dropped by qla2x00_sp_free() Signed-off-by: Ben Hutchings --- This probably isn't quite right, since it's based on only a brief code review and is untested. And maybe there's some reason that this race condition is somehow avoided. This depends on the previous two fixes I sent for qla2xxx. Ben. drivers/scsi/qla2xxx/qla_def.h | 1 + drivers/scsi/qla2xxx/qla_gs.c | 4 ++-- drivers/scsi/qla2xxx/qla_init.c | 10 +++++++--- drivers/scsi/qla2xxx/qla_inline.h | 12 ++++++++++++ drivers/scsi/qla2xxx/qla_iocb.c | 6 ++---- 5 files changed, 24 insertions(+), 9 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h index c9689f97c307..0337bacd0dc7 100644 --- a/drivers/scsi/qla2xxx/qla_def.h +++ b/drivers/scsi/qla2xxx/qla_def.h @@ -483,6 +483,7 @@ struct srb_iocb { struct timer_list timer; void (*timeout)(void *); + refcount_t ref; }; /* Values for srb_ctx type */ diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c index fcde5ea203c0..e98ba70b7cbe 100644 --- a/drivers/scsi/qla2xxx/qla_gs.c +++ b/drivers/scsi/qla2xxx/qla_gs.c @@ -545,7 +545,7 @@ static void qla2x00_async_sns_sp_done(void *s, int rc) if (!e) goto err2; - del_timer(&sp->u.iocb_cmd.timer); + qla2x00_del_timer(sp); e->u.iosb.sp = sp; qla2x00_post_work(vha, e); return; @@ -4175,7 +4175,7 @@ void qla24xx_async_gpnft_done(scsi_qla_host_t *vha, srb_t *sp) { ql_dbg(ql_dbg_disc, vha, 0xffff, "%s enter\n", __func__); - del_timer(&sp->u.iocb_cmd.timer); + qla2x00_del_timer(sp); qla24xx_async_gnnft(vha, sp, sp->gen2); } diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c index 45319119606a..ecdb78924ca8 100644 --- a/drivers/scsi/qla2xxx/qla_init.c +++ b/drivers/scsi/qla2xxx/qla_init.c @@ -60,6 +60,9 @@ qla2x00_sp_timeout(struct timer_list *t) iocb = &sp->u.iocb_cmd; iocb->timeout(sp); spin_unlock_irqrestore(&vha->hw->hardware_lock, flags); + + if (refcount_dec_and_test(&iocb->ref)) + qla2x00_rel_sp(sp); } void @@ -68,8 +71,9 @@ qla2x00_sp_free(void *ptr) srb_t *sp = ptr; struct srb_iocb *iocb = &sp->u.iocb_cmd; - del_timer(&iocb->timer); - qla2x00_rel_sp(sp); + qla2x00_del_timer(sp); + if (refcount_dec_and_test(&iocb->ref)) + qla2x00_rel_sp(sp); } /* Asynchronous Login/Logout Routines -------------------------------------- */ @@ -1553,7 +1557,7 @@ qla24xx_abort_sp_done(void *ptr, int res) srb_t *sp = ptr; struct srb_iocb *abt = &sp->u.iocb_cmd; - if (del_timer(&sp->u.iocb_cmd.timer)) + if (qla2x00_del_timer(sp)) complete(&abt->u.abt.comp); } diff --git a/drivers/scsi/qla2xxx/qla_inline.h b/drivers/scsi/qla2xxx/qla_inline.h index 06c4a843e2ad..75af2c4bb92f 100644 --- a/drivers/scsi/qla2xxx/qla_inline.h +++ b/drivers/scsi/qla2xxx/qla_inline.h @@ -277,9 +277,21 @@ qla2x00_init_timer(srb_t *sp, unsigned long tmo) init_completion(&sp->u.iocb_cmd.u.fxiocb.fxiocb_comp); if (sp->type == SRB_ELS_DCMD) init_completion(&sp->u.iocb_cmd.u.els_logo.comp); + refcount_set(&sp->u.iocb_cmd.ref, 2); add_timer(&sp->u.iocb_cmd.timer); } +static inline int +qla2x00_del_timer(srb_t *sp) +{ + int rval; + + rval = del_timer(&sp->u.iocb_cmd.timer); + if (rval) + refcount_dec(&sp->u.iocb_cmd.ref); + return rval; +} + static inline int qla2x00_gid_list_size(struct qla_hw_data *ha) { diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c index 6a719c1f8af5..bfde6ebc30d3 100644 --- a/drivers/scsi/qla2xxx/qla_iocb.c +++ b/drivers/scsi/qla2xxx/qla_iocb.c @@ -2384,8 +2384,7 @@ qla2x00_els_dcmd_sp_free(void *data) elsio->u.els_logo.els_logo_pyld, elsio->u.els_logo.els_logo_pyld_dma); - del_timer(&elsio->timer); - qla2x00_rel_sp(sp); + qla2x00_sp_free(sp); } static void @@ -2581,8 +2580,7 @@ qla2x00_els_dcmd2_sp_free(void *data) elsio->u.els_plogi.els_resp_pyld, elsio->u.els_plogi.els_resp_pyld_dma); - del_timer(&elsio->timer); - qla2x00_rel_sp(sp); + qla2x00_sp_free(sp); } static void