From patchwork Fri May 5 09:42:50 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoph Hellwig X-Patchwork-Id: 9713225 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 63A7960235 for ; Fri, 5 May 2017 09:42:54 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6E28928630 for ; Fri, 5 May 2017 09:42:54 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 5ED63286BC; Fri, 5 May 2017 09:42:54 +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 B047128630 for ; Fri, 5 May 2017 09:42:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751281AbdEEJmw (ORCPT ); Fri, 5 May 2017 05:42:52 -0400 Received: from verein.lst.de ([213.95.11.211]:54063 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751249AbdEEJmw (ORCPT ); Fri, 5 May 2017 05:42:52 -0400 Received: by newverein.lst.de (Postfix, from userid 2407) id 5DC4D7F339; Fri, 5 May 2017 11:42:50 +0200 (CEST) Date: Fri, 5 May 2017 11:42:50 +0200 From: Christoph Hellwig To: Bart Van Assche Cc: Nicholas Bellinger , target-devel@vger.kernel.org, Hannes Reinecke , Christoph Hellwig , Andy Grover , David Disseldorp , stable@vger.kernel.org Subject: Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands Message-ID: <20170505094250.GA6138@lst.de> References: <20170504225102.8931-1-bart.vanassche@sandisk.com> <20170504225102.8931-7-bart.vanassche@sandisk.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170504225102.8931-7-bart.vanassche@sandisk.com> User-Agent: Mutt/1.5.17 (2007-11-01) 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 Hi Bart, what do you think about the variant below instead which avoids overloading the size variable? --- From 206696ec37cf4f6efe093964c2bdc96100de1f62 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 5 May 2017 11:40:38 +0200 Subject: target: fix and cleanup size calculation in sbc_parse_cdb Calculate the data buffer size individually for each command instead of trying to generalize it. This fixes the size calculation for VERIFY and WRITE_VERIFY, while making the code a lot easier to understand. Fixes: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing") Reported-by: Bart Van Assche Signed-off-by: Christoph Hellwig --- drivers/target/target_core_sbc.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index 5807123214e5..0bd879b9ce38 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -899,12 +899,14 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) switch (cdb[0]) { case READ_6: sectors = transport_get_sectors_6(cdb); + size = sbc_get_size(cmd, sectors); cmd->t_task_lba = transport_lba_21(cdb); cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB; cmd->execute_cmd = sbc_execute_rw; break; case READ_10: sectors = transport_get_sectors_10(cdb); + size = sbc_get_size(cmd, sectors); cmd->t_task_lba = transport_lba_32(cdb); if (sbc_check_dpofua(dev, cmd, cdb)) @@ -919,6 +921,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) break; case READ_12: sectors = transport_get_sectors_12(cdb); + size = sbc_get_size(cmd, sectors); cmd->t_task_lba = transport_lba_32(cdb); if (sbc_check_dpofua(dev, cmd, cdb)) @@ -933,6 +936,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) break; case READ_16: sectors = transport_get_sectors_16(cdb); + size = sbc_get_size(cmd, sectors); cmd->t_task_lba = transport_lba_64(cdb); if (sbc_check_dpofua(dev, cmd, cdb)) @@ -947,12 +951,14 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) break; case WRITE_6: sectors = transport_get_sectors_6(cdb); + size = sbc_get_size(cmd, sectors); cmd->t_task_lba = transport_lba_21(cdb); cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB; cmd->execute_cmd = sbc_execute_rw; break; case WRITE_10: sectors = transport_get_sectors_10(cdb); + size = sbc_get_size(cmd, sectors); cmd->t_task_lba = transport_lba_32(cdb); if (sbc_check_dpofua(dev, cmd, cdb)) @@ -974,6 +980,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) goto check_lba; case WRITE_12: sectors = transport_get_sectors_12(cdb); + size = sbc_get_size(cmd, sectors); cmd->t_task_lba = transport_lba_32(cdb); if (sbc_check_dpofua(dev, cmd, cdb)) @@ -988,6 +995,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) break; case WRITE_16: sectors = transport_get_sectors_16(cdb); + size = sbc_get_size(cmd, sectors); cmd->t_task_lba = transport_lba_64(cdb); if (sbc_check_dpofua(dev, cmd, cdb)) @@ -1005,6 +1013,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) !(cmd->se_cmd_flags & SCF_BIDI)) return TCM_INVALID_CDB_FIELD; sectors = transport_get_sectors_10(cdb); + size = sbc_get_size(cmd, sectors); if (sbc_check_dpofua(dev, cmd, cdb)) return TCM_INVALID_CDB_FIELD; @@ -1024,6 +1033,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) switch (service_action) { case XDWRITEREAD_32: sectors = transport_get_sectors_32(cdb); + size = sbc_get_size(cmd, sectors); if (sbc_check_dpofua(dev, cmd, cdb)) return TCM_INVALID_CDB_FIELD; @@ -1116,7 +1126,13 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) sectors = transport_get_sectors_16(cdb); cmd->t_task_lba = transport_lba_64(cdb); } + + /* + * XXX: why treat sectors / size check differently for + * the offload / non-offload cases? + */ if (ops->execute_sync_cache) { + size = sbc_get_size(cmd, sectors); cmd->execute_cmd = ops->execute_sync_cache; goto check_lba; } @@ -1205,9 +1221,6 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) end_lba, cmd->t_task_lba, sectors); return TCM_ADDRESS_OUT_OF_RANGE; } - - if (!(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE)) - size = sbc_get_size(cmd, sectors); } return target_cmd_size_check(cmd, size);