From patchwork Tue Dec 8 00:48:59 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Himanshu Madhani X-Patchwork-Id: 7793211 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 0353C9F349 for ; Tue, 8 Dec 2015 01:30:16 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id CB99B204EB for ; Tue, 8 Dec 2015 01:30:14 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9C53B204B5 for ; Tue, 8 Dec 2015 01:30:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932767AbbLHB34 (ORCPT ); Mon, 7 Dec 2015 20:29:56 -0500 Received: from mx0b-0016ce01.pphosted.com ([67.231.156.153]:13237 "EHLO mx0b-0016ce01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932610AbbLHB3d (ORCPT ); Mon, 7 Dec 2015 20:29:33 -0500 Received: from pps.filterd (m0085408.ppops.net [127.0.0.1]) by mx0b-0016ce01.pphosted.com (8.15.0.59/8.15.0.59) with SMTP id tB81R5h1005714; Mon, 7 Dec 2015 17:29:32 -0800 Received: from avcashub1.qlogic.com (avcashub1.qlogic.com [198.70.193.115]) by mx0b-0016ce01.pphosted.com with ESMTP id 1ykystkquc-4 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NOT); Mon, 07 Dec 2015 17:29:32 -0800 Received: from dut6217.mv.qlogic.com (172.29.56.217) by qlc.com (10.1.4.190) with Microsoft SMTP Server id 14.3.235.1; Mon, 7 Dec 2015 17:29:29 -0800 Received: by dut6217.mv.qlogic.com (Postfix, from userid 0) id 708E3522134; Mon, 7 Dec 2015 19:49:08 -0500 (EST) From: Himanshu Madhani To: , CC: , , Subject: [PATCH 12/20] target/tmr: LUN reset cause cmd premature free. Date: Mon, 7 Dec 2015 19:48:59 -0500 Message-ID: <1449535747-2850-13-git-send-email-himanshu.madhani@qlogic.com> X-Mailer: git-send-email 1.7.7 In-Reply-To: <1449535747-2850-1-git-send-email-himanshu.madhani@qlogic.com> References: <1449535747-2850-1-git-send-email-himanshu.madhani@qlogic.com> MIME-Version: 1.0 disclaimer: bypass X-Proofpoint-Virus-Version: vendor=nai engine=5700 definitions=8008 signatures=670664 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 clxscore=1015 suspectscore=2 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1507310000 definitions=main-1512080026 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.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, 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: Quinn Tran During LUN/Target reset, the TMR code attempt to intercept cmds and try to aborted them. Current code assume cmds are always intercepted at the back end device. The cleanup code would issue a "queue_status() & check_stop_free()" to terminate the command. However, when a cmd is intercepted at the front end/Fabric layer, current code introduce premature free or cause Fabric to double free. When command is intercepted at Fabric layer, it means a check_stop_free(cmd_kref--) has been called. The extra check_stop_free in the Lun Reset cleanup code causes early free. When a cmd in the Fabric layer is completed, the normal free code adds another another free which introduce a double free. To fix the issue: - add a new flag/CMD_T_SENT_STATUS to track command that have made it down to fabric layer after back end good/bad completion. - if cmd reach Fabric Layer at Lun Reset time, add an extra cmd_kref count to prevent premature free. Signed-off-by: Quinn Tran Signed-off-by: Himanshu Madhani --- drivers/target/target_core_tmr.c | 33 +++++++++++++++++++++++++++++++- drivers/target/target_core_transport.c | 30 +++++++++++++++++++++++++++++ include/target/target_core_base.h | 1 + 3 files changed, 63 insertions(+), 1 deletions(-) diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c index 28fb301..41f8b57 100644 --- a/drivers/target/target_core_tmr.c +++ b/drivers/target/target_core_tmr.c @@ -243,7 +243,9 @@ static void core_tmr_drain_state_list( { LIST_HEAD(drain_task_list); struct se_cmd *cmd, *next; - unsigned long flags; + unsigned long flags, flags2; + int rmkref; + struct se_session *se_sess; /* * Complete outstanding commands with TASK_ABORTED SAM status. @@ -282,6 +284,16 @@ static void core_tmr_drain_state_list( if (prout_cmd == cmd) continue; + se_sess = cmd->se_sess; + /* take an extra kref to prevent cmd free race condition. */ + spin_lock_irqsave(&se_sess->sess_cmd_lock, flags2); + if (!kref_get_unless_zero(&cmd->cmd_kref)) { + /* cmd is already in the free process */ + spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags2); + continue; + } + spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags2); + list_move_tail(&cmd->state_list, &drain_task_list); cmd->state_active = false; } @@ -320,9 +332,28 @@ static void core_tmr_drain_state_list( target_stop_cmd(cmd, &flags); cmd->transport_state |= CMD_T_ABORTED; + + /* CMD_T_SENT_STATUS: cmd is down in fabric layer. + * A check stop has been called. Keep the extra kref + * from above because core_tmr_handle_tas_abort will + * generate another check_stop. + * + * !CMD_T_SENT_STATUS: cmd intercepted at back end. + * Remove the extra kref from above because only + * 1 check_stop is required or generated by + * core_tmr_handle_tas_abort() + */ + rmkref = 0; + if (!((cmd->t_state == TRANSPORT_COMPLETE) && + (cmd->transport_state & CMD_T_SENT_STATUS))) + rmkref = 1; + spin_unlock_irqrestore(&cmd->t_state_lock, flags); core_tmr_handle_tas_abort(tmr_nacl, cmd, tas); + + if (rmkref) + target_put_sess_cmd(cmd); } } diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 4fdcee2..cdd18bf 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -639,9 +639,14 @@ void transport_cmd_finish_abort(struct se_cmd *cmd, int remove) static void target_complete_failure_work(struct work_struct *work) { struct se_cmd *cmd = container_of(work, struct se_cmd, work); + unsigned long flags; transport_generic_request_failure(cmd, TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE); + + spin_lock_irqsave(&cmd->t_state_lock, flags); + cmd->transport_state |= CMD_T_SENT_STATUS; + spin_unlock_irqrestore(&cmd->t_state_lock, flags); } /* @@ -1659,6 +1664,7 @@ void transport_generic_request_failure(struct se_cmd *cmd, sense_reason_t sense_reason) { int ret = 0, post_ret = 0; + unsigned long flags; pr_debug("-----[ Storage Engine Exception for cmd: %p ITT: 0x%08llx" " CDB: 0x%02x\n", cmd, cmd->tag, cmd->t_task_cdb[0]); @@ -1670,6 +1676,10 @@ void transport_generic_request_failure(struct se_cmd *cmd, (cmd->transport_state & CMD_T_STOP) != 0, (cmd->transport_state & CMD_T_SENT) != 0); + spin_lock_irqsave(&cmd->t_state_lock, flags); + cmd->transport_state |= CMD_T_SENT_STATUS; + spin_unlock_irqrestore(&cmd->t_state_lock, flags); + /* * For SAM Task Attribute emulation for failed struct se_cmd */ @@ -1951,6 +1961,7 @@ static void transport_complete_task_attr(struct se_cmd *cmd) static void transport_complete_qf(struct se_cmd *cmd) { int ret = 0; + unsigned long flags; transport_complete_task_attr(cmd); @@ -1986,6 +1997,10 @@ out: } transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); + + spin_lock_irqsave(&cmd->t_state_lock, flags); + cmd->transport_state |= CMD_T_SENT_STATUS; + spin_unlock_irqrestore(&cmd->t_state_lock, flags); } static void transport_handle_queue_full( @@ -2032,6 +2047,7 @@ static void target_complete_ok_work(struct work_struct *work) { struct se_cmd *cmd = container_of(work, struct se_cmd, work); int ret; + unsigned long flags; /* * Check if we need to move delayed/dormant tasks from cmds on the @@ -2060,6 +2076,10 @@ static void target_complete_ok_work(struct work_struct *work) transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); + + spin_lock_irqsave(&cmd->t_state_lock, flags); + cmd->transport_state |= CMD_T_SENT_STATUS; + spin_unlock_irqrestore(&cmd->t_state_lock, flags); return; } /* @@ -2086,6 +2106,11 @@ static void target_complete_ok_work(struct work_struct *work) transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); + + spin_lock_irqsave(&cmd->t_state_lock, flags); + cmd->transport_state |= CMD_T_SENT_STATUS; + spin_unlock_irqrestore(&cmd->t_state_lock, flags); + return; } } @@ -2136,6 +2161,7 @@ queue_rsp: ret = cmd->se_tfo->queue_status(cmd); if (ret == -EAGAIN || ret == -ENOMEM) goto queue_full; + break; default: break; @@ -2143,6 +2169,10 @@ queue_rsp: transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); + + spin_lock_irqsave(&cmd->t_state_lock, flags); + cmd->transport_state |= CMD_T_SENT_STATUS; + spin_unlock_irqrestore(&cmd->t_state_lock, flags); return; queue_full: diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index aabf0ac..efccd71 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -490,6 +490,7 @@ struct se_cmd { #define CMD_T_DEV_ACTIVE (1 << 7) #define CMD_T_REQUEST_STOP (1 << 8) #define CMD_T_BUSY (1 << 9) +#define CMD_T_SENT_STATUS (1 << 10) spinlock_t t_state_lock; struct kref cmd_kref; struct completion t_transport_stop_comp;