* [PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands.
@ 2017-02-07 23:35 Parav Pandit
2017-02-08 8:24 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: Parav Pandit @ 2017-02-07 23:35 UTC (permalink / raw)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands.
2017-02-07 23:35 [PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands Parav Pandit
@ 2017-02-08 8:24 ` Christoph Hellwig
2017-02-08 9:43 ` Sagi Grimberg
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Christoph Hellwig @ 2017-02-08 8:24 UTC (permalink / raw)
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands.
2017-02-08 8:24 ` Christoph Hellwig
@ 2017-02-08 9:43 ` Sagi Grimberg
2017-02-08 11:54 ` Christoph Hellwig
2017-02-08 15:11 ` Parav Pandit
2017-02-08 20:06 ` Parav Pandit
2 siblings, 1 reply; 6+ messages in thread
From: Sagi Grimberg @ 2017-02-08 9:43 UTC (permalink / raw)
> 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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands.
2017-02-08 9:43 ` Sagi Grimberg
@ 2017-02-08 11:54 ` Christoph Hellwig
0 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2017-02-08 11:54 UTC (permalink / raw)
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands.
2017-02-08 8:24 ` Christoph Hellwig
2017-02-08 9:43 ` Sagi Grimberg
@ 2017-02-08 15:11 ` Parav Pandit
2017-02-08 20:06 ` Parav Pandit
2 siblings, 0 replies; 6+ messages in thread
From: Parav Pandit @ 2017-02-08 15:11 UTC (permalink / raw)
> -----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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands.
2017-02-08 8:24 ` Christoph Hellwig
2017-02-08 9:43 ` Sagi Grimberg
2017-02-08 15:11 ` Parav Pandit
@ 2017-02-08 20:06 ` Parav Pandit
2 siblings, 0 replies; 6+ messages in thread
From: Parav Pandit @ 2017-02-08 20:06 UTC (permalink / raw)
> -----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?
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-08 20:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-07 23:35 [PATCH] nvmet: Do not check for sg_cnt being zero for read/write commands Parav Pandit
2017-02-08 8:24 ` Christoph Hellwig
2017-02-08 9:43 ` Sagi Grimberg
2017-02-08 11:54 ` Christoph Hellwig
2017-02-08 15:11 ` Parav Pandit
2017-02-08 20:06 ` Parav Pandit
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.