Received: by 2002:a05:7412:8d10:b0:f3:1519:9f41 with SMTP id bj16csp5699564rdb; Wed, 13 Dec 2023 17:27:08 -0800 (PST) X-Google-Smtp-Source: AGHT+IEluvBi+NyM+/QHC2NSy1QYNUkEFflfbEBgPpI+qdy7VJA2vdD74XMWQz9Ycwza03i+H1Iz X-Received: by 2002:a17:902:988e:b0:1d0:6ffe:a2c with SMTP id s14-20020a170902988e00b001d06ffe0a2cmr7563863plp.138.1702517128006; Wed, 13 Dec 2023 17:25:28 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1702517127; cv=none; d=google.com; s=arc-20160816; b=hmnA6XJ+wT6/8VIg8vJpbj9EGyKi3XsCdu/mOcsLE5WSwULUvH8ahWS/neZ5vhaqrL ThfwUlufbe1Ys4UQZDzVja3Jv1xg/LiM9Z+tVM2CSqDyc1oefYScYUoTAnEvAGdETWQE 3/BhXMOyds2YzXlpWxZyhPJqu6ze3K/2J/y7MDC6D0Ic9MRFSqaGqa2IE6GMi84XVmYc jTT8ChAZ0XDOsqSki0fWtMG0JiNDRrmSx4GfQtVMivRWmjeS3tPm1NBcZHEAYIUrYsCw xCpNxMKQ1ioM9zKF+bFnTFzLaS0TiSpG1Ou8DVdwDM7tm/0eD2EfeoQleg8aZmSthqBL gQpQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:cc:to:subject :message-id:date:from:in-reply-to:references:mime-version :dkim-signature; bh=JHMtuMnMAbDQ3tdue8zOZbOg6oc/EvH9admOYuIg/XM=; fh=LDvMt9WwG7Yr0EZQ9gfQmczooo0pJS8Wxdsp4dkm6Rc=; b=poOxaCBOqKiQxxlfj6XXjqAF2Cqiv2kL+2VexzgtV73Mtg4FGB6oy53xAis8Sh/xjH vfquSZjQpBIORjhZ8+r9iRCRD313uoqmEdw7s8c6OweZoaKI/UfgoqRO7LUyblE85/KT 5dXw0hF5QLbDs5gK7ZsGsT360twb8uxB/euvYcOEO0REAfCqxI2Z3Nzxe+7YHJCsfhLD jVtIE+u8TVmwZ7nnyeL8pViClEoWGKrXmT0BFtReYGqcKqYAWy8OeaZx/kY/IaNry8j7 o60ZAnmIPFCEqwEclBYBNDi4UVf5wGS11vn+TovNxgFflgNQE7gs1HTPPvdg6nbn0hpI aRBw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@suse.com header.s=google header.b=CLQMHNkg; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=suse.com Return-Path: Received: from howler.vger.email (howler.vger.email. [2620:137:e000::3:4]) by mx.google.com with ESMTPS id x18-20020a170902ec9200b001d0c7c691a4si10658411plg.365.2023.12.13.17.25.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 13 Dec 2023 17:25:27 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4; Authentication-Results: mx.google.com; dkim=pass header.i=@suse.com header.s=google header.b=CLQMHNkg; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=suse.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by howler.vger.email (Postfix) with ESMTP id 3FA16809348D; Wed, 13 Dec 2023 17:25:25 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at howler.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1442933AbjLNBZC (ORCPT + 99 others); Wed, 13 Dec 2023 20:25:02 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45282 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234163AbjLNBZC (ORCPT ); Wed, 13 Dec 2023 20:25:02 -0500 Received: from mail-ed1-x531.google.com (mail-ed1-x531.google.com [IPv6:2a00:1450:4864:20::531]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A1FD5E0 for ; Wed, 13 Dec 2023 17:25:06 -0800 (PST) Received: by mail-ed1-x531.google.com with SMTP id 4fb4d7f45d1cf-54cde11d0f4so10440957a12.2 for ; Wed, 13 Dec 2023 17:25:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1702517105; x=1703121905; darn=vger.kernel.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=JHMtuMnMAbDQ3tdue8zOZbOg6oc/EvH9admOYuIg/XM=; b=CLQMHNkgx49CAyQjPOw+Cxufa1zazaZ2py7fEQ25e979Mvf9HqcedQYJgTo6qAzkz0 dgFR98pe5bS+EuAKtL9kw9IK9sjnamnR8WbRqCHDeI2zfww/eMIy+aearqlEv1LNAEo2 EMiS28vMv1yO+k0AcH0X+/T9ccGguwPL+ylQah/Ft3OYMmWFxmGTpeqRG1uzY3FEFy+I xcOlIYNMjAymiBEKWocjV9TPr9njW41ptuzkSBEF4wupU3uAUlmt9Z2HmIXHYSJnjDD7 mTNhnisKrCsJMyABLVyj3/ZY++XQP1HubbAc/S71gXb4JlWIEuhc7BJOjYSxWkOT1Ux0 1r4g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702517105; x=1703121905; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=JHMtuMnMAbDQ3tdue8zOZbOg6oc/EvH9admOYuIg/XM=; b=TG16m9zIaKNDb3AjvwhPHpbXFvukqYtO2rcrWifYMcZFGsTB85P7S1OxW/Ce7QBgro IzTN0Q+Q8hwd9xDzWpQ+y00sEMHz8KUEZW+sDJYsc8+h33lqsLfZ6GfNbb6poAl5fYvD +zdoxjGFAsKvcvvBC5wVm7lDazZ51pwjMtV003KgDEoBm4G6y72MacxwftrwdPhuHbz+ xES3+YFtCdIau+r8uMGuf26eBZk9nKsoWNc6wPKT6LhfzGe4z6C6HZ8Of3JZC0frAqnR mwNwa23gAJjowtrXOvgmgj03WX4X1fp7mIvDyfVYwIBscpBnLXQ0C5smQ7yHBDv3XeYB gQsA== X-Gm-Message-State: AOJu0YxJqA9WBAA4ZE7jDJOK/E2/JPE9CJa5yc4ghPCauJHXhLbV3cGT V/wT0PUav7dmmj509JlZDdWWRJjrn7GoDLKx5gYxoA== X-Received: by 2002:a17:906:739b:b0:a22:f37a:b425 with SMTP id f27-20020a170906739b00b00a22f37ab425mr1292007ejl.61.1702517105116; Wed, 13 Dec 2023 17:25:05 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Lee Duncan Date: Wed, 13 Dec 2023 17:24:54 -0800 Message-ID: Subject: Re: [PATCH 1/2] scsi: target: iscsi: handle SCSI immediate commands To: Chris Leech Cc: target-devel@vger.kernel.org, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, dbond@suse.com, hare@suse.de, michael.christie@oracle.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Wed, 13 Dec 2023 17:25:25 -0800 (PST) Apologies on my first reply having HTML. I'm learning a new MUA. On Wed, Dec 13, 2023 at 12:06=E2=80=AFPM Chris Leech wr= ote: > > On Thu, Dec 07, 2023 at 09:42:34AM -0800, lduncan@suse.com wrote: > > From: Lee Duncan > > > > Some iSCSI initiators send SCSI PDUs with the "immediate" bit > > set, and this is allowed according to RFC 3720. Commands with > > the "Immediate" bit set are called "immediate commands". From > > section 3.2.2.1. "Command Numbering and Acknowledging": > > > > The target MUST NOT transmit a MaxCmdSN that is less than > > ExpCmdSN-1. For non-immediate commands, the CmdSN field can take an= y > > value from ExpCmdSN to MaxCmdSN inclusive. The target MUST silently > > ignore any non-immediate command outside of this range or non- > > immediate duplicates within the range. The CmdSN carried by > > immediate commands may lie outside the ExpCmdSN to MaxCmdSN range. > > For example, if the initiator has previously sent a non-immediate > > command carrying the CmdSN equal to MaxCmdSN, the target window is > > closed. For group task management commands issued as immediate > > commands, CmdSN indicates the scope of the group action (e.g., on > > ABORT TASK SET indicates which commands are aborted). > > > > This fixed an issue with fastlinq qedi Converged Network Adapter > > initiator firmware, trying to use an LIO target for booting. These > > changes made booting possible, with or without ImmediateData enabled. > > > > Signed-off-by: Lee Duncan > > Reviewed-by: David Bond > > --- > > drivers/target/iscsi/iscsi_target.c | 12 +++--------- > > drivers/target/iscsi/iscsi_target_util.c | 10 ++++++++-- > > 2 files changed, 11 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi= /iscsi_target.c > > index 1d25e64b068a..f246e5015868 100644 > > --- a/drivers/target/iscsi/iscsi_target.c > > +++ b/drivers/target/iscsi/iscsi_target.c > > @@ -1060,13 +1060,6 @@ int iscsit_setup_scsi_cmd(struct iscsit_conn *co= nn, struct iscsit_cmd *cmd, > > ISCSI_REASON_BOOKMARK_INVALI= D, buf); > > } > > > > - if (hdr->opcode & ISCSI_OP_IMMEDIATE) { > > - pr_err("Illegally set Immediate Bit in iSCSI Initiator" > > - " Scsi Command PDU.\n"); > > - return iscsit_add_reject_cmd(cmd, > > - ISCSI_REASON_BOOKMARK_INVALI= D, buf); > > - } > > - > > if (payload_length && !conn->sess->sess_ops->ImmediateData) { > > pr_err("ImmediateData=3DNo but DataSegmentLength=3D%u," > > " protocol error.\n", payload_length); > > This seems right, as the flag is checked again later in the same > function. > > > @@ -1255,14 +1248,15 @@ int iscsit_process_scsi_cmd(struct iscsit_conn = *conn, struct iscsit_cmd *cmd, > > /* > > * Check the CmdSN against ExpCmdSN/MaxCmdSN here if > > * the Immediate Bit is not set, and no Immediate > > - * Data is attached. > > + * Data is attached. Also skip the check if this is > > + * an immediate command. > > This comment addition seems redundant, isn't that what the "Immediate > Bit is not set" already means? The spec is confusing with respect to this. The "Immediate Bit" means an immediate command. These commands are done "now", not queued, and they do not increment the expected sequence number. Immediate data is different, and unfortunately named IMHO. It's when a PDU supplies the data for the SCSI command in the current PDU instead of the next PDU. > > > * > > * A PDU/CmdSN carrying Immediate Data can only > > * be processed after the DataCRC has passed. > > * If the DataCRC fails, the CmdSN MUST NOT > > * be acknowledged. (See below) > > */ > > - if (!cmd->immediate_data) { > > + if (!cmd->immediate_data && !cmd->immediate_cmd) { > > cmdsn_ret =3D iscsit_sequence_cmd(conn, cmd, > > (unsigned char *)hdr, hdr->cmdsn)= ; > > if (cmdsn_ret =3D=3D CMDSN_ERROR_CANNOT_RECOVER) > > Are you sure this needs to be checking both conditions here? I'm > struggling to understand why CmdSN checking would be bypassed for > immediate data. Is this a longstanding bug where the condition should > have been on immediate_cmd (and only immediate_cmd) instead? The immediate data check was there already, and there haven't been any bugs I know of, so I assumed that part of the code was ok. > > Or is this because of the handling the immediate data with DataCRC case > mentioned? I do see iscsit_sequence_cmd also being called in > iscsit_get_immediate_data. I will check that but I suspect you are correct. > > - Chris Leech >