From patchwork Thu Jul 13 19:27:17 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Nicholas A. Bellinger" X-Patchwork-Id: 9839517 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 BDF8660A4C for ; Thu, 13 Jul 2017 18:02:57 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 285B828759 for ; Thu, 13 Jul 2017 20:58:58 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 269EE288A4; Thu, 13 Jul 2017 20:58:58 +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 EA98028759 for ; Thu, 13 Jul 2017 20:58:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752831AbdGMT1U (ORCPT ); Thu, 13 Jul 2017 15:27:20 -0400 Received: from mail.linux-iscsi.org ([67.23.28.174]:38305 "EHLO linux-iscsi.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752735AbdGMT1T (ORCPT ); Thu, 13 Jul 2017 15:27:19 -0400 Received: from [172.16.2.183] (unknown [50.225.59.10]) (using SSLv3 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: nab) by linux-iscsi.org (Postfix) with ESMTPSA id 00D2440B19; Thu, 13 Jul 2017 19:32:12 +0000 (UTC) Message-ID: <1499974037.7987.24.camel@haakon3.daterainc.com> Subject: Re: [PATCH] iscsi-target: Reject immediate data underflow larger than SCSI transfer length From: "Nicholas A. Bellinger" To: Bart Van Assche Cc: "linux-scsi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "mchristi@redhat.com" , "roland@purestorage.com" , "target-devel@vger.kernel.org" , "martin.petersen@oracle.com" , "hare@suse.de" Date: Thu, 13 Jul 2017 12:27:17 -0700 In-Reply-To: <1499789824.2586.16.camel@wdc.com> References: <1496895685-18464-1-git-send-email-nab@linux-iscsi.org> <1496936253.3028.1.camel@wdc.com> <1496991302.28997.66.camel@haakon3.risingtidesystems.com> <1499757761.28145.45.camel@haakon3.daterainc.com> <1499789824.2586.16.camel@wdc.com> X-Mailer: Evolution 3.4.4-1 Mime-Version: 1.0 Sender: target-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: target-devel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, 2017-07-11 at 16:17 +0000, Bart Van Assche wrote: > On Tue, 2017-07-11 at 00:22 -0700, Nicholas A. Bellinger wrote: > > So rejecting this case as already done in commit abb85a9b51 is the > > correct approach for >= v4.3.y. > > Hello Nic, > > I hope that you agree that the current target_cmd_size_check() implementation > is complicated and ugly. Patch 30/33 of the patch series I referred to in my > e-mail removes a significant number of lines of code from that function. So > my patch series not only makes target_cmd_size_check() easier to maintain and > to verify but it makes that function also faster. Hence please reconsider the > approach from my patch series. For patch 30/33, see also > https://www.spinics.net/lists/target-devel/msg15384.html. For reference, here is the patch your advocating: The original code is not 'complicated' or 'ugly', and the proposed change doesn't make anything 'faster' nor removes a 'significant' number of LoC. So no, I don't buy any of that line of reasoning. ;-) It comes down to two items. First, if SCF_SCSI_DATA_CDBs with underflow/overflow will be allowed, and second the block_size != 512 check. For the former, I've still never seen a host environment in the wild over the last 15 years that generates underflow/overflow for DATA CDBs with an LBA. So I'm reluctant to randomly allow this for all cases and fabrics, considering no host environment actually requires it. That said, I do understand libiscsi generates this for WRITE_VERIFY, but I'm still undecided if that's a good enough a reason to enable it for all cases in upstream. For the latter item, it's fine to drop the legacy block_size != 512 check, and I'll take a patch for that separate from the other bit. --- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index c28e3b58150b..6cd49fe578a7 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1164,23 +1164,6 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size) " %u does not match SCSI CDB Length: %u for SAM Opcode:" " 0x%02x\n", cmd->se_tfo->get_fabric_name(), cmd->data_length, size, cmd->t_task_cdb[0]); - - if (cmd->data_direction == DMA_TO_DEVICE && - cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) { - pr_err("Rejecting underflow/overflow WRITE data\n"); - return TCM_INVALID_CDB_FIELD; - } - /* - * Reject READ_* or WRITE_* with overflow/underflow for - * type SCF_SCSI_DATA_CDB. - */ - if (dev->dev_attrib.block_size != 512) { - pr_err("Failing OVERFLOW/UNDERFLOW for LBA op" - " CDB on non 512-byte sector setup subsystem" - " plugin: %s\n", dev->transport->name); - /* Returns CHECK_CONDITION + INVALID_CDB_FIELD */ - return TCM_INVALID_CDB_FIELD; - } /* * For the overflow case keep the existing fabric provided * ->data_length. Otherwise for the underflow case, reset