From patchwork Tue Jan 12 10:24:04 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Nicholas A. Bellinger" X-Patchwork-Id: 8016031 Return-Path: X-Original-To: patchwork-linux-scsi@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 0DCC59F1C0 for ; Tue, 12 Jan 2016 10:28:52 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id B8F93202EB for ; Tue, 12 Jan 2016 10:28:50 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C80C120295 for ; Tue, 12 Jan 2016 10:28:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964924AbcALK0i (ORCPT ); Tue, 12 Jan 2016 05:26:38 -0500 Received: from mail-ob0-f171.google.com ([209.85.214.171]:35451 "EHLO mail-ob0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964939AbcALK0S (ORCPT ); Tue, 12 Jan 2016 05:26:18 -0500 Received: by mail-ob0-f171.google.com with SMTP id py5so55514186obc.2 for ; Tue, 12 Jan 2016 02:26:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=daterainc-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=3Pee3nIezjnvF0hZ93ToZNpqEr7J8WWkjgFGUAtRtQA=; b=Fno+51pzmsyn7pDAM5peC8KWfXNKniWmJo4QDefGnHpfmUDPvUkHnLMeZgYKIf8o5i HMtX4H/DdPxGHUz3xexRgH3M9U25UVU2c8gRMYukP2radVeCp3tAFJq+/KjQdQpgB7/f hcYMCTEDeyXXRDnlEPfJo8Ch5+OfnMDJ4nCH3UlxvHXBsL7FUH6zeSDnKHj9ABge6FGC 9CQjZWURV8qUdMc1AUhNqbmDW5KD1bMHsZQk+jNKRavfdXHn+CbYvn9ak5q7IvNQLxok /XYAbIP/rID4mTz1L9zJlexE0DTgp6ZHrVQK1IP5lPRPVQvjVldg/Zfp0dTdD5Ne9IoR QqzQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=3Pee3nIezjnvF0hZ93ToZNpqEr7J8WWkjgFGUAtRtQA=; b=hN2sZiRMIHt8yvnBrkYcrifLa3d6nfjRBcWsH2qzCEYKzXFsQrE8DE2fvc+4iO4p/5 4zLK2zAxHXUEDGYSi10ojEcDbjmQ+S6iE2avRXSovKjKkT4wk82ESBR6nKwWRYEqAq3Y ox7GDFAY82RC3Mun42pTUviEQhobh9sersyi6k5CnG54BkCFOASsY9YzlDssos0tWjO2 A1LghmwR+oUx3k7OlgqDZhhrPPfmcjWNknIhvVcG7uyLjEA/o4YmGxWll5ksuKIlDAM5 8ueyijD9wPnxNjNulX5GJy/ZOLB3Kjco7LTu/yK0F7MUkdNeiCJVj0V3Xk7k431qQCAm dcXg== X-Gm-Message-State: ALoCoQkBkJsVap+G5bKGj2h97HdUYT2ifYohh2zDMwps/U9lJ44cHRYhEhM66Idl/xoqx99Pb+zvrYHgZcoCpDh9JMfqtOZs3w== X-Received: by 10.182.245.167 with SMTP id xp7mr94285210obc.63.1452594377596; Tue, 12 Jan 2016 02:26:17 -0800 (PST) Received: from localhost.localdomain (mail.linux-iscsi.org. [67.23.28.174]) by smtp.gmail.com with ESMTPSA id cd5sm33032547oec.15.2016.01.12.02.26.16 (version=TLS1 cipher=AES128-SHA bits=128/128); Tue, 12 Jan 2016 02:26:17 -0800 (PST) From: "Nicholas A. Bellinger" To: target-devel Cc: linux-scsi , Quinn Tran , Himanshu Madhani , Sagi Grimberg , Christoph Hellwig , Hannes Reinecke , Andy Grover , Nicholas Bellinger Subject: [PATCH 1/2] target: Fix LUN_RESET active I/O handling for ACK_KREF Date: Tue, 12 Jan 2016 10:24:04 +0000 Message-Id: <1452594245-921-2-git-send-email-nab@daterainc.com> X-Mailer: git-send-email 1.7.2.5 In-Reply-To: <1452594245-921-1-git-send-email-nab@daterainc.com> References: <1452594245-921-1-git-send-email-nab@daterainc.com> Sender: linux-scsi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP From: Nicholas Bellinger This patch fixes a NULL pointer se_cmd->cmd_kref < 0 refcount bug during TMR LUN_RESET with active se_cmd I/O, that can be triggered during se_cmd descriptor shutdown + release via core_tmr_drain_state_list() code. To address this bug, add common __target_check_io_state() helper for ABORT_TASK + LUN_RESET w/ CMD_T_COMPLETE checking, and set CMD_T_ABORTED + obtain ->cmd_kref for both cases ahead of last target_put_sess_cmd() after TFO->aborted_task() -> transport_cmd_finish_abort() callback has completed. It also introduces SCF_ACK_KREF to determine when transport_cmd_finish_abort() needs to drop the second extra reference, ahead of calling target_put_sess_cmd() for the final kref_put(&se_cmd->cmd_kref). Finally, move transport_put_cmd() release of SGL + TMR + extended CDB memory into target_free_cmd_mem() in order to avoid potential resource leaks in TMR ABORT_TASK + LUN_RESET code-paths. Also update target_release_cmd_kref() accordingly. Cc: Quinn Tran Cc: Himanshu Madhani Cc: Sagi Grimberg Cc: Christoph Hellwig Cc: Hannes Reinecke Cc: Andy Grover Signed-off-by: Nicholas Bellinger --- drivers/target/target_core_tmr.c | 72 +++++++++++++++++++++++++--------- drivers/target/target_core_transport.c | 49 ++++++++++++----------- include/target/target_core_base.h | 1 + 3 files changed, 78 insertions(+), 44 deletions(-) diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c index 7137042..af4adb6 100644 --- a/drivers/target/target_core_tmr.c +++ b/drivers/target/target_core_tmr.c @@ -107,6 +107,29 @@ static int target_check_cdb_and_preempt(struct list_head *list, return 1; } +/* + * Called with se_session->sess_cmd_lock held with irq disabled + */ +static bool __target_check_io_state(struct se_cmd *se_cmd) +{ + struct se_session *sess = se_cmd->se_sess; + + if (!sess) + return false; + + spin_lock(&se_cmd->t_state_lock); + if (se_cmd->transport_state & CMD_T_COMPLETE) { + printk("Attempted to abort io tag: %llu already complete," + " skipping\n", se_cmd->tag); + spin_unlock(&se_cmd->t_state_lock); + return false; + } + se_cmd->transport_state |= CMD_T_ABORTED; + spin_unlock(&se_cmd->t_state_lock); + + return kref_get_unless_zero(&se_cmd->cmd_kref); +} + void core_tmr_abort_task( struct se_device *dev, struct se_tmr_req *tmr, @@ -132,27 +155,23 @@ void core_tmr_abort_task( printk("ABORT_TASK: Found referenced %s task_tag: %llu\n", se_cmd->se_tfo->get_fabric_name(), ref_tag); - - spin_lock(&se_cmd->t_state_lock); - if (se_cmd->transport_state & CMD_T_COMPLETE) { - printk("ABORT_TASK: ref_tag: %llu already complete," - " skipping\n", ref_tag); - spin_unlock(&se_cmd->t_state_lock); + /* + * Obtain cmd_kref and move to list for shutdown processing + * if se_cmd->cmd_kref is still active, the command has not + * already reached CMD_T_COMPLETE + */ + if (!__target_check_io_state(se_cmd)) { spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags); goto out; } - se_cmd->transport_state |= CMD_T_ABORTED; - spin_unlock(&se_cmd->t_state_lock); - list_del_init(&se_cmd->se_cmd_list); - kref_get(&se_cmd->cmd_kref); spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags); cancel_work_sync(&se_cmd->work); transport_wait_for_tasks(se_cmd); - target_put_sess_cmd(se_cmd); transport_cmd_finish_abort(se_cmd, true); + target_put_sess_cmd(se_cmd); printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for" " ref_tag: %llu\n", ref_tag); @@ -237,8 +256,10 @@ static void core_tmr_drain_state_list( struct list_head *preempt_and_abort_list) { LIST_HEAD(drain_task_list); + struct se_session *sess; struct se_cmd *cmd, *next; unsigned long flags; + int rc; /* * Complete outstanding commands with TASK_ABORTED SAM status. @@ -277,6 +298,24 @@ static void core_tmr_drain_state_list( if (prout_cmd == cmd) continue; + sess = cmd->se_sess; + if (!sess) { + pr_err("NULL cmd->sess for tag: %llu\n", cmd->tag); + dump_stack(); + continue; + } + /* + * Obtain cmd_kref and move to list for shutdown processing + * if se_cmd->cmd_kref is still active, the command has not + * already reached CMD_T_COMPLETE + */ + spin_lock(&sess->sess_cmd_lock); + rc = __target_check_io_state(cmd); + spin_unlock(&sess->sess_cmd_lock); + if (!rc) { + printk("LUN_RESET I/O: non-zero kref_get_unless_zero\n"); + continue; + } list_move_tail(&cmd->state_list, &drain_task_list); cmd->state_active = false; } @@ -308,16 +347,11 @@ static void core_tmr_drain_state_list( * loop above, but we do it down here given that * cancel_work_sync may block. */ - if (cmd->t_state == TRANSPORT_COMPLETE) - cancel_work_sync(&cmd->work); - - spin_lock_irqsave(&cmd->t_state_lock, flags); - target_stop_cmd(cmd, &flags); - - cmd->transport_state |= CMD_T_ABORTED; - spin_unlock_irqrestore(&cmd->t_state_lock, flags); + cancel_work_sync(&cmd->work); + transport_wait_for_tasks(cmd); core_tmr_handle_tas_abort(tmr_nacl, cmd, tas); + target_put_sess_cmd(cmd); } } diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index c5035b9..f2c596b 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -626,6 +626,8 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd) void transport_cmd_finish_abort(struct se_cmd *cmd, int remove) { + bool ack_kref = (cmd->se_cmd_flags & SCF_ACK_KREF); + if (cmd->se_cmd_flags & SCF_SE_LUN_CMD) transport_lun_remove_cmd(cmd); /* @@ -637,7 +639,7 @@ void transport_cmd_finish_abort(struct se_cmd *cmd, int remove) if (transport_cmd_check_stop_to_fabric(cmd)) return; - if (remove) + if (remove && ack_kref) transport_put_cmd(cmd); } @@ -2219,20 +2221,14 @@ static inline void transport_free_pages(struct se_cmd *cmd) } /** - * transport_release_cmd - free a command - * @cmd: command to free + * transport_put_cmd - release a reference to a command + * @cmd: command to release * - * This routine unconditionally frees a command, and reference counting - * or list removal must be done in the caller. + * This routine releases our reference to the command and frees it if possible. */ -static int transport_release_cmd(struct se_cmd *cmd) +static int transport_put_cmd(struct se_cmd *cmd) { BUG_ON(!cmd->se_tfo); - - if (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB) - core_tmr_release_req(cmd->se_tmr_req); - if (cmd->t_task_cdb != cmd->__t_task_cdb) - kfree(cmd->t_task_cdb); /* * If this cmd has been setup with target_get_sess_cmd(), drop * the kref and call ->release_cmd() in kref callback. @@ -2240,18 +2236,6 @@ static int transport_release_cmd(struct se_cmd *cmd) return target_put_sess_cmd(cmd); } -/** - * transport_put_cmd - release a reference to a command - * @cmd: command to release - * - * This routine releases our reference to the command and frees it if possible. - */ -static int transport_put_cmd(struct se_cmd *cmd) -{ - transport_free_pages(cmd); - return transport_release_cmd(cmd); -} - void *transport_kmap_data_sg(struct se_cmd *cmd) { struct scatterlist *sg = cmd->t_data_sg; @@ -2456,7 +2440,7 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks) if (wait_for_tasks && (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)) transport_wait_for_tasks(cmd); - ret = transport_release_cmd(cmd); + ret = transport_put_cmd(cmd); } else { if (wait_for_tasks) transport_wait_for_tasks(cmd); @@ -2495,8 +2479,10 @@ int target_get_sess_cmd(struct se_cmd *se_cmd, bool ack_kref) * fabric acknowledgement that requires two target_put_sess_cmd() * invocations before se_cmd descriptor release. */ - if (ack_kref) + if (ack_kref) { + se_cmd->se_cmd_flags |= SCF_ACK_KREF; kref_get(&se_cmd->cmd_kref); + } spin_lock_irqsave(&se_sess->sess_cmd_lock, flags); if (se_sess->sess_tearing_down) { @@ -2514,6 +2500,16 @@ out: } EXPORT_SYMBOL(target_get_sess_cmd); +static void target_free_cmd_mem(struct se_cmd *cmd) +{ + transport_free_pages(cmd); + + if (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB) + core_tmr_release_req(cmd->se_tmr_req); + if (cmd->t_task_cdb != cmd->__t_task_cdb) + kfree(cmd->t_task_cdb); +} + static void target_release_cmd_kref(struct kref *kref) __releases(&se_cmd->se_sess->sess_cmd_lock) { @@ -2522,17 +2518,20 @@ static void target_release_cmd_kref(struct kref *kref) if (list_empty(&se_cmd->se_cmd_list)) { spin_unlock(&se_sess->sess_cmd_lock); + target_free_cmd_mem(se_cmd); se_cmd->se_tfo->release_cmd(se_cmd); return; } if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) { spin_unlock(&se_sess->sess_cmd_lock); + target_free_cmd_mem(se_cmd); complete(&se_cmd->cmd_wait_comp); return; } list_del(&se_cmd->se_cmd_list); spin_unlock(&se_sess->sess_cmd_lock); + target_free_cmd_mem(se_cmd); se_cmd->se_tfo->release_cmd(se_cmd); } diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index a4bed07..1060c52 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -140,6 +140,7 @@ enum se_cmd_flags_table { SCF_COMPARE_AND_WRITE = 0x00080000, SCF_COMPARE_AND_WRITE_POST = 0x00100000, SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x00200000, + SCF_ACK_KREF = 0x00400000, }; /* struct se_dev_entry->lun_flags and struct se_lun->lun_access */