[PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands.

[PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands.

[Parav Pandit]

sg_cnt cannot be zero for inline or sge mode from the fabric drivers
because nvme read/write commands have minimum of one block to be
read or written. (nlb dw12 is 0 based value).
So sg_cnt check for 0 is not useful.

Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
Signed-off-by: Parav Pandit <parav at mellanox.com>
---
 drivers/nvme/target/io-cmd.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/nvme/target/io-cmd.c b/drivers/nvme/target/io-cmd.c
index 4195115..e6a3a8a 100644
--- a/drivers/nvme/target/io-cmd.c
+++ b/drivers/nvme/target/io-cmd.c
@@ -49,11 +49,6 @@ static void nvmet_execute_rw(struct nvmet_req *req)
 	blk_qc_t cookie;
 	int op, op_flags = 0, i;
 
-	if (!req->sg_cnt) {
-		nvmet_req_complete(req, 0);
-		return;
-	}
-
 	if (req->cmd->rw.opcode == nvme_cmd_write) {
 		op = REQ_OP_WRITE;
 		op_flags = REQ_SYNC | REQ_IDLE;
-- 
1.8.3.1

[PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands.

[Christoph Hellwig]

On Tue, Feb 07, 2017@05:35:57PM -0600, Parav Pandit wrote:
> sg_cnt cannot be zero for inline or sge mode from the fabric drivers
> because nvme read/write commands have minimum of one block to be
> read or written. (nlb dw12 is 0 based value).
> So sg_cnt check for 0 is not useful.

Except that we currently don't verify the data transfer in the SGL
matches that in the command.  It's been on my todo list for a while,
so maybe you can take it off my plate first before we'll aply this
patch.

[PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands.

[Sagi Grimberg]

> Except that we currently don't verify the data transfer in the SGL
> matches that in the command.  It's been on my todo list for a while,
> so maybe you can take it off my plate first before we'll aply this
> patch.

Any reason not to take it as is and add the sgl validity check
incrementally?

[PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands.

[Christoph Hellwig]

On Wed, Feb 08, 2017@11:43:23AM +0200, Sagi Grimberg wrote:
>
>> Except that we currently don't verify the data transfer in the SGL
>> matches that in the command.  It's been on my todo list for a while,
>> so maybe you can take it off my plate first before we'll aply this
>> patch.
>
> Any reason not to take it as is and add the sgl validity check
> incrementally?

Because that would allow a bad actor host to crash the target easily.

[PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands.

[Parav Pandit]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch at lst.de]
> Sent: Wednesday, February 8, 2017 2:24 AM
> To: Parav Pandit <parav at mellanox.com>
> Cc: hch at lst.de; sagi at grimberg.me; linux-nvme at lists.infradead.org
> Subject: Re: [PATCH] nvmet: Do not check for sg_cnt being zero for
> read/write commands.
> 
> On Tue, Feb 07, 2017@05:35:57PM -0600, Parav Pandit wrote:
> > sg_cnt cannot be zero for inline or sge mode from the fabric drivers
> > because nvme read/write commands have minimum of one block to be
> read
> > or written. (nlb dw12 is 0 based value).
> > So sg_cnt check for 0 is not useful.
> 
> Except that we currently don't verify the data transfer in the SGL matches
> that in the command.  It's been on my todo list for a while, so maybe you can
> take it off my plate first before we'll aply this patch.

Yes. I hit an error when there was a mismatch between nlba and sge length.
That check is needed to fail the nvme requests. I will post that patch soon.

[PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands.

[Parav Pandit]

> -----Original Message-----
> From: Parav Pandit
> Sent: Wednesday, February 8, 2017 9:11 AM
> To: 'Christoph Hellwig' <hch at lst.de>
> Cc: sagi at grimberg.me; linux-nvme at lists.infradead.org
> Subject: RE: [PATCH] nvmet: Do not check for sg_cnt being zero for
> read/write commands.
> 
> 
> > -----Original Message-----
> > From: Christoph Hellwig [mailto:hch at lst.de]
> > Sent: Wednesday, February 8, 2017 2:24 AM
> > To: Parav Pandit <parav at mellanox.com>
> > Cc: hch at lst.de; sagi at grimberg.me; linux-nvme at lists.infradead.org
> > Subject: Re: [PATCH] nvmet: Do not check for sg_cnt being zero for
> > read/write commands.
> >
> > On Tue, Feb 07, 2017@05:35:57PM -0600, Parav Pandit wrote:
> > > sg_cnt cannot be zero for inline or sge mode from the fabric drivers
> > > because nvme read/write commands have minimum of one block to be
> > read
> > > or written. (nlb dw12 is 0 based value).
> > > So sg_cnt check for 0 is not useful.
> >
> > Except that we currently don't verify the data transfer in the SGL
> > matches that in the command.  It's been on my todo list for a while,
> > so maybe you can take it off my plate first before we'll aply this patch.
> 
> Yes. I hit an error when there was a mismatch between nlba and sge length.
> That check is needed to fail the nvme requests. I will post that patch soon.

However check for sg_cnt being not zero doesn't catch that error either, so both the checks are unrelated.
I think this patch still holds fine?